* [PATCHi next] veth: advertise peer link relationship for both devices
@ 2016-06-10 16:32 Lance Richardson
2016-06-11 22:43 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Lance Richardson @ 2016-06-10 16:32 UTC (permalink / raw)
To: netdev; +Cc: nicolas.dichtel
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(-)
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);
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCHi next] veth: advertise peer link relationship for both devices
2016-06-10 16:32 [PATCHi next] veth: advertise peer link relationship for both devices Lance Richardson
@ 2016-06-11 22:43 ` David Miller
2016-06-12 2:04 ` Lance Richardson
0 siblings, 1 reply; 4+ messages in thread
From: David Miller @ 2016-06-11 22:43 UTC (permalink / raw)
To: lrichard; +Cc: netdev, nicolas.dichtel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHi next] veth: advertise peer link relationship for both devices
2016-06-11 22:43 ` David Miller
@ 2016-06-12 2:04 ` Lance Richardson
2016-06-12 2:30 ` David Miller
0 siblings, 1 reply; 4+ messages in thread
From: Lance Richardson @ 2016-06-12 2:04 UTC (permalink / raw)
To: David Miller; +Cc: netdev, nicolas dichtel
----- 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCHi next] veth: advertise peer link relationship for both devices
2016-06-12 2:04 ` Lance Richardson
@ 2016-06-12 2:30 ` David Miller
0 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2016-06-12 2:30 UTC (permalink / raw)
To: lrichard; +Cc: netdev, nicolas.dichtel
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.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-06-12 2:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-10 16:32 [PATCHi next] veth: advertise peer link relationship for both devices Lance Richardson
2016-06-11 22:43 ` David Miller
2016-06-12 2:04 ` Lance Richardson
2016-06-12 2:30 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).