Re: [net v3] veth: advertise peer link once both links are tied together

2016-06-10 Thread Lance Richardson 
- Original Message -
> From: "Nicolas Dichtel" <nicolas.dich...@6wind.com>
> To: "Lance Richardson" <lrich...@redhat.com>, "Vincent Bernat" 
> <vinc...@bernat.im>
> Cc: "David S. Miller" <da...@davemloft.net>, "Vijay Pandurangan" 
> <vij...@vijayp.ca>, "Paolo Abeni"
> <pab...@redhat.com>, netdev@vger.kernel.org
> Sent: Friday, June 10, 2016 9:15:01 AM
> Subject: Re: [net v3] veth: advertise peer link once both links are tied 
> together
> 
> Le 08/06/2016 22:30, Lance Richardson a écrit :
> [snip]
> > I've been pondering how to fix this very problem off-and-on for a few
> > months
> > now, without having arrived at any solution that was particularly
> > satisfying.
> > 
> > The main constraints I've been trying to meet are:
> >- User-space should be informed of veth pairing for both peers.
> >- RTM_NEWLINK messages should not refer to interfaces that haven't
> >  been announced to user-space via previous RTM_NEWLINK messages.
> >- The first (and only the first) RTM_NEWLINK message for a given
> >  interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
> >  messages should have ifi_changes set to reflect the flags that
> >  have changed.
> > 
> > This is the closest I've come to a satisfactory solution, it does meet
> > the above constraints but still seems a little unnatural to me:
> The patch looks good to me. Can you submit it formally?
> 

Will post in a bit, thanks!

  Lance

Re: [net v3] veth: advertise peer link once both links are tied together

2016-06-10 Thread Nicolas Dichtel 
Le 08/06/2016 22:30, Lance Richardson a écrit :
[snip]
> I've been pondering how to fix this very problem off-and-on for a few months
> now, without having arrived at any solution that was particularly satisfying.
> 
> The main constraints I've been trying to meet are:
>- User-space should be informed of veth pairing for both peers.
>- RTM_NEWLINK messages should not refer to interfaces that haven't
>  been announced to user-space via previous RTM_NEWLINK messages.
>- The first (and only the first) RTM_NEWLINK message for a given
>  interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
>  messages should have ifi_changes set to reflect the flags that
>  have changed.
> 
> This is the closest I've come to a satisfactory solution, it does meet
> the above constraints but still seems a little unnatural to me:
The patch looks good to me. Can you submit it formally?

Re: [net v3] veth: advertise peer link once both links are tied together

2016-06-08 Thread Lance Richardson 
- Original Message -
> From: "Nicolas Dichtel" <nicolas.dich...@6wind.com>
> To: "Vincent Bernat" <vinc...@bernat.im>
> Cc: "David S. Miller" <da...@davemloft.net>, "Vijay Pandurangan" 
> <vij...@vijayp.ca>, "Paolo Abeni"
> <pab...@redhat.com>, netdev@vger.kernel.org
> Sent: Tuesday, May 31, 2016 5:17:20 AM
> Subject: Re: [net v3] veth: advertise peer link once both links are tied 
> together
> 
> Le 31/05/2016 08:29, Vincent Bernat a écrit :
> >  ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel <nicolas.dich...@6wind.com> :
> > 
> >>>> +
> >>>> +rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
> >>>
> >>> Maybe ~0U would be better than hijacking IFF_SLAVE?
> >> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change
> >> field
> >> not an attribute number.
> > 
> > There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
> > update the patch nonetheless.
> Sorry, I read it too quickly, IFF_SLAVE is a flag, not an attribute.
> But this field indicates to the userland which flags has changed and this
> flag
> does not change here ;-)
> 

I've been pondering how to fix this very problem off-and-on for a few months
now, without having arrived at any solution that was particularly satisfying.

The main constraints I've been trying to meet are:
   - User-space should be informed of veth pairing for both peers.
   - RTM_NEWLINK messages should not refer to interfaces that haven't
 been announced to user-space via previous RTM_NEWLINK messages.
   - The first (and only the first) RTM_NEWLINK message for a given
 interface should have ifi_changes set to ~0U, subsequent RTM_NEWLINK
 messages should have ifi_changes set to reflect the flags that
 have changed.

This is the closest I've come to a satisfactory solution, it does meet
the above constraints but still seems a little unnatural to me:

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
@@ -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..28ee417 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,13 @@ 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);

Sample output:

# ip link add dev vm1 type veth peer name vm2
5: vm2@NONE: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN 
link/ether 36:a7:b4:dc:27:3b brd ff:ff:ff:ff:ff:ff
6: vm1@vm2: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN 
link/ether 72:78:30:f6:e6:3a brd ff:ff:ff:ff:ff:ff
5: vm2@vm1: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noop state DOWN 
link/ether 36:a7:b4:dc:27:3b brd ff:ff:ff:ff:ff:ff

Re: [net v3] veth: advertise peer link once both links are tied together

2016-05-31 Thread Nicolas Dichtel 
Le 31/05/2016 08:29, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel  :
> 
 +
 +  rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
>>>
>>> Maybe ~0U would be better than hijacking IFF_SLAVE?
>> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change 
>> field
>> not an attribute number.
> 
> There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
> update the patch nonetheless.
Sorry, I read it too quickly, IFF_SLAVE is a flag, not an attribute.
But this field indicates to the userland which flags has changed and this flag
does not change here ;-)

Re: [net v3] veth: advertise peer link once both links are tied together

2016-05-31 Thread Vincent Bernat 
 ❦ 30 mai 2016 18:27 CEST, Nicolas Dichtel  :

>>> +
>>> +   rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
>> 
>> Maybe ~0U would be better than hijacking IFF_SLAVE?
> IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change field
> not an attribute number.

There are some use of IFF_SLAVE (in bonding/bond_main.c). But, I'll
update the patch nonetheless.
-- 
Use free-form input when possible.
- The Elements of Programming Style (Kernighan & Plauger)

Re: [net v3] veth: advertise peer link once both links are tied together

2016-05-30 Thread Nicolas Dichtel 
Le 30/05/2016 18:01, Vincent Bernat a écrit :
>  ❦ 30 mai 2016 17:58 CEST, Vincent Bernat  :
> 
>> +
>> +rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
> 
> Maybe ~0U would be better than hijacking IFF_SLAVE?
IFF_SLAVE is wrong. It's a flag here, that will be put in the ifi_change field
not an attribute number.

Re: [net v3] veth: advertise peer link once both links are tied together

2016-05-30 Thread Vincent Bernat 
 ❦ 30 mai 2016 17:58 CEST, Vincent Bernat  :

> +
> + rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);

Maybe ~0U would be better than hijacking IFF_SLAVE?
-- 
Anyone who has had a bull by the tail knows five or six more things
than someone who hasn't.
-- Mark Twain

[net v3] veth: advertise peer link once both links are tied together

2016-05-30 Thread Vincent Bernat 
When the peer link is created, its "iflink" information is not
advertised through Netlink. Once created, the local device is advertised
with this information but if a user is maintaining a cache from all
updates, it will still miss the iflink for the peer link:

2: veth0@NONE:  mtu 1500 qdisc noop state DOWN group 
default
link/ether ae:0e:08:af:fb:a0 brd ff:ff:ff:ff:ff:ff
3: veth1@veth0:  mtu 1500 qdisc noop state DOWN 
group default
link/ether 3a:31:f1:36:2e:e5 brd ff:ff:ff:ff:ff:ff

With this patch, we advertise again the peer link to let any user pick
the appropriate iflink information:

3: veth0@NONE:  mtu 1500 qdisc noop state DOWN group 
default
link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
3: veth0@veth1:  mtu 1500 qdisc noop state DOWN group 
default
link/ether fa:ba:12:26:99:00 brd ff:ff:ff:ff:ff:ff
4: veth1@veth0:  mtu 1500 qdisc noop state DOWN 
group default
link/ether ea:e4:e2:26:3c:87 brd ff:ff:ff:ff:ff:ff

Signed-off-by: Vincent Bernat 
---
v3:
 - send an additional netlink messages once the peer link is tied to
   avoid any chicken/egg problem

v2:
 - ensure the device is unregistered in case of link configuration failure

 drivers/net/veth.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index f37a6e61d4ad..2376d17b8f53 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -466,6 +466,8 @@ static int veth_newlink(struct net *src_net, struct 
net_device *dev,
 
priv = netdev_priv(peer);
rcu_assign_pointer(priv->peer, dev);
+
+   rtmsg_ifinfo(RTM_NEWLINK, peer, IFF_SLAVE, GFP_KERNEL);
return 0;
 
 err_register_dev:
-- 
2.8.1