From 5bbcfbaa11a92732f9bbc8d5f77e9311e6ac3d56 Mon Sep 17 00:00:00 2001 From: Yu Watanabe Date: Sun, 25 Sep 2022 13:18:24 +0900 Subject: [PATCH] udev: drop workaround for slow read of phys_port_name sysattr TL;DR This effectively reverts 8327fd1b11c5fb6529d46dfb40e2af981ffa8545, eaba9bb3e69635d2c490c5e1b0d262b763753e1d, and its follow-ups, as the original issue was already fixed by the kernel side. The original issue that the above commits tried to 'fix' is that reading phys_port_name triggers a lock in the kernel, hence processing multiple interfaces at the same time causes extreme slow down. To workaround the issue, the above commits made several necessary information retrieved through netlink instead of sysfs attributes. A patch set for the kernel was proposed as a fix for the issue: https://lore.kernel.org/all/20210928125500.167943-1-atenart@kernel.org/ and some of them were merged to v5.16: https://github.com/torvalds/linux/commit/146e5e733310379f51924111068f08a3af0db830, It has been already backported to 5.4.160, 5.10.80, 5.14.19, and 5.15.3. When these commits were proposed, it is already claimed that such issue should be fixed by the kernel side, and udevd should not workaround it. Neverthless the feature was introduced, as these have theoretical performance improvement, even if phys_port_name sysattr does not have the above issue, as in that way udevd can obtain multiple information about the interface with a single netlink socket operation. See the discussion in #20744. However, in reality, only `iflink`, `type`, `address`, and `phys_port_name` attributes from netlink are used in the udev net_id builtin command. Hence, after the original issue being fixed in the kernel side, there should be almost no performance improvement for udevd. Furthermore, combining attributes from netlink and sysfs makes hard to test net_id builtin. See #21725. Let's drop mostly meaningless code, and make net_id builtin easily testable. Closes #21725. --- src/udev/meson.build | 6 -- src/udev/test-udev-netlink.c | 157 --------------------------------- src/udev/udev-builtin-net_id.c | 75 +++++++++++----- src/udev/udev-event.c | 5 +- src/udev/udev-netlink.h | 41 --------- src/udev/udev-rules.c | 3 +- 6 files changed, 54 insertions(+), 233 deletions(-) delete mode 100644 src/udev/test-udev-netlink.c delete mode 100644 src/udev/udev-netlink.h diff --git a/src/udev/meson.build b/src/udev/meson.build index c6711beb5a2..08a1d97e817 100644 --- a/src/udev/meson.build +++ b/src/udev/meson.build @@ -39,8 +39,6 @@ libudevd_core_sources = files( 'udev-builtin-net_setup_link.c', 'udev-builtin-path_id.c', 'udev-builtin-usb_id.c', - 'udev-netlink.c', - 'udev-netlink.h', 'net/link-config.c', 'net/link-config.h', ) @@ -214,10 +212,6 @@ tests += [ [threads, libacl]], - [files('test-udev-netlink.c', - 'udev-netlink.c', - 'udev-netlink.h')], - [files('fido_id/test-fido-id-desc.c', 'fido_id/fido_id_desc.c')], ] diff --git a/src/udev/test-udev-netlink.c b/src/udev/test-udev-netlink.c deleted file mode 100644 index 341414c5ef3..00000000000 --- a/src/udev/test-udev-netlink.c +++ /dev/null @@ -1,157 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ - -#include "sd-device.h" - -#include "arphrd-util.h" -#include "ether-addr-util.h" -#include "parse-util.h" -#include "tests.h" -#include "udev-netlink.h" - -static void test_link_info_one(sd_netlink *rtnl, int ifindex) { - _cleanup_(link_info_clear) LinkInfo info = LINK_INFO_NULL; - _cleanup_(sd_device_unrefp) sd_device *dev = NULL, *dev_with_netlink = NULL; - const char *s, *t; - unsigned u; - - log_debug("/* %s(ifindex=%i) */", __func__, ifindex); - - assert_se(link_info_get(&rtnl, ifindex, &info) >= 0); - assert_se(sd_device_new_from_ifindex(&dev, ifindex) >= 0); - assert_se(sd_device_new_from_ifindex(&dev_with_netlink, ifindex) >= 0); - - /* check iftype */ - log_debug("iftype: %"PRIu16" (%s)", info.iftype, strna(arphrd_to_name(info.iftype))); - assert_se(sd_device_get_sysattr_value(dev, "type", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.iftype); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "type", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.iftype); - - /* check hardware address length */ - log_debug("hardware address length: %zu", info.hw_addr.length); - assert_se(sd_device_get_sysattr_value(dev, "addr_len", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.hw_addr.length); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "addr_len", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.hw_addr.length); - - /* check hardware address */ - log_debug("hardware address: %s", HW_ADDR_TO_STR(&info.hw_addr)); - assert_se(sd_device_get_sysattr_value(dev, "address", &s) >= 0); - assert_se(streq(s, HW_ADDR_TO_STR(&info.hw_addr))); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "address", &s) >= 0); - assert_se(streq(s, HW_ADDR_TO_STR(&info.hw_addr))); - - /* check broadcast address */ - log_debug("broadcast address: %s", HW_ADDR_TO_STR(&info.broadcast)); - assert_se(sd_device_get_sysattr_value(dev, "broadcast", &s) >= 0); - assert_se(streq(s, HW_ADDR_TO_STR(&info.broadcast))); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "broadcast", &s) >= 0); - assert_se(streq(s, HW_ADDR_TO_STR(&info.broadcast))); - - /* check ifname */ - log_debug("ifname: %s", info.ifname); - assert_se(sd_device_get_sysname(dev, &s) >= 0); - assert_se(streq(s, info.ifname)); - assert_se(sd_device_get_sysname(dev_with_netlink, &s) >= 0); - assert_se(streq(s, info.ifname)); - - /* check mtu */ - log_debug("mtu: %"PRIu32, info.mtu); - assert_se(sd_device_get_sysattr_value(dev, "mtu", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.mtu); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "mtu", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.mtu); - - /* check iflink */ - log_debug("iflink: %"PRIu32, info.iflink); - assert_se(sd_device_get_sysattr_value(dev, "iflink", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.iflink); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "iflink", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.iflink); - - /* check link_mode */ - log_debug("link_mode: %"PRIu8, info.link_mode); - assert_se(sd_device_get_sysattr_value(dev, "link_mode", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.link_mode); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "link_mode", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.link_mode); - - /* check ifalias */ - log_debug("ifalias: %s", strna(info.ifalias)); - assert_se(sd_device_get_sysattr_value(dev, "ifalias", &s) >= 0); - assert_se(streq(s, strempty(info.ifalias))); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "ifalias", &s) >= 0); - assert_se(streq(s, strempty(info.ifalias))); - - /* check netdev_group */ - log_debug("netdev_group: %"PRIu32, info.group); - assert_se(sd_device_get_sysattr_value(dev, "netdev_group", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.group); - assert_se(device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "netdev_group", &s) >= 0); - assert_se(safe_atou(s, &u) >= 0); - assert_se(u == info.group); - - /* check phys_port_id */ - log_debug("phys_port_id: (%s)", - info.phys_port_id_supported ? "supported" : "unsupported"); - s = t = NULL; - (void) sd_device_get_sysattr_value(dev, "phys_port_id", &s); - (void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_port_id", &t); - assert_se(streq_ptr(s, t)); - - /* check phys_switch_id */ - log_debug("phys_switch_id: (%s)", - info.phys_switch_id_supported ? "supported" : "unsupported"); - s = t = NULL; - (void) sd_device_get_sysattr_value(dev, "phys_switch_id", &s); - (void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_switch_id", &t); - assert_se(streq_ptr(s, t)); - - /* check phys_port_name */ - log_debug("phys_port_name: %s (%s)", - strna(info.phys_port_name), - info.phys_port_name_supported ? "supported" : "unsupported"); - s = t = NULL; - (void) sd_device_get_sysattr_value(dev, "phys_port_name", &s); - (void) device_get_sysattr_value_maybe_from_netlink(dev_with_netlink, &rtnl, "phys_port_name", &t); - assert_se(streq_ptr(s, t)); - if (info.phys_port_name_supported) { - assert_se(streq_ptr(s, info.phys_port_name)); - assert_se(streq_ptr(t, info.phys_port_name)); - } -} - -TEST(link_info_get) { - _cleanup_(sd_netlink_unrefp) sd_netlink *rtnl = NULL; - _cleanup_(sd_netlink_message_unrefp) sd_netlink_message *req = NULL, *reply = NULL; - - assert_se(sd_netlink_open(&rtnl) >= 0); - - assert_se(sd_rtnl_message_new_link(rtnl, &req, RTM_GETLINK, 0) >= 0); - assert_se(sd_netlink_message_set_request_dump(req, true) >= 0); - assert_se(sd_netlink_call(rtnl, req, 0, &reply) >= 0); - - for (sd_netlink_message *reply_one = reply; reply_one; reply_one = sd_netlink_message_next(reply_one)) { - uint16_t nlmsg_type; - int ifindex; - - assert_se(sd_netlink_message_get_type(reply_one, &nlmsg_type) >= 0); - assert_se(nlmsg_type == RTM_NEWLINK); - assert_se(sd_rtnl_message_link_get_ifindex(reply_one, &ifindex) >= 0); - - test_link_info_one(rtnl, ifindex); - } -} - -DEFINE_TEST_MAIN(LOG_DEBUG); diff --git a/src/udev/udev-builtin-net_id.c b/src/udev/udev-builtin-net_id.c index 4acbc87b967..bdadd8bbfb6 100644 --- a/src/udev/udev-builtin-net_id.c +++ b/src/udev/udev-builtin-net_id.c @@ -27,6 +27,7 @@ #include "device-private.h" #include "device-util.h" #include "dirent-util.h" +#include "ether-addr-util.h" #include "fd-util.h" #include "fileio.h" #include "glyph-util.h" @@ -38,7 +39,6 @@ #include "strv.h" #include "strxcpyx.h" #include "udev-builtin.h" -#include "udev-netlink.h" #define ONBOARD_14BIT_INDEX_MAX ((1U << 14) - 1) #define ONBOARD_16BIT_INDEX_MAX ((1U << 16) - 1) @@ -76,6 +76,15 @@ typedef struct NetNames { char devicetree_onboard[ALTIFNAMSIZ]; } NetNames; +typedef struct LinkInfo { + int ifindex; + int iflink; + int iftype; + const char *devtype; + const char *phys_port_name; + struct hw_addr_data hw_addr; +} LinkInfo; + /* skip intermediate virtio devices */ static sd_device *skip_virtio(sd_device *dev) { /* there can only ever be one virtio bus per parent device, so we can @@ -1057,37 +1066,57 @@ static int ieee_oui(sd_device *dev, const LinkInfo *info, bool test) { return udev_builtin_hwdb_lookup(dev, NULL, str, NULL, test); } -static int builtin_net_id(sd_device *dev, sd_netlink **rtnl, int argc, char *argv[], bool test) { - _cleanup_(link_info_clear) LinkInfo info = LINK_INFO_NULL; - const char *devtype, *prefix = "en"; - NetNames names = {}; - int ifindex, r; +static int get_link_info(sd_device *dev, LinkInfo *info) { + const char *s; + int r; - r = sd_device_get_ifindex(dev, &ifindex); + assert(dev); + assert(info); + + r = sd_device_get_ifindex(dev, &info->ifindex); + if (r < 0) + return r; + + r = device_get_sysattr_int(dev, "iflink", &info->iflink); if (r < 0) return r; - r = link_info_get(rtnl, ifindex, &info); + r = device_get_sysattr_int(dev, "type", &info->iftype); if (r < 0) return r; - if (!info.phys_port_name_supported) { - const char *s; + r = sd_device_get_devtype(dev, &info->devtype); + if (r < 0 && r != -ENOENT) + return r; - r = sd_device_get_sysattr_value(dev, "phys_port_name", &s); - if (r >= 0) { - info.phys_port_name = strdup(s); - if (!info.phys_port_name) - return log_oom(); - } + r = sd_device_get_sysattr_value(dev, "phys_port_name", &info->phys_port_name); + if (r < 0 && r != -ENOENT) + return r; + + r = sd_device_get_sysattr_value(dev, "address", &s); + if (r < 0 && r != -ENOENT) + return r; + if (r > 0) { + r = parse_hw_addr(s, &info->hw_addr); + if (r < 0) + log_device_debug_errno(dev, r, "Failed to parse 'address' sysattr, ignoring: %m"); } - r = device_cache_sysattr_from_link_info(dev, &info); + return 0; +} + +static int builtin_net_id(sd_device *dev, sd_netlink **rtnl, int argc, char *argv[], bool test) { + const char *prefix; + NetNames names = {}; + LinkInfo info = {}; + int r; + + r = get_link_info(dev, &info); if (r < 0) return r; /* skip stacked devices, like VLANs, ... */ - if (info.ifindex != (int) info.iflink) + if (info.ifindex != info.iflink) return 0; /* handle only ARPHRD_ETHER, ARPHRD_SLIP and ARPHRD_INFINIBAND devices */ @@ -1108,12 +1137,10 @@ static int builtin_net_id(sd_device *dev, sd_netlink **rtnl, int argc, char *arg return 0; } - if (sd_device_get_devtype(dev, &devtype) >= 0) { - if (streq("wlan", devtype)) - prefix = "wl"; - else if (streq("wwan", devtype)) - prefix = "ww"; - } + if (streq_ptr("wlan", info.devtype)) + prefix = "wl"; + else if (streq_ptr("wwan", info.devtype)) + prefix = "ww"; udev_builtin_add_property(dev, test, "ID_NET_NAMING_SCHEME", naming_scheme()->name); diff --git a/src/udev/udev-event.c b/src/udev/udev-event.c index d4622b2f9d8..b3d92d51505 100644 --- a/src/udev/udev-event.c +++ b/src/udev/udev-event.c @@ -30,7 +30,6 @@ #include "strxcpyx.h" #include "udev-builtin.h" #include "udev-event.h" -#include "udev-netlink.h" #include "udev-node.h" #include "udev-util.h" #include "udev-watch.h" @@ -357,11 +356,11 @@ static ssize_t udev_event_subst_format( /* try to read the attribute the device */ if (!val) - (void) device_get_sysattr_value_maybe_from_netlink(dev, &event->rtnl, attr, &val); + (void) sd_device_get_sysattr_value(dev, attr, &val); /* try to read the attribute of the parent device, other matches have selected */ if (!val && event->dev_parent && event->dev_parent != dev) - (void) device_get_sysattr_value_maybe_from_netlink(event->dev_parent, &event->rtnl, attr, &val); + (void) sd_device_get_sysattr_value(event->dev_parent, attr, &val); if (!val) goto null_terminate; diff --git a/src/udev/udev-netlink.h b/src/udev/udev-netlink.h deleted file mode 100644 index a82352dd61e..00000000000 --- a/src/udev/udev-netlink.h +++ /dev/null @@ -1,41 +0,0 @@ -/* SPDX-License-Identifier: LGPL-2.1-or-later */ -#pragma once - -#include "sd-device.h" -#include "sd-netlink.h" - -#include "ether-addr-util.h" - -typedef struct LinkInfo { - int ifindex; - uint16_t iftype; /* ARPHRD_* (type) */ - - struct hw_addr_data hw_addr; /* IFLA_ADDRESS (address, addr_len) */ - struct hw_addr_data broadcast; /* IFLA_BROADCAST (broadcast) */ - char *ifname; /* IFLA_IFNAME */ - uint32_t mtu; /* IFLA_MTU (mtu) */ - uint32_t iflink; /* IFLA_LINK (iflink) */ - uint8_t link_mode; /* IFLA_LINKMODE (link_mode) */ - char *ifalias; /* IFLA_IFALIAS (ifalias) */ - uint32_t group; /* IFLA_GROUP (netdev_group) */ - uint8_t *phys_port_id; /* IFLA_PHYS_PORT_ID (phys_port_id) */ - size_t phys_port_id_len; - uint8_t *phys_switch_id; /* IFLA_PHYS_SWITCH_ID (phys_switch_id) */ - size_t phys_switch_id_len; - char *phys_port_name; /* IFLA_PHYS_PORT_NAME (phys_port_name) */ - - bool phys_port_id_supported; - bool phys_switch_id_supported; - bool phys_port_name_supported; -} LinkInfo; - -#define LINK_INFO_NULL ((LinkInfo) {}) - -void link_info_clear(LinkInfo *info); -int link_info_get(sd_netlink **rtnl, int ifindex, LinkInfo *ret); -int device_cache_sysattr_from_link_info(sd_device *device, LinkInfo *info); -int device_get_sysattr_value_maybe_from_netlink( - sd_device *device, - sd_netlink **rtnl, - const char *sysattr, - const char **ret_value); diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index eaa3578489b..f44a174d1f3 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -29,7 +29,6 @@ #include "syslog-util.h" #include "udev-builtin.h" #include "udev-event.h" -#include "udev-netlink.h" #include "udev-node.h" #include "udev-rules.h" #include "udev-util.h" @@ -1419,7 +1418,7 @@ static bool token_match_attr(UdevRules *rules, UdevRuleToken *token, sd_device * name = nbuf; _fallthrough_; case SUBST_TYPE_PLAIN: - if (device_get_sysattr_value_maybe_from_netlink(dev, &event->rtnl, name, &value) < 0) + if (sd_device_get_sysattr_value(dev, name, &value) < 0) return false; break; case SUBST_TYPE_SUBSYS: -- 2.47.3