| Message ID | 1491312333-18904-1-git-send-email-vyasevic@redhat.com |
|---|---|
| State | Awaiting Upstream, archived |
| Delegated to: | stephen hemminger |
| Headers | show |
On 4/4/17 9:25 AM, Vladislav Yasevich wrote: > Add IFLA_EVENT handling so that event types can be viewed with > 'monitor' command. This gives a little more information for why > a given message was receivied. > > Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> > --- > include/linux/if_link.h | 21 +++++++++++++++++++++ > ip/ipaddress.c | 31 +++++++++++++++++++++++++++++++ > 2 files changed, 52 insertions(+) per comments on the email thread about reducing notifications, the kernel patch for this should be reverted (and hence this iproute2 patch is not needed) in favor of using a bitmask. Right now there are too many redundant notifications to userspace. Vlad: can you look at a bitmask version of IFLA_EVENT that shows what changed in the newlink message from do_setlink()?
From: David Ahern <dsa@cumulusnetworks.com> Date: Sat, 8 Apr 2017 18:24:06 -0400 > per comments on the email thread about reducing notifications, the > kernel patch for this should be reverted (and hence this iproute2 patch > is not needed) in favor of using a bitmask. Right now there are too many > redundant notifications to userspace. I must have missed something in all the discussion, which patch needs to be reverted from my tree exactly?
On 4/8/17 10:33 PM, David Miller wrote: > From: David Ahern <dsa@cumulusnetworks.com> > Date: Sat, 8 Apr 2017 18:24:06 -0400 > >> per comments on the email thread about reducing notifications, the >> kernel patch for this should be reverted (and hence this iproute2 patch >> is not needed) in favor of using a bitmask. Right now there are too many >> redundant notifications to userspace. > > I must have missed something in all the discussion, which patch needs > to be reverted from my tree exactly? > Here's the thread: https://www.spinics.net/lists/netdev/msg429154.html The comment is that def12888c161 is adding a uapi that leads to way too many notifications (e.g., on a setlink). It would be more efficient (read less notifications) to have do_setlink emit a single message with the IFLA_EVENT (or something else appropriately named) that indicates what attributes changed. Right now, a change MTU leads to 3 notifications causing unnecessary churn in userspace to track what the state of the link is.
From: David Ahern <dsa@cumulusnetworks.com> Date: Sat, 8 Apr 2017 22:54:07 -0400 > On 4/8/17 10:33 PM, David Miller wrote: >> From: David Ahern <dsa@cumulusnetworks.com> >> Date: Sat, 8 Apr 2017 18:24:06 -0400 >> >>> per comments on the email thread about reducing notifications, the >>> kernel patch for this should be reverted (and hence this iproute2 patch >>> is not needed) in favor of using a bitmask. Right now there are too many >>> redundant notifications to userspace. >> >> I must have missed something in all the discussion, which patch needs >> to be reverted from my tree exactly? >> > > Here's the thread: > https://www.spinics.net/lists/netdev/msg429154.html > > The comment is that def12888c161 is adding a uapi that leads to way too > many notifications (e.g., on a setlink). > > It would be more efficient (read less notifications) to have do_setlink > emit a single message with the IFLA_EVENT (or something else > appropriately named) that indicates what attributes changed. Right now, > a change MTU leads to 3 notifications causing unnecessary churn in > userspace to track what the state of the link is. Ok, I'll queue up the revert. I guess this means your rtnetlink patch set is going to need changes or a respin, therefore.
diff --git a/include/linux/if_link.h b/include/linux/if_link.h index b0bdbd6..b6d211a 100644 --- a/include/linux/if_link.h +++ b/include/linux/if_link.h @@ -157,6 +157,7 @@ enum { IFLA_GSO_MAX_SIZE, IFLA_PAD, IFLA_XDP, + IFLA_EVENT, __IFLA_MAX }; @@ -890,4 +891,24 @@ enum { #define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1) +enum { + IFLA_EVENT_UNSPEC, + IFLA_EVENT_REBOOT, + IFLA_EVENT_CHANGE_MTU, + IFLA_EVENT_CHANGE_ADDR, + IFLA_EVENT_CHANGE_NAME, + IFLA_EVENT_FEAT_CHANGE, + IFLA_EVENT_BONDING_FAILOVER, + IFLA_EVENT_POST_TYPE_CHANGE, + IFLA_EVENT_NOTIFY_PEERS, + IFLA_EVENT_CHANGE_UPPER, + IFLA_EVENT_RESEND_IGMP, + IFLA_EVENT_PRE_CHANGE_MTU, + IFLA_EVENT_CHANGE_INFO_DATA, + IFLA_EVENT_PRE_CHANGE_UPPER, + IFLA_EVENT_CHANGE_LOWER_STATE, + IFLA_EVENT_UDP_TUNNEL_PUSH_INFO, + IFLA_EVENT_CHANGE_TX_QUEUE_LEN, +}; + #endif /* _LINUX_IF_LINK_H */ diff --git a/ip/ipaddress.c b/ip/ipaddress.c index b8d9c7d..ffcfa5e 100644 --- a/ip/ipaddress.c +++ b/ip/ipaddress.c @@ -753,6 +753,34 @@ int print_linkinfo_brief(const struct sockaddr_nl *who, return 0; } +static const char *netdev_events[] = {"UNKNOWN", + "REBOOT", + "CHANGE_MTU", + "CHANGE_ADDR", + "CHANGE_NAME", + "FEATURE_CHANGE", + "BONDING_FAILOVER", + "POST_TYPE_CHANGE", + "NOTIFY_PEERS", + "CHANGE_UPPER", + "RESEND_IGMP", + "PRE_CHANGE_MTU", + "CHANGE_INFO_DATA", + "PRE_CHANGE_UPPER", + "CHANGE_LOWER_STATE", + "UDP_TUNNEL_PUSH_INFO", + "CHANGE_TXQUEUE_LEN"}; + +static void print_dev_event(FILE *f, __u32 event) +{ + if (event >= ARRAY_SIZE(netdev_events)) + fprintf(f, "event %d ", event); + else { + if (event) + fprintf(f, "event %s ", netdev_events[event]); + } +} + int print_linkinfo(const struct sockaddr_nl *who, struct nlmsghdr *n, void *arg) { @@ -858,6 +886,9 @@ int print_linkinfo(const struct sockaddr_nl *who, if (filter.showqueue) print_queuelen(fp, tb); + if (tb[IFLA_EVENT]) + print_dev_event(fp, rta_getattr_u32(tb[IFLA_EVENT])); + if (!filter.family || filter.family == AF_PACKET || show_details) { SPRINT_BUF(b1); fprintf(fp, "%s", _SL_);
Add IFLA_EVENT handling so that event types can be viewed with 'monitor' command. This gives a little more information for why a given message was receivied. Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com> --- include/linux/if_link.h | 21 +++++++++++++++++++++ ip/ipaddress.c | 31 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+)