Re: [net v3] veth: advertise peer link once both links are tied together
- 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
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
- 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
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
❦ 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
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
❦ 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