diff mbox

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

Message ID 694175272.49661350.1465417849878.JavaMail.zimbra@redhat.com
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Lance Richardson June 8, 2016, 8:30 p.m. UTC
----- Original Message -----
> From: "Nicolas Dichtel" <nicolas.dichtel@6wind.com>
> To: "Vincent Bernat" <vincent@bernat.im>
> Cc: "David S. Miller" <davem@davemloft.net>, "Vijay Pandurangan" <vijayp@vijayp.ca>, "Paolo Abeni"
> <pabeni@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.dichtel@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:


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

Comments

Nicolas Dichtel June 10, 2016, 1:15 p.m. UTC | #1
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?
Lance Richardson June 10, 2016, 1:20 p.m. UTC | #2
----- Original Message -----
> From: "Nicolas Dichtel" <nicolas.dichtel@6wind.com>
> To: "Lance Richardson" <lrichard@redhat.com>, "Vincent Bernat" <vincent@bernat.im>
> Cc: "David S. Miller" <davem@davemloft.net>, "Vijay Pandurangan" <vijayp@vijayp.ca>, "Paolo Abeni"
> <pabeni@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
diff mbox

Patch

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);