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

Ilya Maximets i.maximets at ovn.org
Thu Apr 1 19:17:43 UTC 2021


On 3/4/21 4:32 PM, 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.
> 
> Signed-off-by: Michal Kazior <michal at 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.


More information about the dev mailing list