Re: [ovs-dev] [PATCH v2] netlink: ignore IFLA_WIRELESS events

[Gregory Rose]

On 1/14/2021 1:09 AM, Michal Kazior wrote:

From: Michal Kazior <mic...@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 <mic...@plume.com>

Hi Michal,

The patch looks fine to me.  Applies cleanly to master
and did not cause any regressions in 'make check'.

I'm curious since I don't ever use OVS with wireless ports
if there is somewhere this issue has been reported.  If
so then we should add it to the commit message.

Otherwise,

Acked-by: Greg Rose <gvrose8...@gmail.com>

---

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. */
-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 *);

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev