[PATCH net-next] rtnl: Add support for netdev event to link messages

[PATCH net-next] rtnl: Add support for netdev event to link messages

[Vladislav Yasevich]

RTNL currently generates notifications on some netdev notifier events.
However, user space has no idea what changed.  All it sees is the
data and has to infer what has changed.  For some events that is not
possible.

This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
that would have an encoding of the which event triggered this
notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
NETDEV_MTUCHANGED) are supported.  These events could be interesting
in the virt space to trigger additional configuration commands to VMs.
Other events of interest may be added later.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 include/linux/rtnetlink.h    |  3 ++-
 include/uapi/linux/if_link.h |  7 ++++++
 net/core/dev.c               |  2 +-
 net/core/rtnetlink.c         | 51 ++++++++++++++++++++++++++++++++++++--------
 4 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/include/linux/rtnetlink.h b/include/linux/rtnetlink.h
index 57e5484..0459018 100644
--- a/include/linux/rtnetlink.h
+++ b/include/linux/rtnetlink.h
@@ -18,7 +18,8 @@ extern int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst,
 
 void rtmsg_ifinfo(int type, struct net_device *dev, unsigned change, gfp_t flags);
 struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
-				       unsigned change, gfp_t flags);
+				       unsigned change, unsigned long event,
+				       gfp_t flags);
 void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev,
 		       gfp_t flags);
 
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 320fc1e..8164d56 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -157,6 +157,7 @@ enum {
 	IFLA_GSO_MAX_SIZE,
 	IFLA_PAD,
 	IFLA_XDP,
+	IFLA_EVENT,
 	__IFLA_MAX
 };
 
@@ -890,6 +891,12 @@ enum {
 	__IFLA_XDP_MAX,
 };
 
+enum {
+	IFLA_EVENT_UNSPEC,
+	IFLA_EVENT_CHANGE_MTU,
+	IFLA_EVENT_NOTIFY_PEERS,
+};
+
 #define IFLA_XDP_MAX (__IFLA_XDP_MAX - 1)
 
 #endif /* _UAPI_LINUX_IF_LINK_H */
diff --git a/net/core/dev.c b/net/core/dev.c
index ef9fe60e..7efb417 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -6840,7 +6840,7 @@ static void rollback_registered_many(struct list_head *head)
 
 		if (!dev->rtnl_link_ops ||
 		    dev->rtnl_link_state == RTNL_LINK_INITIALIZED)
-			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U,
+			skb = rtmsg_ifinfo_build_skb(RTM_DELLINK, dev, ~0U, 0,
 						     GFP_KERNEL);
 
 		/*
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9c3947a..63f5854 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -944,6 +944,7 @@ static noinline size_t if_nlmsg_size(const struct net_device *dev,
 	       + nla_total_size(MAX_PHYS_ITEM_ID_LEN) /* IFLA_PHYS_SWITCH_ID */
 	       + nla_total_size(IFNAMSIZ) /* IFLA_PHYS_PORT_NAME */
 	       + rtnl_xdp_size(dev) /* IFLA_XDP */
+	       + nla_total_size(4)  /* IFLA_EVENT */
 	       + nla_total_size(1); /* IFLA_PROTO_DOWN */
 
 }
@@ -1276,9 +1277,28 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
 	return err;
 }
 
+static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
+{
+	u32 rtnl_event;
+
+	switch (event) {
+	case NETDEV_NOTIFY_PEERS:
+		rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
+		break;
+	case NETDEV_CHANGEMTU:
+		rtnl_event = IFLA_EVENT_CHANGE_MTU;
+		break;
+	default:
+		return 0;
+	}
+
+	return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
+}
+
 static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 			    int type, u32 pid, u32 seq, u32 change,
-			    unsigned int flags, u32 ext_filter_mask)
+			    unsigned int flags, u32 ext_filter_mask,
+			    unsigned long event)
 {
 	struct ifinfomsg *ifm;
 	struct nlmsghdr *nlh;
@@ -1327,6 +1347,9 @@ static int rtnl_fill_ifinfo(struct sk_buff *skb, struct net_device *dev,
 	    nla_put_u8(skb, IFLA_PROTO_DOWN, dev->proto_down))
 		goto nla_put_failure;
 
+	if (rtnl_fill_link_event(skb, event))
+		goto nla_put_failure;
+
 	if (rtnl_fill_link_ifmap(skb, dev))
 		goto nla_put_failure;
 
@@ -1461,6 +1484,7 @@ static const struct nla_policy ifla_policy[IFLA_MAX+1] = {
 	[IFLA_LINK_NETNSID]	= { .type = NLA_S32 },
 	[IFLA_PROTO_DOWN]	= { .type = NLA_U8 },
 	[IFLA_XDP]		= { .type = NLA_NESTED },
+	[IFLA_EVENT]		= { .type = NLA_U32 },
 };
 
 static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
@@ -1619,7 +1643,7 @@ static int rtnl_dump_ifinfo(struct sk_buff *skb, struct netlink_callback *cb)
 					       NETLINK_CB(cb->skb).portid,
 					       cb->nlh->nlmsg_seq, 0,
 					       flags,
-					       ext_filter_mask);
+					       ext_filter_mask, 0);
 			/* If we ran out of room on the first message,
 			 * we're in trouble
 			 */
@@ -2710,7 +2734,7 @@ static int rtnl_getlink(struct sk_buff *skb, struct nlmsghdr* nlh)
 		return -ENOBUFS;
 
 	err = rtnl_fill_ifinfo(nskb, dev, RTM_NEWLINK, NETLINK_CB(skb).portid,
-			       nlh->nlmsg_seq, 0, 0, ext_filter_mask);
+			       nlh->nlmsg_seq, 0, 0, ext_filter_mask, 0);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size */
 		WARN_ON(err == -EMSGSIZE);
@@ -2782,7 +2806,8 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
 }
 
 struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
-				       unsigned int change, gfp_t flags)
+				       unsigned int change, 
+				       unsigned long event, gfp_t flags)
 {
 	struct net *net = dev_net(dev);
 	struct sk_buff *skb;
@@ -2793,7 +2818,7 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 	if (skb == NULL)
 		goto errout;
 
-	err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0);
+	err = rtnl_fill_ifinfo(skb, dev, type, 0, 0, change, 0, 0, event);
 	if (err < 0) {
 		/* -EMSGSIZE implies BUG in if_nlmsg_size() */
 		WARN_ON(err == -EMSGSIZE);
@@ -2814,18 +2839,25 @@ void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
 	rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
 }
 
-void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
-		  gfp_t flags)
+static void rtmsg_ifinfo_event(int type, struct net_device *dev,
+			       unsigned int change, unsigned long event,
+			       gfp_t flags)
 {
 	struct sk_buff *skb;
 
 	if (dev->reg_state != NETREG_REGISTERED)
 		return;
 
-	skb = rtmsg_ifinfo_build_skb(type, dev, change, flags);
+	skb = rtmsg_ifinfo_build_skb(type, dev, change, event, flags);
 	if (skb)
 		rtmsg_ifinfo_send(skb, dev, flags);
 }
+
+void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
+		  gfp_t flags)
+{
+	rtmsg_ifinfo_event(type, dev, change, 0, flags);
+}
 EXPORT_SYMBOL(rtmsg_ifinfo);
 
 static int nlmsg_populate_fdb_fill(struct sk_buff *skb,
@@ -4044,6 +4076,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
 	return skb->len;
 }
 
+
 /* Process one rtnetlink message. */
 
 static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
@@ -4131,7 +4164,7 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 	case NETDEV_BONDING_INFO:
 		break;
 	default:
-		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
+		rtmsg_ifinfo_event(RTM_NEWLINK, dev, 0, event, GFP_KERNEL);
 		break;
 	}
 	return NOTIFY_DONE;
-- 
2.7.4

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[David Miller]

From: Vladislav Yasevich <vyasevich@gmail.com>
Date: Sat, 25 Mar 2017 21:59:47 -0400

> RTNL currently generates notifications on some netdev notifier events.
> However, user space has no idea what changed.  All it sees is the
> data and has to infer what has changed.  For some events that is not
> possible.
> 
> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
> that would have an encoding of the which event triggered this
> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
> NETDEV_MTUCHANGED) are supported.  These events could be interesting
> in the virt space to trigger additional configuration commands to VMs.
> Other events of interest may be added later.
> 
> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>

At what point do we start providing the metadata for the changed
values as well?  You'd probably need to provide both the old and
new values to cover all cases.

> @@ -4044,6 +4076,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>  	return skb->len;
>  }
>  
> +
>  /* Process one rtnetlink message. */
>  
>  static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)

Please don't add more empty lines between functions, one is enough.

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Vlad Yasevich]

[ resending to list.  hit the wrong reply button last time ]

On 03/27/2017 06:58 PM, David Miller wrote:
> From: Vladislav Yasevich <vyasevich@gmail.com>
> Date: Sat, 25 Mar 2017 21:59:47 -0400
> 
>> RTNL currently generates notifications on some netdev notifier events.
>> However, user space has no idea what changed.  All it sees is the
>> data and has to infer what has changed.  For some events that is not
>> possible.
>>
>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>> that would have an encoding of the which event triggered this
>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>> in the virt space to trigger additional configuration commands to VMs.
>> Other events of interest may be added later.
>>
>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
> 
> At what point do we start providing the metadata for the changed
> values as well?  You'd probably need to provide both the old and
> new values to cover all cases.

I don't think if that would be possible because of when events are triggered.
We send these notifications after all the changes have already been made, so
it might be tough to carry old data.

Looking at just the two events I am supporting in this patch, we could actually
supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.
For the use cases I am looking at, it isn't usefull, but easy enough to add.

> 
>> @@ -4044,6 +4076,7 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>  	return skb->len;
>>  }
>>  
>> +
>>  /* Process one rtnetlink message. */
>>  
>>  static int rtnetlink_rcv_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
> 
> Please don't add more empty lines between functions, one is enough.
> 

Sorry, got left-over after moving the code around.  Will remove when resubmitting.

-vlad

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Roopa Prabhu]

On 3/29/17, 5:23 AM, Vlad Yasevich wrote:
> [ resending to list.  hit the wrong reply button last time ]
>
> On 03/27/2017 06:58 PM, David Miller wrote:
>> From: Vladislav Yasevich <vyasevich@gmail.com>
>> Date: Sat, 25 Mar 2017 21:59:47 -0400
>>
>>> RTNL currently generates notifications on some netdev notifier events.
>>> However, user space has no idea what changed.  All it sees is the
>>> data and has to infer what has changed.  For some events that is not
>>> possible.
>>>
>>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>>> that would have an encoding of the which event triggered this
>>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>>> in the virt space to trigger additional configuration commands to VMs.
>>> Other events of interest may be added later.
>>>
>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>> At what point do we start providing the metadata for the changed
>> values as well?  You'd probably need to provide both the old and
>> new values to cover all cases.
> I don't think if that would be possible because of when events are triggered.
> We send these notifications after all the changes have already been made, so
> it might be tough to carry old data.
>
> Looking at just the two events I am supporting in this patch, we could actually
> supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.

But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
changes. There are already enough notifications generated for links (I know you are not
suggesting adding it here)
> For the use cases I am looking at, it isn't usefull, but easy enough to add.
>
Most of the times a single notification can carry multiple changes, this helps user-space..by
cutting down on notifications in systems with large number of links. I don't see IFLA_EVENT attribute
handle multiple changes..

Given the number of attributes for which events are generated, I think a model where user-space
maintains a cache and diff's the new link object with the old one works best in all cases.

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Vlad Yasevich]

On 03/29/2017 12:37 PM, Roopa Prabhu wrote:
> On 3/29/17, 5:23 AM, Vlad Yasevich wrote:
>> [ resending to list.  hit the wrong reply button last time ]
>>
>> On 03/27/2017 06:58 PM, David Miller wrote:
>>> From: Vladislav Yasevich <vyasevich@gmail.com>
>>> Date: Sat, 25 Mar 2017 21:59:47 -0400
>>>
>>>> RTNL currently generates notifications on some netdev notifier events.
>>>> However, user space has no idea what changed.  All it sees is the
>>>> data and has to infer what has changed.  For some events that is not
>>>> possible.
>>>>
>>>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>>>> that would have an encoding of the which event triggered this
>>>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>>>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>>>> in the virt space to trigger additional configuration commands to VMs.
>>>> Other events of interest may be added later.
>>>>
>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>> At what point do we start providing the metadata for the changed
>>> values as well?  You'd probably need to provide both the old and
>>> new values to cover all cases.
>> I don't think if that would be possible because of when events are triggered.
>> We send these notifications after all the changes have already been made, so
>> it might be tough to carry old data.
>>
>> Looking at just the two events I am supporting in this patch, we could actually
>> supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.
> 
> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
> changes. There are already enough notifications generated for links (I know you are not
> suggesting adding it here)

Actually, this one already triggers a link notification to userspace.  It just has
no event data in it to tell you that. :)

>> For the use cases I am looking at, it isn't usefull, but easy enough to add.
>>
> Most of the times a single notification can carry multiple changes, this helps user-space..by
> cutting down on notifications in systems with large number of links. I don't see IFLA_EVENT attribute
> handle multiple changes..
> 

No it doesn't handle multiple changes mainly because we already generate a link
notification for a lot of the events.  This patch doesn't add any additional user space
notifications.  All it does is add the "event" information to existing ones so that user
space may know what happened.

For instance, if you change the mtu on an interface, you get the following:

1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00

The above is the result when you run
 # ip l s lo mtu 1500

With this patch, you'd be able to tell that the notification 2 above was a result of mtu
change.  The first one was a result of "PRECHANGEMTU".  Didn't look to see what the
third one is.

> Given the number of attributes for which events are generated, I think a model where user-space
> maintains a cache and diff's the new link object with the old one works best in all cases.
> 

This patch doesn't preclude this.  It doesn't change how many notifications are generated.
 All it does is a carry a hint as to why a particular notification is generated.

It's also impossible to tell what happened if the data did not change.  As and example,
how does one know that a NETDEV_NOTIFY_PEERS or NETDEV_IGMP_RESEND netdev event occurred?

And if you ask why we need those, consider a case where a VM is connected to the network
through a bridge or macvtap on top of active-backup bond.  Now, there is failover
in the bond and bond generates the above events.  The hypervisor will update the switches
with its own mac/multicast groups, but the VM has no idea this happened.

-vlad

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[David Ahern]

On 3/29/17 11:05 AM, Vlad Yasevich wrote:
> On 03/29/2017 12:37 PM, Roopa Prabhu wrote:
>> On 3/29/17, 5:23 AM, Vlad Yasevich wrote:
>>> [ resending to list.  hit the wrong reply button last time ]
>>>
>>> On 03/27/2017 06:58 PM, David Miller wrote:
>>>> From: Vladislav Yasevich <vyasevich@gmail.com>
>>>> Date: Sat, 25 Mar 2017 21:59:47 -0400
>>>>
>>>>> RTNL currently generates notifications on some netdev notifier events.
>>>>> However, user space has no idea what changed.  All it sees is the
>>>>> data and has to infer what has changed.  For some events that is not
>>>>> possible.
>>>>>
>>>>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>>>>> that would have an encoding of the which event triggered this
>>>>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>>>>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>>>>> in the virt space to trigger additional configuration commands to VMs.
>>>>> Other events of interest may be added later.
>>>>>
>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>> At what point do we start providing the metadata for the changed
>>>> values as well?  You'd probably need to provide both the old and
>>>> new values to cover all cases.
>>> I don't think if that would be possible because of when events are triggered.
>>> We send these notifications after all the changes have already been made, so
>>> it might be tough to carry old data.
>>>
>>> Looking at just the two events I am supporting in this patch, we could actually
>>> supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.
>>
>> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
>> changes. There are already enough notifications generated for links (I know you are not
>> suggesting adding it here)
> 
> Actually, this one already triggers a link notification to userspace.  It just has
> no event data in it to tell you that. :)

Is it intentional or unintentional? perhaps rtnetlink_event should be a
whitelist -- events that userspace should be notified about. Seems like
NETDEV_ events have been added without rtnetlink_event getting updated.
For example, does userspace care about NETDEV_UDP_TUNNEL_PUSH_INFO or
NETDEV_CHANGE_TX_QUEUE_LEN?

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Vlad Yasevich]

On 03/29/2017 03:11 PM, David Ahern wrote:
> On 3/29/17 11:05 AM, Vlad Yasevich wrote:
>> On 03/29/2017 12:37 PM, Roopa Prabhu wrote:
>>> On 3/29/17, 5:23 AM, Vlad Yasevich wrote:
>>>> [ resending to list.  hit the wrong reply button last time ]
>>>>
>>>> On 03/27/2017 06:58 PM, David Miller wrote:
>>>>> From: Vladislav Yasevich <vyasevich@gmail.com>
>>>>> Date: Sat, 25 Mar 2017 21:59:47 -0400
>>>>>
>>>>>> RTNL currently generates notifications on some netdev notifier events.
>>>>>> However, user space has no idea what changed.  All it sees is the
>>>>>> data and has to infer what has changed.  For some events that is not
>>>>>> possible.
>>>>>>
>>>>>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>>>>>> that would have an encoding of the which event triggered this
>>>>>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>>>>>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>>>>>> in the virt space to trigger additional configuration commands to VMs.
>>>>>> Other events of interest may be added later.
>>>>>>
>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>>> At what point do we start providing the metadata for the changed
>>>>> values as well?  You'd probably need to provide both the old and
>>>>> new values to cover all cases.
>>>> I don't think if that would be possible because of when events are triggered.
>>>> We send these notifications after all the changes have already been made, so
>>>> it might be tough to carry old data.
>>>>
>>>> Looking at just the two events I am supporting in this patch, we could actually
>>>> supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.
>>>
>>> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
>>> changes. There are already enough notifications generated for links (I know you are not
>>> suggesting adding it here)
>>
>> Actually, this one already triggers a link notification to userspace.  It just has
>> no event data in it to tell you that. :)
> 
> Is it intentional or unintentional? perhaps rtnetlink_event should be a
> whitelist -- events that userspace should be notified about. Seems like
> NETDEV_ events have been added without rtnetlink_event getting updated.

I think a 'whitelist' was attempted, but as you mentioned, it hasn't been updated...
I'll defer the definitive answer to someone else.  It seems Patrick added a comment
in commit a2835763 to update the white list and it's been a few times.

> For example, does userspace care about NETDEV_UDP_TUNNEL_PUSH_INFO or
> NETDEV_CHANGE_TX_QUEUE_LEN?
> 

Probably not the first, but possibly the second.  If txquelen is changed on a device,
some apps might want to know about it.

-vlad

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Vlad Yasevich]

On 03/30/2017 09:39 AM, Vlad Yasevich wrote:
> On 03/29/2017 03:11 PM, David Ahern wrote:
>> On 3/29/17 11:05 AM, Vlad Yasevich wrote:
>>> On 03/29/2017 12:37 PM, Roopa Prabhu wrote:
>>>> On 3/29/17, 5:23 AM, Vlad Yasevich wrote:
>>>>> [ resending to list.  hit the wrong reply button last time ]
>>>>>
>>>>> On 03/27/2017 06:58 PM, David Miller wrote:
>>>>>> From: Vladislav Yasevich <vyasevich@gmail.com>
>>>>>> Date: Sat, 25 Mar 2017 21:59:47 -0400
>>>>>>
>>>>>>> RTNL currently generates notifications on some netdev notifier events.
>>>>>>> However, user space has no idea what changed.  All it sees is the
>>>>>>> data and has to infer what has changed.  For some events that is not
>>>>>>> possible.
>>>>>>>
>>>>>>> This patch adds a new field to RTM_NEWLINK message called IFLA_EVENT
>>>>>>> that would have an encoding of the which event triggered this
>>>>>>> notification.  Currectly, only 2 events (NETDEV_NOTIFY_PEERS and
>>>>>>> NETDEV_MTUCHANGED) are supported.  These events could be interesting
>>>>>>> in the virt space to trigger additional configuration commands to VMs.
>>>>>>> Other events of interest may be added later.
>>>>>>>
>>>>>>> Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
>>>>>> At what point do we start providing the metadata for the changed
>>>>>> values as well?  You'd probably need to provide both the old and
>>>>>> new values to cover all cases.
>>>>> I don't think if that would be possible because of when events are triggered.
>>>>> We send these notifications after all the changes have already been made, so
>>>>> it might be tough to carry old data.
>>>>>
>>>>> Looking at just the two events I am supporting in this patch, we could actually
>>>>> supply the old mtu data through a NETDEV_PRECHANGEMTU event, if it is necessary.
>>>>
>>>> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
>>>> changes. There are already enough notifications generated for links (I know you are not
>>>> suggesting adding it here)
>>>
>>> Actually, this one already triggers a link notification to userspace.  It just has
>>> no event data in it to tell you that. :)
>>
>> Is it intentional or unintentional? perhaps rtnetlink_event should be a
>> whitelist -- events that userspace should be notified about. Seems like
>> NETDEV_ events have been added without rtnetlink_event getting updated.
> 
> I think a 'whitelist' was attempted, but as you mentioned, it hasn't been updated...
> I'll defer the definitive answer to someone else.  It seems Patrick added a comment
> in commit a2835763 to update the white list and it's been a few times.
>

This is actually an interesting point.  Looking at some commits that have added
events to black list in rtnetlink-event, it might have been much easier to debug
those issues if we had the 'event' encoding in the netlink message.

I think it might be worthwhile to add all allowed event types to this new encoding
so we can userspace can see just what's its getting.

-vlad

>> For example, does userspace care about NETDEV_UDP_TUNNEL_PUSH_INFO or
>> NETDEV_CHANGE_TX_QUEUE_LEN?
>>
> 
> Probably not the first, but possibly the second.  If txquelen is changed on a device,
> some apps might want to know about it.
>

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[David Ahern]

On 3/30/17 7:47 AM, Vlad Yasevich wrote:
>>>>> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
>>>>> changes. There are already enough notifications generated for links (I know you are not
>>>>> suggesting adding it here)
>>>>
>>>> Actually, this one already triggers a link notification to userspace.  It just has
>>>> no event data in it to tell you that. :)
>>>
>>> Is it intentional or unintentional? perhaps rtnetlink_event should be a
>>> whitelist -- events that userspace should be notified about. Seems like
>>> NETDEV_ events have been added without rtnetlink_event getting updated.
>>
>> I think a 'whitelist' was attempted, but as you mentioned, it hasn't been updated...
>> I'll defer the definitive answer to someone else.  It seems Patrick added a comment
>> in commit a2835763 to update the white list and it's been a few times.
>>
> 
> This is actually an interesting point.  Looking at some commits that have added
> events to black list in rtnetlink-event, it might have been much easier to debug
> those issues if we had the 'event' encoding in the netlink message.
> 
> I think it might be worthwhile to add all allowed event types to this new encoding
> so we can userspace can see just what's its getting.
> 

My point is that it is easy to add NETDEV events; takes extra effort to
update rtnetlink_event to say "don't send a notification to userspace".

A number of those events are for kernel processing, so why send anything
to userspace? In that case a default of don't notify userspace and then
having a list of events that should send the notification makes the
intent explicit.

Looking at git commit logs for NETDEV_PRECHANGEMTU, it seems that it was
added for bonding and teaming to simplify kernel processing; userspace
does not need to be notified so no intention there.

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[Vladislav Yasevich]

On 03/30/2017 10:11 AM, David Ahern wrote:
> On 3/30/17 7:47 AM, Vlad Yasevich wrote:
>>>>>> But, NETDEV_PRECHANGEMTU will be a unnecessary notification to userspace without
>>>>>> changes. There are already enough notifications generated for links (I know you are not
>>>>>> suggesting adding it here)
>>>>>
>>>>> Actually, this one already triggers a link notification to userspace.  It just has
>>>>> no event data in it to tell you that. :)
>>>>
>>>> Is it intentional or unintentional? perhaps rtnetlink_event should be a
>>>> whitelist -- events that userspace should be notified about. Seems like
>>>> NETDEV_ events have been added without rtnetlink_event getting updated.
>>>
>>> I think a 'whitelist' was attempted, but as you mentioned, it hasn't been updated...
>>> I'll defer the definitive answer to someone else.  It seems Patrick added a comment
>>> in commit a2835763 to update the white list and it's been a few times.
>>>
>>
>> This is actually an interesting point.  Looking at some commits that have added
>> events to black list in rtnetlink-event, it might have been much easier to debug
>> those issues if we had the 'event' encoding in the netlink message.
>>
>> I think it might be worthwhile to add all allowed event types to this new encoding
>> so we can userspace can see just what's its getting.
>>
> 
> My point is that it is easy to add NETDEV events; takes extra effort to
> update rtnetlink_event to say "don't send a notification to userspace".
> 
> A number of those events are for kernel processing, so why send anything
> to userspace? In that case a default of don't notify userspace and then
> having a list of events that should send the notification makes the
> intent explicit.
> 
> Looking at git commit logs for NETDEV_PRECHANGEMTU, it seems that it was
> added for bonding and teaming to simplify kernel processing; userspace
> does not need to be notified so no intention there.
> 

So, something like the patch below would be better in your opinion as a
starting point.  It'll can at least get the discussion strarted on whether
an event would usefull to user space or not.

However, that's really a separate point from what I was originally try to do.
I would like to provide the event type itself to the user, so the user may
perform some action based on that event.

-- >8 --

From: Vladislav Yasevich <vyasevic@redhat.com>
Date: Thu, 30 Mar 2017 10:42:53 -0400
Subject: [PATCH] rtnetlink: Convert rtnetlink_event to white list

The rtnetlink_event currently functions as a blacklist where
we block cerntain netdev events from being sent to user space.
As a result, events have been added to the system that userspace
probably doesn't care about.

This patch converts the implementation to the white list so that
newly events would have to be specifically added to the list to
be sent to userspace.  This would force new event implementers to
consider whether a given event is usefull to user space or if it's
just a kernel event.

Signed-off-by: Vladislav Yasevich <vyasevic@redhat.com>
---
 net/core/rtnetlink.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 9c3947a..f48a60d 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -4116,22 +4116,23 @@ static int rtnetlink_event(struct notifier_block *this, unsigned long event, voi
 	struct net_device *dev = netdev_notifier_info_to_dev(ptr);
 
 	switch (event) {
-	case NETDEV_UP:
-	case NETDEV_DOWN:
-	case NETDEV_PRE_UP:
-	case NETDEV_POST_INIT:
-	case NETDEV_REGISTER:
-	case NETDEV_CHANGE:
-	case NETDEV_PRE_TYPE_CHANGE:
-	case NETDEV_GOING_DOWN:
-	case NETDEV_UNREGISTER:
-	case NETDEV_UNREGISTER_FINAL:
-	case NETDEV_RELEASE:
-	case NETDEV_JOIN:
-	case NETDEV_BONDING_INFO:
+	case NETDEV_REBOOT:
+	case NETDEV_CHANGEMTU:
+	case NETDEV_CHANGENAME:
+	case NETDEV_FEAT_CHANGE:
+	case NETDEV_BONDING_FAILOVER:
+	case NETDEV_NOTIFY_PEERS:
+	case NETDEV_CHANGEUPPER:
+	case NETDEV_RESEND_IGMP:
+	case NETDEV_PRECHANGEMTU:
+	case NETDEV_CHANGEINFODATA:
+	case NETDEV_PRECHANGEUPPER:
+	case NETDEV_CHANGELOWERSTATE:
+	case NETDEV_UDP_TUNNEL_PUSH_INFO:
+	case NETDEV_CHANGE_TX_QUEUE_LEN:
+		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
 		break;
 	default:
-		rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
 		break;
 	}
 	return NOTIFY_DONE;
-- 
2.7.4

Re: [PATCH net-next] rtnl: Add support for netdev event to link messages

[David Ahern]

On 3/30/17 9:21 AM, Vladislav Yasevich wrote:
> 
> So, something like the patch below would be better in your opinion as a
> starting point.  It'll can at least get the discussion strarted on whether
> an event would usefull to user space or not.

IMO that is a more direct, explicit statement of what is intended to happen.

> 
> However, that's really a separate point from what I was originally try to do.
> I would like to provide the event type itself to the user, so the user may
> perform some action based on that event.


yes, we took a tangent. I was asking about why the PRECHANGEMTU was
causing a message to be sent to userspace.