All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
       [not found] <cover.1535712096.git.lorenzo.bianconi@redhat.com>
@ 2018-08-31 11:43 ` Lorenzo Bianconi
  2018-08-31 15:24   ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2018-08-31 11:43 UTC (permalink / raw)
  To: davem; +Cc: netdev

When moving a veth device to another namespace, userspace receives a
RTM_DELLINK message indicating the device has been removed from current
netns. However, the other peer does not receive a netlink event
containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
attributes.
Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values

Reviewed-by: Stefano Brivio <sbrivio@redhat.com>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since RFC:
- export rtmsg_ifinfo_build_skb() symbol and do not use rtmsg_ifinfo_newnet()
  directly
---
 drivers/net/veth.c   | 66 +++++++++++++++++++++++++++++++++++++++++++-
 net/core/rtnetlink.c |  1 +
 2 files changed, 66 insertions(+), 1 deletion(-)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 8d679c8b7f25..0caffc93d74d 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -1242,18 +1242,82 @@ static struct rtnl_link_ops veth_link_ops = {
 	.get_link_net	= veth_get_link_net,
 };
 
+static int veth_notify(struct notifier_block *this,
+		       unsigned long event, void *ptr)
+{
+	struct net_device *peer, *dev = netdev_notifier_info_to_dev(ptr);
+	struct net *peer_net, *net = dev_net(dev);
+	int nsid, ret = NOTIFY_DONE;
+	struct veth_priv *priv;
+	struct sk_buff *skb;
+
+	if (dev->netdev_ops != &veth_netdev_ops)
+		return NOTIFY_DONE;
+
+	if (event != NETDEV_REGISTER)
+		return NOTIFY_DONE;
+
+	priv = netdev_priv(dev);
+
+	rcu_read_lock();
+
+	peer = rcu_dereference(priv->peer);
+	if (!peer)
+		goto out;
+
+	peer_net = dev_net(peer);
+	/* do not forward events if both veth devices
+	 * are in the same namespace
+	 */
+	if (peer_net == net)
+		goto out;
+
+	/* notify on peer namespace new IFLA_LINK_NETNSID
+	 * and IFLA_LINK values
+	 */
+	nsid = peernet2id_alloc(peer_net, net);
+	skb = rtmsg_ifinfo_build_skb(RTM_NEWLINK, peer, ~0U,
+				     IFLA_EVENT_NONE, GFP_ATOMIC,
+				     &nsid, dev->ifindex);
+	if (skb) {
+		rtnl_notify(skb, peer_net, 0, RTNLGRP_LINK, NULL,
+			    GFP_ATOMIC);
+		ret = NOTIFY_OK;
+	}
+
+out:
+	rcu_read_unlock();
+
+	return ret;
+}
+
+static struct notifier_block veth_notifier = {
+	.notifier_call = veth_notify,
+};
+
 /*
  * init/fini
  */
 
 static __init int veth_init(void)
 {
-	return rtnl_link_register(&veth_link_ops);
+	int err;
+
+	err = register_netdevice_notifier(&veth_notifier);
+	if (err < 0)
+		return err;
+
+	err = rtnl_link_register(&veth_link_ops);
+	if (err < 0)
+		unregister_netdevice_notifier(&veth_notifier);
+
+	return err;
 }
 
 static __exit void veth_exit(void)
 {
 	rtnl_link_unregister(&veth_link_ops);
+	unregister_netdevice_notifier(&veth_notifier);
 }
 
 module_init(veth_init);
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 24431e578310..b2df84db7670 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -3326,6 +3326,7 @@ struct sk_buff *rtmsg_ifinfo_build_skb(int type, struct net_device *dev,
 		rtnl_set_sk_err(net, RTNLGRP_LINK, err);
 	return NULL;
 }
+EXPORT_SYMBOL(rtmsg_ifinfo_build_skb);
 
 void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
 {
-- 
2.17.1

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-08-31 11:43 ` [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace Lorenzo Bianconi
@ 2018-08-31 15:24   ` David Ahern
  2018-08-31 16:19     ` Lorenzo Bianconi
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-08-31 15:24 UTC (permalink / raw)
  To: Lorenzo Bianconi, davem; +Cc: netdev

On 8/31/18 5:43 AM, Lorenzo Bianconi wrote:
> When moving a veth device to another namespace, userspace receives a
> RTM_DELLINK message indicating the device has been removed from current
> netns. However, the other peer does not receive a netlink event
> containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
> attributes.
> Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
> namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values
> 

A newlink message is generated in the new namespace. What information is
missing from that message?

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-08-31 15:24   ` David Ahern
@ 2018-08-31 16:19     ` Lorenzo Bianconi
  2018-08-31 16:21       ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2018-08-31 16:19 UTC (permalink / raw)
  To: David Ahern; +Cc: davem, netdev

> On 8/31/18 5:43 AM, Lorenzo Bianconi wrote:
> > When moving a veth device to another namespace, userspace receives a
> > RTM_DELLINK message indicating the device has been removed from current
> > netns. However, the other peer does not receive a netlink event
> > containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
> > attributes.
> > Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
> > namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values
> > 
> 
> A newlink message is generated in the new namespace. What information is
> missing from that message?
> 

Hi David,

let's assume we have two veth paired devices (veth0 and veth1) on inet
namespace. When moving a veth1 to another namespace, userspace is notified
with RTM_DELLINK event on inet namespace to indicate that veth1 has been
moved to another namespace. However some userspace applications
(e.g. NetworkManager), listening for events on inet namespace, are interested
in veth1 ifindex in the new namespace. This patch sends a new RTM_NEWLINK event
in inet namespace to provide new values for IFLA_LINK_NETNSID/IFLA_LINK 

Regards,
Lorenzo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-08-31 16:19     ` Lorenzo Bianconi
@ 2018-08-31 16:21       ` David Ahern
  2018-08-31 16:54         ` Lorenzo Bianconi
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-08-31 16:21 UTC (permalink / raw)
  To: Lorenzo Bianconi; +Cc: davem, netdev

On 8/31/18 10:19 AM, Lorenzo Bianconi wrote:
>> On 8/31/18 5:43 AM, Lorenzo Bianconi wrote:
>>> When moving a veth device to another namespace, userspace receives a
>>> RTM_DELLINK message indicating the device has been removed from current
>>> netns. However, the other peer does not receive a netlink event
>>> containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
>>> attributes.
>>> Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
>>> namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values
>>>
>>
>> A newlink message is generated in the new namespace. What information is
>> missing from that message?
>>
> 
> Hi David,
> 
> let's assume we have two veth paired devices (veth0 and veth1) on inet
> namespace. When moving a veth1 to another namespace, userspace is notified
> with RTM_DELLINK event on inet namespace to indicate that veth1 has been
> moved to another namespace. However some userspace applications
> (e.g. NetworkManager), listening for events on inet namespace, are interested
> in veth1 ifindex in the new namespace. This patch sends a new RTM_NEWLINK event
> in inet namespace to provide new values for IFLA_LINK_NETNSID/IFLA_LINK 

This is in init namespace
$ ip li set veth2 netns foo

$ ip monitor
Deleted 20: veth2@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc
noop state DOWN group default
    link/ether c6:d0:d6:c5:23:7d brd ff:ff:ff:ff:ff:ff new-netns foo
new-ifindex 20

It shows the new namespace in the delete message.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-08-31 16:21       ` David Ahern
@ 2018-08-31 16:54         ` Lorenzo Bianconi
  2018-09-01  9:05           ` Lorenzo Bianconi
  0 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2018-08-31 16:54 UTC (permalink / raw)
  To: David Ahern; +Cc: davem, netdev

> On 8/31/18 10:19 AM, Lorenzo Bianconi wrote:
> >> On 8/31/18 5:43 AM, Lorenzo Bianconi wrote:
> >>> When moving a veth device to another namespace, userspace receives a
> >>> RTM_DELLINK message indicating the device has been removed from current
> >>> netns. However, the other peer does not receive a netlink event
> >>> containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
> >>> attributes.
> >>> Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
> >>> namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values
> >>>
> >>
> >> A newlink message is generated in the new namespace. What information is
> >> missing from that message?
> >>
> > 
> > Hi David,
> > 
> > let's assume we have two veth paired devices (veth0 and veth1) on inet
> > namespace. When moving a veth1 to another namespace, userspace is notified
> > with RTM_DELLINK event on inet namespace to indicate that veth1 has been
> > moved to another namespace. However some userspace applications
> > (e.g. NetworkManager), listening for events on inet namespace, are interested
> > in veth1 ifindex in the new namespace. This patch sends a new RTM_NEWLINK event
> > in inet namespace to provide new values for IFLA_LINK_NETNSID/IFLA_LINK 
> 
> This is in init namespace
> $ ip li set veth2 netns foo
> 
> $ ip monitor
> Deleted 20: veth2@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc
> noop state DOWN group default
>     link/ether c6:d0:d6:c5:23:7d brd ff:ff:ff:ff:ff:ff new-netns foo
> new-ifindex 20
> 
> It shows the new namespace in the delete message.

Ops, I have not noticed this info has been already introduced in
the commit 38e01b30563a ("dev: advertise the new ifindex when the netns
iface changes"). Thanks for the hint.

DaveM please drop this patch.

Regards,
Lorenzo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-08-31 16:54         ` Lorenzo Bianconi
@ 2018-09-01  9:05           ` Lorenzo Bianconi
  2018-09-01 23:45             ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Lorenzo Bianconi @ 2018-09-01  9:05 UTC (permalink / raw)
  To: David Ahern; +Cc: David S. Miller, Network Development, Thomas Haller

>
> > On 8/31/18 10:19 AM, Lorenzo Bianconi wrote:
> > >> On 8/31/18 5:43 AM, Lorenzo Bianconi wrote:
> > >>> When moving a veth device to another namespace, userspace receives a
> > >>> RTM_DELLINK message indicating the device has been removed from current
> > >>> netns. However, the other peer does not receive a netlink event
> > >>> containing new values for IFLA_LINK_NETNSID and IFLA_LINK veth
> > >>> attributes.
> > >>> Fix that behaviour sending to userspace a RTM_NEWLINK message in the peer
> > >>> namespace to report new IFLA_LINK_NETNSID/IFLA_LINK values
> > >>>
> > >>
> > >> A newlink message is generated in the new namespace. What information is
> > >> missing from that message?
> > >>
> > >
> > > Hi David,
> > >
> > > let's assume we have two veth paired devices (veth0 and veth1) on inet
> > > namespace. When moving a veth1 to another namespace, userspace is notified
> > > with RTM_DELLINK event on inet namespace to indicate that veth1 has been
> > > moved to another namespace. However some userspace applications
> > > (e.g. NetworkManager), listening for events on inet namespace, are interested
> > > in veth1 ifindex in the new namespace. This patch sends a new RTM_NEWLINK event
> > > in inet namespace to provide new values for IFLA_LINK_NETNSID/IFLA_LINK
> >
> > This is in init namespace
> > $ ip li set veth2 netns foo
> >
> > $ ip monitor
> > Deleted 20: veth2@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc
> > noop state DOWN group default
> >     link/ether c6:d0:d6:c5:23:7d brd ff:ff:ff:ff:ff:ff new-netns foo
> > new-ifindex 20
> >
> > It shows the new namespace in the delete message.
>

Hi David,

I was thinking about the commit 38e01b30563a and then I realized I
misread the code
yesterday. The commit 38e01b30563a provides all relevant info but it
emits the event
for veth1 (the device moved in the new namespace).
An userspace application will not receive that message if it filters
events for just
a specific device (veth0 in this case) despite that some device
properties have changed
(since veth0 and veth1 are paired devices). To fix that behavior in
veth_notify routine
I emits a RTM_NEWLINK event for veth0.

Regards,
Lorenzo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-09-01  9:05           ` Lorenzo Bianconi
@ 2018-09-01 23:45             ` David Ahern
  2018-09-03  9:10               ` Thomas Haller
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-09-01 23:45 UTC (permalink / raw)
  To: Lorenzo Bianconi; +Cc: David S. Miller, Network Development, Thomas Haller

On 9/1/18 3:05 AM, Lorenzo Bianconi wrote:
> 
> I was thinking about the commit 38e01b30563a and then I realized I
> misread the code
> yesterday. The commit 38e01b30563a provides all relevant info but it
> emits the event
> for veth1 (the device moved in the new namespace).
> An userspace application will not receive that message if it filters
> events for just
> a specific device (veth0 in this case) despite that some device
> properties have changed
> (since veth0 and veth1 are paired devices). To fix that behavior in
> veth_notify routine
> I emits a RTM_NEWLINK event for veth0.

Userspace is managing a veth a pair. It moves one of them to a new
namespace and decides to filter messages related to that device before
the move. If it did not filter the DELLINK message it would get the
information you want. That to me is a userspace problem. What am I missing?

Fundamentally, your proposal means 3 messages when moving a device to a
new namespace which is what I think is unnecessary.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-09-01 23:45             ` David Ahern
@ 2018-09-03  9:10               ` Thomas Haller
  2018-09-04  2:54                 ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Haller @ 2018-09-03  9:10 UTC (permalink / raw)
  To: David Ahern, Lorenzo Bianconi; +Cc: David S. Miller, Network Development

[-- Attachment #1: Type: text/plain, Size: 2516 bytes --]

Hi,

On Sat, 2018-09-01 at 17:45 -0600, David Ahern wrote:
> On 9/1/18 3:05 AM, Lorenzo Bianconi wrote:
> > 
> > I was thinking about the commit 38e01b30563a and then I realized I
> > misread the code
> > yesterday. The commit 38e01b30563a provides all relevant info but
> > it
> > emits the event
> > for veth1 (the device moved in the new namespace).
> > An userspace application will not receive that message if it
> > filters
> > events for just
> > a specific device (veth0 in this case) despite that some device
> > properties have changed
> > (since veth0 and veth1 are paired devices). To fix that behavior in
> > veth_notify routine
> > I emits a RTM_NEWLINK event for veth0.
> 
> Userspace is managing a veth a pair. It moves one of them to a new
> namespace and decides to filter messages related to that device
> before
> the move. If it did not filter the DELLINK message it would get the
> information you want. That to me is a userspace problem. What am I
> missing?

The userspace component which moves the veth peer is likely not the
same that is listening to these events.

> Fundamentally, your proposal means 3 messages when moving a device to
> a
> new namespace which is what I think is unnecessary.

You are correct, the necessary information is signaled in this case.

Usually, one can manage the information about one link by considering
only RTM_NEWLINK/RTM_DELLINK message for that link. That seems
desirable behavior of the netlink API, as it simplifies user space
implementations.

For example, libnl3's cache for links doesn't properly handles this,
and you end up with invalid data in the cache. You may call that a
userspace problem and bug. But does it really need to be that
complicated?

FWIW, a similar thing was also NACK'ed earlier:
  http://lists.openwall.net/netdev/2016/06/12/8



Paolo and Lorenzo pointed out another issue when moving the peer to a
third namespace:

>>>>>>>>>>>>>>>>>>>>>>>>>>>

ip netns del ns1
ip netns del ns2
ip netns del ns3
ip netns add ns1
ip netns add ns2
ip netns add ns3


ip -n ns1 link add dev veth0 type veth peer name veth1
ip -n ns1 monitor link &

ip -n ns1 link set veth1 netns ns2
# Deleted 9: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default 
#    link/ether 86:79:1d:c6:45:bc brd ff:ff:ff:ff:ff:ff new-netnsid 0 new-ifindex 9

ip -n ns2 link set veth1 netns ns3
# no event.

>>>>>>>>>>>>>>>>>>>>>>





best,
Thomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-09-03  9:10               ` Thomas Haller
@ 2018-09-04  2:54                 ` David Ahern
  2018-09-07 18:52                   ` Thomas Haller
  0 siblings, 1 reply; 11+ messages in thread
From: David Ahern @ 2018-09-04  2:54 UTC (permalink / raw)
  To: Thomas Haller, Lorenzo Bianconi; +Cc: David S. Miller, Network Development

On 9/3/18 3:10 AM, Thomas Haller wrote:
> Hi,
> 
> On Sat, 2018-09-01 at 17:45 -0600, David Ahern wrote:
>> On 9/1/18 3:05 AM, Lorenzo Bianconi wrote:
>>>
>>> I was thinking about the commit 38e01b30563a and then I realized I
>>> misread the code
>>> yesterday. The commit 38e01b30563a provides all relevant info but
>>> it
>>> emits the event
>>> for veth1 (the device moved in the new namespace).
>>> An userspace application will not receive that message if it
>>> filters
>>> events for just
>>> a specific device (veth0 in this case) despite that some device
>>> properties have changed
>>> (since veth0 and veth1 are paired devices). To fix that behavior in
>>> veth_notify routine
>>> I emits a RTM_NEWLINK event for veth0.
>>
>> Userspace is managing a veth a pair. It moves one of them to a new
>> namespace and decides to filter messages related to that device
>> before
>> the move. If it did not filter the DELLINK message it would get the
>> information you want. That to me is a userspace problem. What am I
>> missing?
> 
> The userspace component which moves the veth peer is likely not the
> same that is listening to these events.
> 
>> Fundamentally, your proposal means 3 messages when moving a device to
>> a
>> new namespace which is what I think is unnecessary.
> 
> You are correct, the necessary information is signaled in this case.
> 
> Usually, one can manage the information about one link by considering
> only RTM_NEWLINK/RTM_DELLINK message for that link. That seems
> desirable behavior of the netlink API, as it simplifies user space
> implementations.
> 
> For example, libnl3's cache for links doesn't properly handles this,
> and you end up with invalid data in the cache. You may call that a
> userspace problem and bug. But does it really need to be that
> complicated?
> 
> FWIW, a similar thing was also NACK'ed earlier:
>   http://lists.openwall.net/netdev/2016/06/12/8

And from what I can see, I agree with that NACK; a 3rd message is not
necessary. Userspace has the ability to learn what it needs and should
be looking at the existing options that bleed data across namespaces.

> 
> 
> 
> Paolo and Lorenzo pointed out another issue when moving the peer to a
> third namespace:
> 
>>>>>>>>>>>>>>>>>>>>>>>>>>>>
> 
> ip netns del ns1
> ip netns del ns2
> ip netns del ns3
> ip netns add ns1
> ip netns add ns2
> ip netns add ns3
> 
> 
> ip -n ns1 link add dev veth0 type veth peer name veth1
> ip -n ns1 monitor link &
> 
> ip -n ns1 link set veth1 netns ns2
> # Deleted 9: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN group default 
> #    link/ether 86:79:1d:c6:45:bc brd ff:ff:ff:ff:ff:ff new-netnsid 0 new-ifindex 9
> 
> ip -n ns2 link set veth1 netns ns3
> # no event.


ip li add veth1 type veth peer name veth2
ip netns add foo
ip netns add bar
--> start ip monitor here; see below

ip li set veth2 netns foo
ip -netns foo li set veth2 netns bar


>From init_net:
$ ip monitor all-nsid
[nsid current]Deleted inet veth2
[nsid current]Deleted inet6 veth2
[nsid current]nsid 0 (iproute2 netns name: foo)
[nsid current]Deleted 16: veth2@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu
1500 qdisc noop state DOWN group default
    link/ether 0a:28:87:f9:17:7c brd ff:ff:ff:ff:ff:ff new-netns foo
new-ifindex 16
[nsid current]inet if16 forwarding on rp_filter off mc_forwarding off
proxy_neigh off ignore_routes_with_linkdown off
[nsid current]inet6 if16 forwarding off mc_forwarding off proxy_neigh
off ignore_routes_with_linkdown off
[nsid current]nsid 0 (iproute2 netns name: foo)
[nsid current]16: veth2@if17: <BROADCAST,MULTICAST> mtu 1500 qdisc noop
state DOWN group default
    link/ether 0a:28:87:f9:17:7c brd ff:ff:ff:ff:ff:ff link-netnsid 0
[nsid current]Deleted inet veth2
[nsid current]Deleted inet6 veth2
[nsid current]nsid 1
[nsid current]Deleted 16: veth2@if17: <BROADCAST,MULTICAST> mtu 1500
qdisc noop state DOWN group default
    link/ether 0a:28:87:f9:17:7c brd ff:ff:ff:ff:ff:ff link-netnsid 0
new-netnsid 1 new-ifindex 16

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-09-04  2:54                 ` David Ahern
@ 2018-09-07 18:52                   ` Thomas Haller
  2018-09-09  1:16                     ` David Ahern
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Haller @ 2018-09-07 18:52 UTC (permalink / raw)
  To: David Ahern, Lorenzo Bianconi; +Cc: David S. Miller, Network Development

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

Hi David,


On Mon, 2018-09-03 at 20:54 -0600, David Ahern wrote:

> From init_net:
> $ ip monitor all-nsid

I thought the concern of the patch is the overhead of sending one
additional RTM_NEWLINK message. This workaround has likely higher
overhead. More importantly, it's so cumbersome, that I doubt anybody
would implementing such a solution.

When the events of one namespace are not sufficient to get all relevant
information (local to the namespace itself), the solution is not
monitor all-nsid.

You might save complexity and performance overhead in kernel. But what
you save here is just moved to user-space, which faces higher
complexity (at multiple places/projects, where developers are not
experts in netlink) and higher overhead.


RTM_GETLINK/NLM_F_DUMP allows to fetch information. The same
information is usually also emitted on changes via RTM_NEWLINK
notifications.
Yes, the information may be avilable somehow, but how cumbersome:

 a) receive RTM_DELLINK, recognize that the message belongs to a 
   veth that was moved to another netns, and recognize that the peer's 
   IFLA_LINK changed. This approach only works when the veth is moved
   away from the current namespace.
 b) or, enable monitor all-nsid, receive RTM_NEWLINK, recognize that 
   the event is for moving a veth between netns, find the peer in the 
   other netns and recognize that the peer's IFLA_LINK changed.


best,
THomas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace
  2018-09-07 18:52                   ` Thomas Haller
@ 2018-09-09  1:16                     ` David Ahern
  0 siblings, 0 replies; 11+ messages in thread
From: David Ahern @ 2018-09-09  1:16 UTC (permalink / raw)
  To: Thomas Haller, Lorenzo Bianconi; +Cc: David S. Miller, Network Development

Hi Thomas:

On 9/7/18 12:52 PM, Thomas Haller wrote:
> Hi David,
> 
> 
> On Mon, 2018-09-03 at 20:54 -0600, David Ahern wrote:
> 
>> From init_net:
>> $ ip monitor all-nsid
> 
> I thought the concern of the patch is the overhead of sending one
> additional RTM_NEWLINK message. This workaround has likely higher
> overhead. More importantly, it's so cumbersome, that I doubt anybody
> would implementing such a solution.

Yes, the concern is additional messages. Each one adds non-negligible
overhead to build and send, something that is measurable in a system at
scale. For example, consider an interface manager creating 4k VLANs on
one or more bridges. The RTM_NEWLINK messages (there are 2) adds 33% to
the overhead and time of creating the vlans. Just in some quick tests I
see times vary from as little as 15 usec to over 1 msec - per message
with the RTNL held. The norm seems to be somewhere between 35 and 40
usecs, but those add up when it is per link and per action.

We have to more disciplined about the size and frequency of these messages.

> 
> When the events of one namespace are not sufficient to get all relevant
> information (local to the namespace itself), the solution is not
> monitor all-nsid.
> 
> You might save complexity and performance overhead in kernel. But what
> you save here is just moved to user-space, which faces higher
> complexity (at multiple places/projects, where developers are not
> experts in netlink) and higher overhead.

First, this is one use case that seems to care about a message coming
back on the veth pairs. The message is sent for everyone, always. Adding
a 3rd message puts additional load on all use cases.

Second, I am questioning this use case driving a kernel change. You say
a userspace app cares about a veth pair but only wants to monitor events
for one end of it. That's a userspace choice. Another argument brought
up is that the other end of the pair can change namespaces faster than
the app can track it. Sure. But the monitored end can move as well - and
faster than the app can track it. e.g.,

1. veth1 and veth2 are a pair.
2. app filters events for veth1 only
3. veth2 is moved to namespace foo
4. app is sent notification of move
5. veth2 is moved to namespace bar
6. app processes notification of the event in step 3, looks in namespace
foo and link is gone.

That seems to be the fundamentals of your request for this new message, yes?

What happens when veth1 is moved to another namespace and then another
namespace - faster than the app can not track it? You have the same
problem. Your use case may not have this problem, but generically it can
happen and this solution does not cover it.

The movement of links is provided to userspace, and userspace needs to
be smart about what it wants to do.

Alternatively, you could provide an API for your interface manager --
whatever is handling the link movement. It know where it is moving
interfaces. You could have it respond to inquiries of where device X is
at any given moment.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-09-09  6:04 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1535712096.git.lorenzo.bianconi@redhat.com>
2018-08-31 11:43 ` [PATCH net-next] veth: report NEWLINK event when moving the peer device in a new namespace Lorenzo Bianconi
2018-08-31 15:24   ` David Ahern
2018-08-31 16:19     ` Lorenzo Bianconi
2018-08-31 16:21       ` David Ahern
2018-08-31 16:54         ` Lorenzo Bianconi
2018-09-01  9:05           ` Lorenzo Bianconi
2018-09-01 23:45             ` David Ahern
2018-09-03  9:10               ` Thomas Haller
2018-09-04  2:54                 ` David Ahern
2018-09-07 18:52                   ` Thomas Haller
2018-09-09  1:16                     ` David Ahern

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.