[ovs-dev] [PATCH v5] netlink: ignore IFLA_WIRELESS events
Ilya Maximets
i.maximets at ovn.org
Tue Apr 13 12:02:05 UTC 2021
On 4/13/21 12:09 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:
> v5:
> - added checks to avoid null deref of `change` [Ilya]
> - added missing period at end of comment [Ilya]
> - moved some checks closer to rtnetlink_parse() [Ilya]
> - made sure change->irrelevant is always initialized [Ilya]
>
> 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 | 3 ++-
> lib/route-table.c | 4 ++++
> lib/rtnetlink.c | 16 ++++++++++++++++
> lib/rtnetlink.h | 3 +++
> 5 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/lib/if-notifier.c b/lib/if-notifier.c
> index 9a64f9b15..f2d7157b9 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 && change->irrelevant) {
> + return;
> + }
> +
> notifier = aux;
> notifier->cb(notifier->aux);
> }
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 6be23dbee..6713c6d68 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) && !change.irrelevant) {
> struct netdev *netdev_ = NULL;
> char dev_name[IFNAMSIZ];
>
> @@ -6344,6 +6344,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..ac82cf262 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 && change->irrelevant) {
> + 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..7e68edde9 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,21 @@ 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.
> + */
> + change->irrelevant = (attrs[IFLA_WIRELESS] &&
> + ifinfo->ifi_change == 0);
This is not enough. This will initialize 'irrelevant' field only
for RTNLGRP_LINK messages. But netdev-linux listens other groups
too and checks the 'irrelevant' field unconditionally.
It's, probbaly, better to just initialize it at the top of the
function to 'false' and update here to 'true' if needed.
> 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..11f454ffc 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. */
>
> /* Network device address status. */
> /* xxx To be added when needed. */
>
More information about the dev
mailing list