[ovs-dev] [PATCH v2] netlink: ignore IFLA_WIRELESS events
Ilya Maximets
i.maximets at ovn.org
Mon Mar 1 19:40:23 UTC 2021
On 1/14/21 10:09 AM, Michal Kazior wrote:
> From: Michal Kazior <michal at plume.com>
>
> Some older wireless drivers - ones relying on the
> old and long deprecated wireless extension ioctl
> system - can generate quite a bit of IFLA_WIRELESS
> events depending on their configuration and
> runtime conditions. These are delivered as
> RTNLGRP_LINK via RTM_NEWLINK messages.
>
> These tend to be relatively easily identifiable
> because they report the change mask being 0. This
> isn't guaranteed but in practice it shouldn't be a
> problem. None of the wireless events that I ever
> observed actually carry any unique information
> about netdev states that ovs-vswitchd is
> interested in. Hence ignoring these shouldn't
> cause any problems.
>
> These events can be responsible for a significant
> CPU churn as ovs-vswitchd attempts to do plenty of
> work for each and every netlink message regardless
> of what that message carries. On low-end devices
> such as consumer-grade routers these can lead to a
> lot of CPU cycles being wasted, adding up to heat
> output and reducing performance.
>
> It could be argued that wireless drivers in
> question should be fixed, but that isn't exactly a
> trivial thing to do. Patching ovs seems far more
> viable while still making sense.
>
> This change requires a slight tweak to the netlink
> abstraction code so that the events can be
> discarded as soon as possible before getting a
> chance to cause the churn.
>
> Signed-off-by: Michal Kazior <michal at plume.com>
Hi, Michal. Thanks for working on this!
The idea seems reasonable to me. Some comments for the implementation
inline.
Bets regards, Ilya Maximets.
> ---
>
> Notes:
> v2:
> - fix bracing style [0day robot / checkpatch]
>
> lib/netdev-linux.c | 4 ++--
> lib/netlink-notifier.c | 4 +++-
> lib/rtnetlink.c | 24 +++++++++++++++++++++---
> lib/rtnetlink.h | 2 +-
> 4 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..301dd9060 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -742,7 +742,7 @@ netdev_linux_run(const struct netdev_class *netdev_class OVS_UNUSED)
> if (!error) {
> struct rtnetlink_change change;
>
> - if (rtnetlink_parse(&buf, &change)) {
> + if (rtnetlink_parse(&buf, &change) > 0) {
> struct netdev *netdev_ = NULL;
> char dev_name[IFNAMSIZ];
>
> @@ -6343,7 +6343,7 @@ netdev_linux_update_via_netlink(struct netdev_linux *netdev)
> return error;
> }
>
> - if (rtnetlink_parse(reply, change)
> + if (rtnetlink_parse(reply, change) > 0
> && change->nlmsg_type == RTM_NEWLINK) {
> bool changed = false;
> error = 0;
> diff --git a/lib/netlink-notifier.c b/lib/netlink-notifier.c
> index dfecb9778..ab5a84abb 100644
> --- a/lib/netlink-notifier.c
> +++ b/lib/netlink-notifier.c
> @@ -189,8 +189,10 @@ nln_run(struct nln *nln)
> if (!error) {
> int group = nln->parse(&buf, nln->change);
>
> - if (group != 0) {
> + if (group > 0) {
> nln_report(nln, nln->change, group);
> + } else if (group == 0) {
> + /* ignore some events */
> } else {
> VLOG_WARN_RL(&rl, "unexpected netlink message contents");
> nln_report(nln, NULL, 0);
> diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> index 125802925..316524c0f 100644
> --- a/lib/rtnetlink.c
> +++ b/lib/rtnetlink.c
> @@ -82,7 +82,7 @@ rtnetlink_parse_link_info(const struct nlattr *nla,
> /* Parses a rtnetlink message 'buf' into 'change'. If 'buf' is unparseable,
> * leaves 'change' untouched and returns false. Otherwise, populates 'change'
> * and returns true. */
Comment above is no longer correct with this change.
In general, changing the semantics of a 'parse' method looks
a bit tricky. Maybe we can do this a bit differently?
I'd suggest to keep the 'rtnetlink_parse' almost as is, but
add some extra flag to 'struct rtnetlink_change', that will
be set for change events that are not interesting for OVS.
Something like 'bool irrelevant;' 'rtnetlink_parse' will set
it to 'true' for wireless events with a fair comment that
"wireless events never really change interface state".
So, 'nln_run' will only report events if this flag is not
set and netdev-linux will check for:
if (rtnetlink_parse(&buf, &change) && !change->irrelevant)
What do you think?
> -bool
> +int
> rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
> {
> const struct nlmsghdr *nlmsg = buf->data;
> @@ -99,6 +99,7 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
> [IFLA_MTU] = { .type = NL_A_U32, .optional = true },
> [IFLA_ADDRESS] = { .type = NL_A_UNSPEC, .optional = true },
> [IFLA_LINKINFO] = { .type = NL_A_NESTED, .optional = true },
> + [IFLA_WIRELESS] = { .type = NL_A_UNSPEC, .optional = true },
> };
>
> struct nlattr *attrs[ARRAY_SIZE(policy)];
> @@ -111,6 +112,23 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
>
> ifinfo = ofpbuf_at(buf, NLMSG_HDRLEN, sizeof *ifinfo);
>
> + /* Wireless events can be spammy and cause a
> + * lot of unnecessary churn and CPU load in
> + * ovs-vswitchd. The best way to filter them out
> + * is to rely on the IFLA_WIRELESS and
> + * ifi_change. As per rtnetlink_ifinfo_prep() in
> + * the kernel, the ifi_change = 0. That combined
> + * with the fact wireless events never really
> + * change interface state (as far as core
> + * networking is concerned) they can be ignored
> + * by ovs-vswitchd. It doesn't understand
> + * wireless extensions anyway and has no way of
> + * presenting these bits into ovsdb.
> + */
> + if (attrs[IFLA_WIRELESS] && ifinfo->ifi_change == 0) {
> + return RTNLGRP_NONE;
> + }
> +
> change->nlmsg_type = nlmsg->nlmsg_type;
> change->if_index = ifinfo->ifi_index;
> change->ifname = nl_attr_get_string(attrs[IFLA_IFNAME]);
> @@ -165,14 +183,14 @@ rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change)
> }
> }
>
> - return parsed;
> + return parsed ? RTNLGRP_LINK : -1;
> }
>
> /* Return RTNLGRP_LINK on success, 0 on parse error. */
> static int
> rtnetlink_parse_cb(struct ofpbuf *buf, void *change)
> {
> - return rtnetlink_parse(buf, change) ? RTNLGRP_LINK : 0;
> + return rtnetlink_parse(buf, change);
> }
>
> /* Registers 'cb' to be called with auxiliary data 'aux' with network device
> diff --git a/lib/rtnetlink.h b/lib/rtnetlink.h
> index b6ddb4bd1..23921a63b 100644
> --- a/lib/rtnetlink.h
> +++ b/lib/rtnetlink.h
> @@ -65,7 +65,7 @@ void rtnetlink_notify_func(const struct rtnetlink_change *change,
>
> bool rtnetlink_type_is_rtnlgrp_link(uint16_t type);
> bool rtnetlink_type_is_rtnlgrp_addr(uint16_t type);
> -bool rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
> +int rtnetlink_parse(struct ofpbuf *buf, struct rtnetlink_change *change);
> struct nln_notifier *
> rtnetlink_notifier_create(rtnetlink_notify_func *, void *aux);
> void rtnetlink_notifier_destroy(struct nln_notifier *);
>
More information about the dev
mailing list