[ovs-dev,v4] netlink: ignore IFLA_WIRELESS events

From: Michal Kazior <michal@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.

Signed-off-by: Michal Kazior <michal@plume.com>
---

Notes:
    v4:
     - fixed comment-too-long checkpatch warnin [0day robot]
    
    v3:
     - dont change rtnetlink_parse() semantics, instead
       extend rtnetlink_change struct and update its
       consumers [Ilya]
     - adjusted commit log to reflect different approach
       [Ilya]
    
    v2:
     - fix bracing style [0day robot / checkpatch]

 lib/if-notifier.c  |  7 ++++++-
 lib/netdev-linux.c |  9 +++++++++
 lib/route-table.c  |  4 ++++
 lib/rtnetlink.c    | 18 ++++++++++++++++++
 lib/rtnetlink.h    |  3 +++
 5 files changed, 40 insertions(+), 1 deletion(-)

On 3/4/21 4:32 PM, Michal Kazior wrote:
> From: Michal Kazior <michal@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.
> 
> Signed-off-by: Michal Kazior <michal@plume.com>
> ---
> 
> Notes:
>     v4:
>      - fixed comment-too-long checkpatch warnin [0day robot]
>     
>     v3:
>      - dont change rtnetlink_parse() semantics, instead
>        extend rtnetlink_change struct and update its
>        consumers [Ilya]
>      - adjusted commit log to reflect different approach
>        [Ilya]
>     
>     v2:
>      - fix bracing style [0day robot / checkpatch]
> 
>  lib/if-notifier.c  |  7 ++++++-
>  lib/netdev-linux.c |  9 +++++++++
>  lib/route-table.c  |  4 ++++
>  lib/rtnetlink.c    | 18 ++++++++++++++++++
>  lib/rtnetlink.h    |  3 +++
>  5 files changed, 40 insertions(+), 1 deletion(-)

Hi.  Thanks for the new version!
And sorry again for slow reviews.

> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..388288f71 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -663,6 +663,10 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
>  {
>      struct linux_lag_member *lag;
>  
> +    if (change->irrelevant) {
> +        return;
> +    }
> +
>      if (change->sub && netdev_linux_kind_is_lag(change->sub)) {
>          lag = shash_find_data(&lag_shash, change->ifname);
>  
> @@ -887,6 +891,10 @@ netdev_linux_update(struct netdev_linux *dev, int nsid,
>                      const struct rtnetlink_change *change)
>      OVS_REQUIRES(dev->mutex)
>  {
> +    if (change->irrelevant) {
> +        return;
> +    }
> +
>      if (netdev_linux_netnsid_is_eq(dev, nsid)) {
>          netdev_linux_update__(dev, change);
>      }

It's unclear why we need to check inside these functions.
I mean, there is only one place where these functions called
and there is no any useful work done there beside calling them.
I think, it's better to just check right after receiving
the change in a same way as in netdev_linux_update_via_netlink().

Something like this:

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index e9ce41d10..ef90fc44c 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -663,10 +663,6 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
 {
     struct linux_lag_member *lag;
 
-    if (change->irrelevant) {
-        return;
-    }
-
     if (change->sub && netdev_linux_kind_is_lag(change->sub)) {
         lag = shash_find_data(&lag_shash, change->ifname);
 
@@ -746,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) && !change->irrelevant) {
                 struct netdev *netdev_ = NULL;
                 char dev_name[IFNAMSIZ];
 
@@ -891,10 +887,6 @@ netdev_linux_update(struct netdev_linux *dev, int nsid,
                     const struct rtnetlink_change *change)
     OVS_REQUIRES(dev->mutex)
 {
-    if (change->irrelevant) {
-        return;
-    }
-
     if (netdev_linux_netnsid_is_eq(dev, nsid)) {
         netdev_linux_update__(dev, change);
     }
---

What do you think?
If it looks good to you, I can squash above diff with your patch and
apply to master.

Best regards, Ilya Maximets.

On Thu, 1 Apr 2021 at 21:17, Ilya Maximets <i.maximets@ovn.org> wrote:
> On 3/4/21 4:32 PM, Michal Kazior wrote:
[...]
> It's unclear why we need to check inside these functions.
> I mean, there is only one place where these functions called
> and there is no any useful work done there beside calling them.
> I think, it's better to just check right after receiving
> the change in a same way as in netdev_linux_update_via_netlink().
>
> Something like this:
>
[...]
> -            if (rtnetlink_parse(&buf, &change)) {
> +            if (rtnetlink_parse(&buf, &change) && !change->irrelevant) {
[...]
>
> What do you think?
> If it looks good to you, I can squash above diff with your patch and
> apply to master.

No particular reason why I did it like that. But you're right. I don't
mind if you change it. Thanks!


Michał

On 3/4/21 4:32 PM, Michal Kazior wrote:
> From: Michal Kazior <michal@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.
> 
> Signed-off-by: Michal Kazior <michal@plume.com>
> ---
> 
> Notes:
>     v4:
>      - fixed comment-too-long checkpatch warnin [0day robot]
>     
>     v3:
>      - dont change rtnetlink_parse() semantics, instead
>        extend rtnetlink_change struct and update its
>        consumers [Ilya]
>      - adjusted commit log to reflect different approach
>        [Ilya]
>     
>     v2:
>      - fix bracing style [0day robot / checkpatch]
> 
>  lib/if-notifier.c  |  7 ++++++-
>  lib/netdev-linux.c |  9 +++++++++
>  lib/route-table.c  |  4 ++++
>  lib/rtnetlink.c    | 18 ++++++++++++++++++
>  lib/rtnetlink.h    |  3 +++
>  5 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/if-notifier.c b/lib/if-notifier.c
> index 9a64f9b15..7ba9cc316 100644
> --- a/lib/if-notifier.c
> +++ b/lib/if-notifier.c
> @@ -26,9 +26,14 @@ struct if_notifier {
>  };
>  
>  static void
> -if_notifier_cb(const struct rtnetlink_change *change OVS_UNUSED, void *aux)
> +if_notifier_cb(const struct rtnetlink_change *change, void *aux)
>  {
>      struct if_notifier *notifier;
> +
> +    if (change->irrelevant) {

Hi.
I tried to run system tests (make check-kernel) and OVS
crashes almost instantly here.

This one and other functions that are called on netlink
change, receives NULL as a 'change' fairly frequently,
so this check should be:

    if (change && change->irrelevant) {

> +        return;
> +    }
> +
>      notifier = aux;
>      notifier->cb(notifier->aux);
>  }
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..388288f71 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -663,6 +663,10 @@ netdev_linux_update_lag(struct rtnetlink_change *change)
>  {
>      struct linux_lag_member *lag;
>  
> +    if (change->irrelevant) {
> +        return;
> +    }
> +
>      if (change->sub && netdev_linux_kind_is_lag(change->sub)) {
>          lag = shash_find_data(&lag_shash, change->ifname);
>  
> @@ -887,6 +891,10 @@ netdev_linux_update(struct netdev_linux *dev, int nsid,
>                      const struct rtnetlink_change *change)
>      OVS_REQUIRES(dev->mutex)
>  {
> +    if (change->irrelevant) {
> +        return;
> +    }
> +
>      if (netdev_linux_netnsid_is_eq(dev, nsid)) {
>          netdev_linux_update__(dev, change);
>      }
> @@ -6344,6 +6352,7 @@ netdev_linux_update_via_netlink(struct netdev_linux *netdev)
>      }
>  
>      if (rtnetlink_parse(reply, change)
> +        && !change->irrelevant
>          && change->nlmsg_type == RTM_NEWLINK) {
>          bool changed = false;
>          error = 0;
> diff --git a/lib/route-table.c b/lib/route-table.c
> index 6c82cdfdd..a4531df1e 100644
> --- a/lib/route-table.c
> +++ b/lib/route-table.c
> @@ -342,6 +342,10 @@ static void
>  name_table_change(const struct rtnetlink_change *change,
>                    void *aux OVS_UNUSED)
>  {
> +    if (change->irrelevant) {

Same here.

> +        return;
> +    }
> +
>      /* Changes to interface status can cause routing table changes that some
>       * versions of the linux kernel do not advertise for some reason. */
>      route_table_valid = false;
> diff --git a/lib/rtnetlink.c b/lib/rtnetlink.c
> index 125802925..ebb032aa7 100644
> --- a/lib/rtnetlink.c
> +++ b/lib/rtnetlink.c
> @@ -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) {
> +                change->irrelevant = true;
> +            }

Caller are not initializing 'change' argument before passing it
to the parsing function.  So, to avoid using of uninitialized
memory, change->irrelevant should be initialized to 'false'
somewhere in the beginning of this function.

> +
>              change->nlmsg_type     = nlmsg->nlmsg_type;
>              change->if_index       = ifinfo->ifi_index;
>              change->ifname         = nl_attr_get_string(attrs[IFLA_IFNAME]);
> diff --git a/lib/rtnetlink.h b/lib/rtnetlink.h
> index b6ddb4bd1..4d11a6409 100644
> --- a/lib/rtnetlink.h
> +++ b/lib/rtnetlink.h
> @@ -45,6 +45,9 @@ struct rtnetlink_change {
>      int mtu;                    /* Current MTU. */
>      struct eth_addr mac;
>      unsigned int ifi_flags;     /* Flags of network device. */
> +    bool irrelevant;            /* Some events, notably wireless extensions,
> +                                   don't really indicate real netdev change
> +                                   that ovs should care about */

Coding style ask to use a period in the end of the comment.

>  
>      /* Network device address status. */
>      /* xxx To be added when needed. */
> 

Please, fix above issues, incorporate the suggestion about
netdev-linux part from the other email and send a new version.

Best regards, Ilya Maximets.