On 3/4/21 4:32 PM, Michal Kazior wrote:
> From: Michal Kazior <[email protected]>
>
> 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 <[email protected]>
> ---
>
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
