| Message ID | 20210114090938.6996-1-kazikcz@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] netlink: ignore IFLA_WIRELESS events | expand |
On 1/14/2021 1:09 AM, 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. > > 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 <michal@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 <gvrose8192@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 *); >
On Tue, 9 Feb 2021 at 00:58, Gregory Rose <gvrose8192@gmail.com> wrote: > On 1/14/2021 1:09 AM, Michal Kazior wrote: > > From: Michal Kazior <michal@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. Hi Greg, I'm unaware of any reports of this sort and I wouldn't be surprised to be the first one. Given my experience - and the fact this has been sitting on the mailing list for almost 2 months with no comments now - I imagine not many people tried mixing OVS with wext drivers, especially vendor drivers. Michał
On 1/14/21 10:09 AM, 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. > > 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 <michal@plume.com> Hi, Michal. Thanks for working on this! The idea seems reasonable to me. Some comments for the implementation inline. Bets regards, Ilya Maximets. > --- > > 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. */ Comment above is no longer correct with this change. In general, changing the semantics of a 'parse' method looks a bit tricky. Maybe we can do this a bit differently? I'd suggest to keep the 'rtnetlink_parse' almost as is, but add some extra flag to 'struct rtnetlink_change', that will be set for change events that are not interesting for OVS. Something like 'bool irrelevant;' 'rtnetlink_parse' will set it to 'true' for wireless events with a fair comment that "wireless events never really change interface state". So, 'nln_run' will only report events if this flag is not set and netdev-linux will check for: if (rtnetlink_parse(&buf, &change) && !change->irrelevant) What do you think? > -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 *); >
On Mon, Mar 1, 2021 at 8:40 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 1/14/21 10:09 AM, Michal Kazior wrote: > > From: Michal Kazior <michal@plume.com> [...] > Hi, Michal. Thanks for working on this! > The idea seems reasonable to me. Some comments for the implementation > inline. [...] > > 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. */ > > Comment above is no longer correct with this change. > In general, changing the semantics of a 'parse' method looks > a bit tricky. Maybe we can do this a bit differently? > I'd suggest to keep the 'rtnetlink_parse' almost as is, but > add some extra flag to 'struct rtnetlink_change', that will > be set for change events that are not interesting for OVS. > > Something like 'bool irrelevant;' 'rtnetlink_parse' will set > it to 'true' for wireless events with a fair comment that > "wireless events never really change interface state". > So, 'nln_run' will only report events if this flag is not > set and netdev-linux will check for: > if (rtnetlink_parse(&buf, &change) && !change->irrelevant) > > What do you think? It does sound reasonable. I think I did consider that back when I was cooking this but apparently I discarded the idea for some reason that I no longer remember. I'll give it a try. Michał
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 *);