[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