Message ID | 20210304153254.1088-1-kazikcz@gmail.com |
---|---|
State | Changes Requested |
Headers | show |
Series | [ovs-dev,v4] netlink: ignore IFLA_WIRELESS events | expand |
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.
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) { + 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) { + 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; + } + 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 */ /* Network device address status. */ /* xxx To be added when needed. */