| Message ID | 1491600340-8359-2-git-send-email-dsa@cumulusnetworks.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On 4/7/17, 2:25 PM, David Ahern wrote: > Changing MTU on a link currently causes 3 messages to be sent to userspace: > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. > > This change is good... multiple notifications for the same event does not help in large scale links setups. However, this reverts what vlad was trying to do with his patchset. Vlad's patch-set relies on the rtnl notifications generated from notifiers (rtnetlink_event) to add specific event (IFLA_EVENT) in notifications. The third notification in your example above is the correct one and is an aggregate notification for a set of changes, but it cannot really fill in all types of events in the single IFLA_EVENT attribute as it stands today. IFLA_EVENT should be a bitmask to include all events in this case (i had indicated this in vlads first version).
On 4/8/17 2:06 PM, Roopa Prabhu wrote: > On 4/7/17, 2:25 PM, David Ahern wrote: >> Changing MTU on a link currently causes 3 messages to be sent to userspace: >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. >> >> > > This change is good... multiple notifications for the same event does not help in large scale links setups. However, this > reverts what vlad was trying to do with his patchset. Vlad's patch-set relies on the rtnl notifications generated from > notifiers (rtnetlink_event) to add specific event (IFLA_EVENT) in notifications. > > The third notification in your example above is the correct one and is an aggregate notification for a set of changes, but > it cannot really fill in all types of events in the single IFLA_EVENT attribute as it stands today. IFLA_EVENT should be > a bitmask to include all events in this case (i had indicated this in vlads first version). > Agreed. I think it would be best to revert def12888c161 before the UAPI goes out. The change can instead add the IFLA_EVENT as a bitmask mentioned here to note the changes in a setlink. On top of that, remove the notifications for the events I mentioned in this set to reduce the overhead on userspace.
On 4/8/17, 11:13 AM, David Ahern wrote: > On 4/8/17 2:06 PM, Roopa Prabhu wrote: >> On 4/7/17, 2:25 PM, David Ahern wrote: >>> Changing MTU on a link currently causes 3 messages to be sent to userspace: >>> >>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU >>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>> >>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU >>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>> >>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default >>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>> >>> Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. >>> >>> >> This change is good... multiple notifications for the same event does not help in large scale links setups. However, this >> reverts what vlad was trying to do with his patchset. Vlad's patch-set relies on the rtnl notifications generated from >> notifiers (rtnetlink_event) to add specific event (IFLA_EVENT) in notifications. >> >> The third notification in your example above is the correct one and is an aggregate notification for a set of changes, but >> it cannot really fill in all types of events in the single IFLA_EVENT attribute as it stands today. IFLA_EVENT should be >> a bitmask to include all events in this case (i had indicated this in vlads first version). >> > Agreed. I think it would be best to revert def12888c161 before the UAPI > goes out. > > The change can instead add the IFLA_EVENT as a bitmask mentioned here to > note the changes in a setlink. On top of that, remove the notifications > for the events I mentioned in this set to reduce the overhead on userspace. ack
On 04/07/2017 05:25 PM, David Ahern wrote: > Changing MTU on a link currently causes 3 messages to be sent to userspace: > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default > link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff > > Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. Actually, I have plans for the CHANGE_MTU event. The last message is not an event. If you remove the event, it is much harder to track mtu changes. -vlad > > Signed-off-by: David Ahern <dsa@cumulusnetworks.com> > --- > include/uapi/linux/if_link.h | 2 -- > net/core/rtnetlink.c | 8 ------- > 2 files changed, 10 deletions(-) > > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h > index 97f6d302f627..e8b7e9342cc0 100644 > --- a/include/uapi/linux/if_link.h > +++ b/include/uapi/linux/if_link.h > @@ -903,7 +903,6 @@ enum { > enum { > IFLA_EVENT_UNSPEC, > IFLA_EVENT_REBOOT, > - IFLA_EVENT_CHANGE_MTU, > IFLA_EVENT_CHANGE_ADDR, > IFLA_EVENT_CHANGE_NAME, > IFLA_EVENT_FEAT_CHANGE, > @@ -912,7 +911,6 @@ enum { > 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, > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index b2bd4c9ee860..72d365ae14b3 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1285,9 +1285,6 @@ static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event) > case NETDEV_REBOOT: > rtnl_event = IFLA_EVENT_REBOOT; > break; > - case NETDEV_CHANGEMTU: > - rtnl_event = IFLA_EVENT_CHANGE_MTU; > - break; > case NETDEV_CHANGEADDR: > rtnl_event = IFLA_EVENT_CHANGE_ADDR; > break; > @@ -1312,9 +1309,6 @@ static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event) > case NETDEV_RESEND_IGMP: > rtnl_event = IFLA_EVENT_RESEND_IGMP; > break; > - case NETDEV_PRECHANGEMTU: > - rtnl_event = IFLA_EVENT_PRE_CHANGE_MTU; > - break; > case NETDEV_CHANGEINFODATA: > rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA; > break; > @@ -4191,7 +4185,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi > > switch (event) { > case NETDEV_REBOOT: > - case NETDEV_CHANGEMTU: > case NETDEV_CHANGEADDR: > case NETDEV_CHANGENAME: > case NETDEV_FEAT_CHANGE: > @@ -4200,7 +4193,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi > case NETDEV_NOTIFY_PEERS: > case NETDEV_CHANGEUPPER: > case NETDEV_RESEND_IGMP: > - case NETDEV_PRECHANGEMTU: > case NETDEV_CHANGEINFODATA: > case NETDEV_PRECHANGEUPPER: > case NETDEV_CHANGELOWERSTATE: >
On 4/8/17 9:36 PM, Vlad Yasevich wrote: > On 04/07/2017 05:25 PM, David Ahern wrote: >> Changing MTU on a link currently causes 3 messages to be sent to userspace: >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default >> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >> >> Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. > Actually, I have plans for the CHANGE_MTU event. The last message is not an event. If you > remove the event, it is much harder to track mtu changes. it is a set of redundant messages. Roopa suggested using a bitmask for the events changed by do_setlink which gets added to the last message seen above. In doing so you only generate 1 message. The above causes libnl (for example): - message 1: delete dummy1 from its cache and re-create it - message 2: delete dummy1 from its cache and re-create it - message 3: delete dummy1 from its cache and re-create it All you need is a single message at the end with an attribute that says "MTU changed"
On 04/08/2017 02:18 PM, Roopa Prabhu wrote: > On 4/8/17, 11:13 AM, David Ahern wrote: >> On 4/8/17 2:06 PM, Roopa Prabhu wrote: >>> On 4/7/17, 2:25 PM, David Ahern wrote: >>>> Changing MTU on a link currently causes 3 messages to be sent to userspace: >>>> >>>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU >>>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>>> >>>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU >>>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>>> >>>> [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default >>>> link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff >>>> >>>> Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. >>>> >>>> >>> This change is good... multiple notifications for the same event does not help in large scale links setups. However, this >>> reverts what vlad was trying to do with his patchset. Vlad's patch-set relies on the rtnl notifications generated from >>> notifiers (rtnetlink_event) to add specific event (IFLA_EVENT) in notifications. >>> >>> The third notification in your example above is the correct one and is an aggregate notification for a set of changes, but >>> it cannot really fill in all types of events in the single IFLA_EVENT attribute as it stands today. IFLA_EVENT should be >>> a bitmask to include all events in this case (i had indicated this in vlads first version). >>> >> Agreed. I think it would be best to revert def12888c161 before the UAPI >> goes out. >> >> The change can instead add the IFLA_EVENT as a bitmask mentioned here to >> note the changes in a setlink. On top of that, remove the notifications >> for the events I mentioned in this set to reduce the overhead on userspace. > > ack > OK, so this will work for the events that are generated as a result of device state change (like mtu, address, and others). However, the original event data may be needed for other events that may be of use to userspace like NETDEV_NOTIFY_PEERS and NETDEV_RESEND_IGMP (possibly others...) -vlad
On 4/10/17 9:39 AM, Vlad Yasevich wrote: > OK, so this will work for the events that are generated as a result of device state change > (like mtu, address, and others). > > However, the original event data may be needed for other events that may be > of use to userspace like NETDEV_NOTIFY_PEERS and NETDEV_RESEND_IGMP (possibly others...) sure. My objection is to multiple messages with identical content. I think the rtnetlink_event message is unique for those 2 netdev events, so no objections if it has value.
On 04/10/2017 11:49 AM, David Ahern wrote: > On 4/10/17 9:39 AM, Vlad Yasevich wrote: >> OK, so this will work for the events that are generated as a result of device state change >> (like mtu, address, and others). >> >> However, the original event data may be needed for other events that may be >> of use to userspace like NETDEV_NOTIFY_PEERS and NETDEV_RESEND_IGMP (possibly others...) > > sure. My objection is to multiple messages with identical content. > > I think the rtnetlink_event message is unique for those 2 netdev events, > so no objections if it has value. > So, I've been looking at adding a bitmap and collecting all modification, however I ran into an interesting issue in do_setlink. Currently the notifications from do_setlink() don't appear to work as one would expect and it's somewhat confusing upon deeper inspection. We have 2 values DO_SETLINK_MODIFIED (1) and DO_SETLINK_NOTIFY (3). These 2 'attempt' to do different jobs, but really fail at it. The function will generate notifications regardless of which of the above values is used. Those changes were done in commit ba9989069f4e426b1e0ed7018eacc9e1ba607095 (cc Nicolas just in case he remembers the history) I am not sure which changes should really trigger a call netdev_state_change(), thus this message. Right now, all changes done in this function trigger them. If that's how that should function, that I can simplify the code. If not, then some of the changes may require us to export the event to the user. To use the dreaded NETDEV_CHANGEMTU event as an example, we used to generate 3 messages (PRECHANGEMTU, CHANGEMTU, and a message from netdev_state_change). With recent changes, we now only generate a message from netdev_state_change. However, mtu change is tagged with DO_SETLINK_MODIFIED which doesn't include the notify bit. So, should there be a NETDEV_CHANGE event associated with this change and a rtnl message (as it is now) or not? It's unclear. -vlad
+Nicolas On 4/14/17 7:51 PM, Vlad Yasevich wrote: > On 04/10/2017 11:49 AM, David Ahern wrote: >> On 4/10/17 9:39 AM, Vlad Yasevich wrote: >>> OK, so this will work for the events that are generated as a result of device state change >>> (like mtu, address, and others). >>> >>> However, the original event data may be needed for other events that may be >>> of use to userspace like NETDEV_NOTIFY_PEERS and NETDEV_RESEND_IGMP (possibly others...) >> >> sure. My objection is to multiple messages with identical content. >> >> I think the rtnetlink_event message is unique for those 2 netdev events, >> so no objections if it has value. >> > > So, I've been looking at adding a bitmap and collecting all modification, however > I ran into an interesting issue in do_setlink. > > Currently the notifications from do_setlink() don't appear to work as one would > expect and it's somewhat confusing upon deeper inspection. > > We have 2 values DO_SETLINK_MODIFIED (1) and DO_SETLINK_NOTIFY (3). These 2 'attempt' > to do different jobs, but really fail at it. The function will generate notifications > regardless of which of the above values is used. > > Those changes were done in commit ba9989069f4e426b1e0ed7018eacc9e1ba607095 (cc Nicolas > just in case he remembers the history) > > I am not sure which changes should really trigger a call netdev_state_change(), thus this > message. Right now, all changes done in this function trigger them. If that's how that > should function, that I can simplify the code. If not, then some of the changes may > require us to export the event to the user. > > To use the dreaded NETDEV_CHANGEMTU event as an example, we used to generate 3 messages > (PRECHANGEMTU, CHANGEMTU, and a message from netdev_state_change). With recent changes, > we now only generate a message from netdev_state_change. However, mtu change is tagged > with DO_SETLINK_MODIFIED which doesn't include the notify bit. So, should there be a > NETDEV_CHANGE event associated with this change and a rtnl message (as it is now) or not? > It's unclear. > > -vlad >
Le 15/04/2017 à 06:26, David Ahern a écrit : > +Nicolas > > On 4/14/17 7:51 PM, Vlad Yasevich wrote: >> On 04/10/2017 11:49 AM, David Ahern wrote: >>> On 4/10/17 9:39 AM, Vlad Yasevich wrote: >>>> OK, so this will work for the events that are generated as a result of device state change >>>> (like mtu, address, and others). >>>> >>>> However, the original event data may be needed for other events that may be >>>> of use to userspace like NETDEV_NOTIFY_PEERS and NETDEV_RESEND_IGMP (possibly others...) >>> >>> sure. My objection is to multiple messages with identical content. >>> >>> I think the rtnetlink_event message is unique for those 2 netdev events, >>> so no objections if it has value. >>> >> >> So, I've been looking at adding a bitmap and collecting all modification, however >> I ran into an interesting issue in do_setlink. >> >> Currently the notifications from do_setlink() don't appear to work as one would >> expect and it's somewhat confusing upon deeper inspection. >> >> We have 2 values DO_SETLINK_MODIFIED (1) and DO_SETLINK_NOTIFY (3). These 2 'attempt' >> to do different jobs, but really fail at it. The function will generate notifications >> regardless of which of the above values is used. >> >> Those changes were done in commit ba9989069f4e426b1e0ed7018eacc9e1ba607095 (cc Nicolas >> just in case he remembers the history) >> >> I am not sure which changes should really trigger a call netdev_state_change(), thus this >> message. Right now, all changes done in this function trigger them. If that's how that >> should function, that I can simplify the code. If not, then some of the changes may >> require us to export the event to the user. If I remember well, the goal was to trigger a netlink event as soon as 'something' changed. It was not the case before that patch, some updates (I don't remember which one) were not advertised via netlink. >> >> To use the dreaded NETDEV_CHANGEMTU event as an example, we used to generate 3 messages >> (PRECHANGEMTU, CHANGEMTU, and a message from netdev_state_change). With recent changes, >> we now only generate a message from netdev_state_change. However, mtu change is tagged >> with DO_SETLINK_MODIFIED which doesn't include the notify bit. So, should there be a >> NETDEV_CHANGE event associated with this change and a rtnl message (as it is now) or not? The rtnl msg is mandatory. Regards, Nicolas
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 97f6d302f627..e8b7e9342cc0 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -903,7 +903,6 @@ enum { enum { IFLA_EVENT_UNSPEC, IFLA_EVENT_REBOOT, - IFLA_EVENT_CHANGE_MTU, IFLA_EVENT_CHANGE_ADDR, IFLA_EVENT_CHANGE_NAME, IFLA_EVENT_FEAT_CHANGE, @@ -912,7 +911,6 @@ enum { 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, diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index b2bd4c9ee860..72d365ae14b3 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1285,9 +1285,6 @@ static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event) case NETDEV_REBOOT: rtnl_event = IFLA_EVENT_REBOOT; break; - case NETDEV_CHANGEMTU: - rtnl_event = IFLA_EVENT_CHANGE_MTU; - break; case NETDEV_CHANGEADDR: rtnl_event = IFLA_EVENT_CHANGE_ADDR; break; @@ -1312,9 +1309,6 @@ static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event) case NETDEV_RESEND_IGMP: rtnl_event = IFLA_EVENT_RESEND_IGMP; break; - case NETDEV_PRECHANGEMTU: - rtnl_event = IFLA_EVENT_PRE_CHANGE_MTU; - break; case NETDEV_CHANGEINFODATA: rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA; break; @@ -4191,7 +4185,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi switch (event) { case NETDEV_REBOOT: - case NETDEV_CHANGEMTU: case NETDEV_CHANGEADDR: case NETDEV_CHANGENAME: case NETDEV_FEAT_CHANGE: @@ -4200,7 +4193,6 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi case NETDEV_NOTIFY_PEERS: case NETDEV_CHANGEUPPER: case NETDEV_RESEND_IGMP: - case NETDEV_PRECHANGEMTU: case NETDEV_CHANGEINFODATA: case NETDEV_PRECHANGEUPPER: case NETDEV_CHANGELOWERSTATE:
Changing MTU on a link currently causes 3 messages to be sent to userspace: [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1490 qdisc noqueue state UNKNOWN group default event PRE_CHANGE_MTU link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default event CHANGE_MTU link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff [LINK]11: dummy1: <BROADCAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default link/ether f2:52:5c:6d:21:f3 brd ff:ff:ff:ff:ff:ff Remove the PRE_CHANGE_MTU and CHANGE_MTU messages. Signed-off-by: David Ahern <dsa@cumulusnetworks.com> --- include/uapi/linux/if_link.h | 2 -- net/core/rtnetlink.c | 8 -------- 2 files changed, 10 deletions(-)