Skip to content

Commit 54ccd70

Browse files
keszybzkeszybz
authored
Merge pull request systemd#20744 from yuwata/udev-netlink
udev: use netlink more aggressively I'm pasting the comment from systemd#20744 (comment) which is quite informative. The code wasn't changed significantly since then: atenart commented 6 days ago: > I ran tests without (93caec7) and with this PR (06735f2) on Fedora, having a few udev rules > using attributes eligible to be cached and creating 50 veth on 4 CPUs. Although the time spent > running the test is variable between runs, I generally saw an improvement when using this PR, e.g: > > 249-910-g93caec7: > real 0m3.691s > user 0m0.022s > sys 0m1.338s > > 249-920-g06735f2: > real 0m2.950s > user 0m0.005s > sys 0m0.399s > > On a different system than the one used above, I even saw a 40% improvement; results depend > on many parameters (distro, udev rules, concurrent daemons accessing sysfs, etc.). > > Because it's quite hard to measure the improvement here (as the kernel behave differently between > the two test cases), I also ran tests using a modified kernel not hitting the trylock logic. There was > an improvement with this PR as well. (Take this with a grain of salt though, as the kernel was > modified not using patches approved upstream).
2 parents e4d294c + 1321f67 commit 54ccd70

13 files changed

+581
-246
lines changed

‎src/basic/ether-addr-util.c‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include <sys/types.h>
88

99
#include "ether-addr-util.h"
10+
#include "hexdecoct.h"
1011
#include "macro.h"
1112
#include "string-util.h"
1213

@@ -15,12 +16,13 @@ char* hw_addr_to_string(const struct hw_addr_data *addr, char buffer[HW_ADDR_TO_
1516
assert(buffer);
1617
assert(addr->length <= HW_ADDR_MAX_SIZE);
1718

18-
for (size_t i = 0; i < addr->length; i++) {
19-
sprintf(&buffer[3*i], "%02"PRIx8, addr->bytes[i]);
20-
if (i < addr->length - 1)
21-
buffer[3*i + 2] = ':';
19+
for (size_t i = 0, j = 0; i < addr->length; i++) {
20+
buffer[j++] = hexchar(addr->bytes[i] >> 4);
21+
buffer[j++] = hexchar(addr->bytes[i] & 0x0f);
22+
buffer[j++] = ':';
2223
}
2324

25+
buffer[addr->length > 0 ? addr->length * 3 - 1 : 0] = '\0';
2426
return buffer;
2527
}
2628

‎src/libsystemd/sd-device/device-private.h‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,15 @@ static inline int device_new_from_watch_handle(sd_device **ret, int wd) {
1818
}
1919

2020
int device_get_device_id(sd_device *device, const char **ret);
21-
2221
int device_get_devlink_priority(sd_device *device, int *priority);
2322
int device_get_watch_handle(sd_device *device);
2423
int device_get_devnode_mode(sd_device *device, mode_t *mode);
2524
int device_get_devnode_uid(sd_device *device, uid_t *uid);
2625
int device_get_devnode_gid(sd_device *device, gid_t *gid);
2726

27+
int device_cache_sysattr_value(sd_device *device, const char *key, char *value);
28+
int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value);
29+
2830
void device_seal(sd_device *device);
2931
void device_set_is_initialized(sd_device *device);
3032
int device_set_watch_handle(sd_device *device, int wd);

‎src/libsystemd/sd-device/sd-device.c‎

Lines changed: 15 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1941,7 +1941,7 @@ _public_ int sd_device_get_trigger_uuid(sd_device *device, sd_id128_t *ret) {
19411941
return 0;
19421942
}
19431943

1944-
static int device_cache_sysattr_value(sd_device *device, const char *key, char *value) {
1944+
int device_cache_sysattr_value(sd_device *device, const char *key, char *value) {
19451945
_unused_ _cleanup_free_ char *old_value = NULL;
19461946
_cleanup_free_ char *new_key = NULL;
19471947
int r;
@@ -1969,47 +1969,37 @@ static int device_cache_sysattr_value(sd_device *device, const char *key, char *
19691969
return 0;
19701970
}
19711971

1972-
static int device_get_cached_sysattr_value(sd_device *device, const char *_key, const char **_value) {
1973-
const char *key = NULL, *value;
1972+
int device_get_cached_sysattr_value(sd_device *device, const char *key, const char **ret_value) {
1973+
const char *k = NULL, *value;
19741974

19751975
assert(device);
1976-
assert(_key);
1977-
1978-
value = hashmap_get2(device->sysattr_values, _key, (void **) &key);
1979-
if (!key)
1980-
return -ENOENT;
1976+
assert(key);
19811977

1982-
if (_value)
1983-
*_value = value;
1978+
value = hashmap_get2(device->sysattr_values, key, (void **) &k);
1979+
if (!k)
1980+
return -ESTALE; /* We have not read the attribute. */
1981+
if (!value)
1982+
return -ENOENT; /* We have looked up the attribute before and it did not exist. */
1983+
if (ret_value)
1984+
*ret_value = value;
19841985
return 0;
19851986
}
19861987

19871988
/* We cache all sysattr lookups. If an attribute does not exist, it is stored
19881989
* with a NULL value in the cache, otherwise the returned string is stored */
19891990
_public_ int sd_device_get_sysattr_value(sd_device *device, const char *sysattr, const char **ret_value) {
19901991
_cleanup_free_ char *value = NULL;
1991-
const char *path, *syspath, *cached_value = NULL;
1992+
const char *path, *syspath;
19921993
struct stat statbuf;
19931994
int r;
19941995

19951996
assert_return(device, -EINVAL);
19961997
assert_return(sysattr, -EINVAL);
19971998

19981999
/* look for possibly already cached result */
1999-
r = device_get_cached_sysattr_value(device, sysattr, &cached_value);
2000-
if (r != -ENOENT) {
2001-
if (r < 0)
2002-
return r;
2003-
2004-
if (!cached_value)
2005-
/* we looked up the sysattr before and it did not exist */
2006-
return -ENOENT;
2007-
2008-
if (ret_value)
2009-
*ret_value = cached_value;
2010-
2011-
return 0;
2012-
}
2000+
r = device_get_cached_sysattr_value(device, sysattr, ret_value);
2001+
if (r != -ESTALE)
2002+
return r;
20132003

20142004
r = sd_device_get_syspath(device, &syspath);
20152005
if (r < 0)

‎src/udev/meson.build‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ libudevd_core_sources = '''
3333
udev-builtin-hwdb.c
3434
udev-builtin-input_id.c
3535
udev-builtin-keyboard.c
36-
udev-builtin-net_id-netlink.c
37-
udev-builtin-net_id-netlink.h
3836
udev-builtin-net_id.c
3937
udev-builtin-net_setup_link.c
4038
udev-builtin-path_id.c
4139
udev-builtin-usb_id.c
40+
udev-netlink.c
41+
udev-netlink.h
4242
net/link-config.c
4343
net/link-config.h
4444
'''.split()
@@ -212,9 +212,9 @@ tests += [
212212
[threads,
213213
libacl]],
214214

215-
[['src/udev/test-udev-builtin-net_id-netlink.c',
216-
'src/udev/udev-builtin-net_id-netlink.c',
217-
'src/udev/udev-builtin-net_id-netlink.h']],
215+
[['src/udev/test-udev-netlink.c',
216+
'src/udev/udev-netlink.c',
217+
'src/udev/udev-netlink.h']],
218218

219219
[['src/udev/fido_id/test-fido-id-desc.c',
220220
'src/udev/fido_id/fido_id_desc.c']],

‎src/udev/test-udev-builtin-net_id-netlink.c‎

Lines changed: 0 additions & 85 deletions
This file was deleted.

‎src/udev/test-udev-netlink.c‎

Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
/* SPDX-License-Identifier: LGPL-2.1-or-later */
2+
3+
#include "sd-device.h"
4+
5+
#include "arphrd-list.h"
6+
#include "ether-addr-util.h"
7+
#include "parse-util.h"
8+
#include "tests.h"
9+
#include "udev-netlink.h"
10+
11+
static void test_link_info_one(sd_netlink *rtnl, int ifindex) {
12+
_cleanup_(link_info_clear) LinkInfo info = LINK_INFO_NULL;
13+
_cleanup_(sd_device_unrefp) sd_device *dev = NULL, *dev_with_netlink = NULL;
14+
const char *s, *t;
15+
unsigned u;
16+
17+
log_debug("/* %s(ifindex=%i) */", __func__, ifindex);
18+
19+
assert_se(link_info_get(&rtnl, ifindex, &info) >= 0);
20+
assert_se(sd_device_new_from_ifindex(&dev, ifindex) >= 0);
21+
assert_se(sd_device_new_from_ifindex(&dev_with_netlink, ifindex) >= 0);
22+
23+
/* check iftype */
24+
log_debug("iftype: %"PRIu16" (%s)", info.iftype, strna(arphrd_to_name(info.iftype)));
25+
assert_se(sd_device_get_sysattr_value(dev, "type", &s) >= 0);
26+
assert_se(safe_atou(s, &u) >= 0);
27+
assert_se(u == info.iftype);
28+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "type", &s) >= 0);
29+
assert_se(safe_atou(s, &u) >= 0);
30+
assert_se(u == info.iftype);
31+
32+
/* check hardware address length */
33+
log_debug("hardware address length: %zu", info.hw_addr.length);
34+
assert_se(sd_device_get_sysattr_value(dev, "addr_len", &s) >= 0);
35+
assert_se(safe_atou(s, &u) >= 0);
36+
assert_se(u == info.hw_addr.length);
37+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "addr_len", &s) >= 0);
38+
assert_se(safe_atou(s, &u) >= 0);
39+
assert_se(u == info.hw_addr.length);
40+
41+
/* check hardware address */
42+
log_debug("hardware address: %s", HW_ADDR_TO_STR(&info.hw_addr));
43+
assert_se(sd_device_get_sysattr_value(dev, "address", &s) >= 0);
44+
assert_se(streq(s, HW_ADDR_TO_STR(&info.hw_addr)));
45+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "address", &s) >= 0);
46+
assert_se(streq(s, HW_ADDR_TO_STR(&info.hw_addr)));
47+
48+
/* check broadcast address */
49+
log_debug("broadcast address: %s", HW_ADDR_TO_STR(&info.broadcast));
50+
assert_se(sd_device_get_sysattr_value(dev, "broadcast", &s) >= 0);
51+
assert_se(streq(s, HW_ADDR_TO_STR(&info.broadcast)));
52+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "broadcast", &s) >= 0);
53+
assert_se(streq(s, HW_ADDR_TO_STR(&info.broadcast)));
54+
55+
/* check ifname */
56+
log_debug("ifname: %s", info.ifname);
57+
assert_se(sd_device_get_sysname(dev, &s) >= 0);
58+
assert_se(streq(s, info.ifname));
59+
assert_se(sd_device_get_sysname(dev_with_netlink, &s) >= 0);
60+
assert_se(streq(s, info.ifname));
61+
62+
/* check mtu */
63+
log_debug("mtu: %"PRIu32, info.mtu);
64+
assert_se(sd_device_get_sysattr_value(dev, "mtu", &s) >= 0);
65+
assert_se(safe_atou(s, &u) >= 0);
66+
assert_se(u == info.mtu);
67+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "mtu", &s) >= 0);
68+
assert_se(safe_atou(s, &u) >= 0);
69+
assert_se(u == info.mtu);
70+
71+
/* check iflink */
72+
log_debug("iflink: %"PRIu32, info.iflink);
73+
assert_se(sd_device_get_sysattr_value(dev, "iflink", &s) >= 0);
74+
assert_se(safe_atou(s, &u) >= 0);
75+
assert_se(u == info.iflink);
76+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "iflink", &s) >= 0);
77+
assert_se(safe_atou(s, &u) >= 0);
78+
assert_se(u == info.iflink);
79+
80+
/* check link_mode */
81+
log_debug("link_mode: %"PRIu8, info.link_mode);
82+
assert_se(sd_device_get_sysattr_value(dev, "link_mode", &s) >= 0);
83+
assert_se(safe_atou(s, &u) >= 0);
84+
assert_se(u == info.link_mode);
85+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "link_mode", &s) >= 0);
86+
assert_se(safe_atou(s, &u) >= 0);
87+
assert_se(u == info.link_mode);
88+
89+
/* check ifalias */
90+
log_debug("ifalias: %s", strna(info.ifalias));
91+
assert_se(sd_device_get_sysattr_value(dev, "ifalias", &s) >= 0);
92+
assert_se(streq(s, strempty(info.ifalias)));
93+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "ifalias", &s) >= 0);
94+
assert_se(streq(s, strempty(info.ifalias)));
95+
96+
/* check netdev_group */
97+
log_debug("netdev_group: %"PRIu32, info.group);
98+
assert_se(sd_device_get_sysattr_value(dev, "netdev_group", &s) >= 0);
99+
assert_se(safe_atou(s, &u) >= 0);
100+
assert_se(u == info.group);
101+
assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "netdev_group", &s) >= 0);
102+
assert_se(safe_atou(s, &u) >= 0);
103+
assert_se(u == info.group);
104+
105+
/* check phys_port_id */
106+
log_debug("phys_port_id: (%s)",
107+
info.phys_port_id_supported ? "supported" : "unsupported");
108+
s = t = NULL;
109+
(void) sd_device_get_sysattr_value(dev, "phys_port_id", &s);
110+
(void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_port_id", &t);
111+
assert_se(streq_ptr(s, t));
112+
113+
/* check phys_switch_id */
114+
log_debug("phys_switch_id: (%s)",
115+
info.phys_switch_id_supported ? "supported" : "unsupported");
116+
s = t = NULL;
117+
(void) sd_device_get_sysattr_value(dev, "phys_switch_id", &s);
118+
(void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_switch_id", &t);
119+
assert_se(streq_ptr(s, t));
120+
121+
/* check phys_port_name */
122+
log_debug("phys_port_name: %s (%s)",
123+
strna(info.phys_port_name),
124+
info.phys_port_name_supported ? "supported" : "unsupported");
125+
s = t = NULL;
126+
(void) sd_device_get_sysattr_value(dev, "phys_port_name", &s);
127+
(void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_port_name", &t);
128+
assert_se(streq_ptr(s, t));
129+
if (info.phys_port_name_supported) {
130+
assert_se(streq_ptr(s, info.phys_port_name));
131+
assert_se(streq_ptr(t, info.phys_port_name));
132+
}
133+
}
134+
135+
static void test_link_info_get(void) {
136+
_cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL;
137+
_cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL, *reply = NULL;
138+
139+
log_debug("/* %s */", __func__);
140+
141+
assert_se(sd_netlink_open(&rtnl) >= 0);
142+
143+
assert_se(sd_rtnl_message_new_link(rtnl, &req, RTM_GETLINK, 0) >= 0);
144+
assert_se(sd_netlink_message_request_dump(req, true) >= 0);
145+
assert_se(sd_netlink_call(rtnl, req, 0, &reply) >= 0);
146+
147+
for (sd_netlink_message *reply_one = reply; reply_one; reply_one = sd_netlink_message_next(reply_one)) {
148+
uint16_t nlmsg_type;
149+
int ifindex;
150+
151+
assert_se(sd_netlink_message_get_type(reply_one, &nlmsg_type) >= 0);
152+
assert_se(nlmsg_type == RTM_NEWLINK);
153+
assert_se(sd_rtnl_message_link_get_ifindex(reply_one, &ifindex) >= 0);
154+
155+
test_link_info_one(rtnl, ifindex);
156+
}
157+
}
158+
159+
int main(int argc, char *argv[]) {
160+
test_setup_logging(LOG_DEBUG);
161+
162+
test_link_info_get();
163+
164+
return 0;
165+
}

0 commit comments

Comments
 (0)