| Message ID | 1465576339-17641-1-git-send-email-lrichard@redhat.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Lance Richardson <lrichard@redhat.com> Date: Fri, 10 Jun 2016 12:32:19 -0400 > Currently, when creating a veth pair, notfications to user > space only include link peer for one end of the veth pair: > # ip monitor link & > # ip link add dev vm1 type veth peer name vm2 > 30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN > link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff > 31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff > > With this change, netlink notifications are sent with complete > information for both interfaces of the veth pair: > > # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN > link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff > 4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff > 3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff > > Signed-off-by: Lance Richardson <lrichard@redhat.com> I don't know about this. First of all, those notifications you get above tell you everything you need to know in order to figure out what both ends of the veth pair are. In fact, I would say that the vm1@vm2 notification #31 above is the _only_ one you absolutely need. > @@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, > > priv = netdev_priv(peer); > rcu_assign_pointer(priv->peer, dev); > + > + err = rtnl_configure_link(dev, NULL); > + if (err < 0) > + goto err_configure_dev; > + > + rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL); > return 0; > > +err_configure_dev: > + /* nothing to do */ > err_register_dev: > /* nothing to do */ > err_configure_peer: If you're registering the peer here explicitly, this means a link configure somewhere else is now superfluous. I really don't like this change at all, both from a necessity perspective as well as from it's implementation.
----- Original Message ----- > From: "David Miller" <davem@davemloft.net> > To: lrichard@redhat.com > Cc: netdev@vger.kernel.org, "nicolas dichtel" <nicolas.dichtel@6wind.com> > Sent: Saturday, June 11, 2016 6:43:40 PM > Subject: Re: [PATCHi next] veth: advertise peer link relationship for both devices > > From: Lance Richardson <lrichard@redhat.com> > Date: Fri, 10 Jun 2016 12:32:19 -0400 > > > Currently, when creating a veth pair, notfications to user > > space only include link peer for one end of the veth pair: > > # ip monitor link & > > # ip link add dev vm1 type veth peer name vm2 > > 30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN > > link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff > > 31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > > link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff > > > > With this change, netlink notifications are sent with complete > > information for both interfaces of the veth pair: > > > > # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN > > link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff > > 4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > > link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff > > 3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN > > link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff > > > > Signed-off-by: Lance Richardson <lrichard@redhat.com> > > I don't know about this. > > First of all, those notifications you get above tell you everything you > need to know in order to figure out what both ends of the veth pair are. > > In fact, I would say that the vm1@vm2 notification #31 above is the _only_ > one you absolutely need. > > > @@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct > > net_device *dev, > > > > priv = netdev_priv(peer); > > rcu_assign_pointer(priv->peer, dev); > > + > > + err = rtnl_configure_link(dev, NULL); > > + if (err < 0) > > + goto err_configure_dev; > > + > > + rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL); > > return 0; > > > > +err_configure_dev: > > + /* nothing to do */ > > err_register_dev: > > /* nothing to do */ > > err_configure_peer: > > If you're registering the peer here explicitly, this means a link configure > somewhere else is now superfluous. > > I really don't like this change at all, both from a necessity perspective as > well as from it's implementation. > I'll confess to not being super-happy with it myself, which is why I've been sitting on this patch for some time now. A hard NAK will help justify a "will not fix" to the reporter of this issue. Thanks, Lance
From: Lance Richardson <lrichard@redhat.com> Date: Sat, 11 Jun 2016 22:04:13 -0400 (EDT) > I'll confess to not being super-happy with it myself, which is why I've > been sitting on this patch for some time now. A hard NAK will help justify > a "will not fix" to the reporter of this issue. Since userspace can learn everything is needs to using the existing notifications, there is no reason for this change, NACK.
diff --git a/drivers/net/veth.c b/drivers/net/veth.c index f37a6e6..9151686 100644 --- a/drivers/net/veth.c +++ b/drivers/net/veth.c @@ -458,7 +458,7 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, netif_carrier_off(dev); /* - * tie the deviced together + * tie the devices together */ priv = netdev_priv(dev); @@ -466,8 +466,16 @@ static int veth_newlink(struct net *src_net, struct net_device *dev, priv = netdev_priv(peer); rcu_assign_pointer(priv->peer, dev); + + err = rtnl_configure_link(dev, NULL); + if (err < 0) + goto err_configure_dev; + + rtmsg_ifinfo(RTM_NEWLINK, peer, 0, GFP_KERNEL); return 0; +err_configure_dev: + /* nothing to do */ err_register_dev: /* nothing to do */ err_configure_peer: diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index d69c464..e0956bb 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2165,6 +2165,7 @@ static int rtnl_dellink(struct sk_buff *skb, struct nlmsghdr *nlh) int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm) { unsigned int old_flags; + unsigned int gchanges; int err; old_flags = dev->flags; @@ -2174,9 +2175,14 @@ int rtnl_configure_link(struct net_device *dev, const struct ifinfomsg *ifm) return err; } - dev->rtnl_link_state = RTNL_LINK_INITIALIZED; + if (dev->rtnl_link_state == RTNL_LINK_INITIALIZING) { + dev->rtnl_link_state = RTNL_LINK_INITIALIZED; + gchanges = ~0U; + } else { + gchanges = dev->flags ^ old_flags; + } - __dev_notify_flags(dev, old_flags, ~0U); + __dev_notify_flags(dev, old_flags, gchanges); return 0; } EXPORT_SYMBOL(rtnl_configure_link);
Currently, when creating a veth pair, notfications to user space only include link peer for one end of the veth pair: # ip monitor link & # ip link add dev vm1 type veth peer name vm2 30: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN link/ether be:e3:b7:0e:14:52 brd ff:ff:ff:ff:ff:ff 31: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN link/ether da:e6:a6:c5:42:54 brd ff:ff:ff:ff:ff:ff With this change, netlink notifications are sent with complete information for both interfaces of the veth pair: # 3: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff 4: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN link/ether b2:05:70:e0:fc:35 brd ff:ff:ff:ff:ff:ff 3: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN link/ether e2:94:54:8a:ac:f5 brd ff:ff:ff:ff:ff:ff Signed-off-by: Lance Richardson <lrichard@redhat.com> --- drivers/net/veth.c | 10 +++++++++- net/core/rtnetlink.c | 10 ++++++++-- 2 files changed, 17 insertions(+), 3 deletions(-)