Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
On Wed, 09 Aug 2017 11:03:05 +0200 Vitaly Kuznetsov wrote: > Stephen Hemminger writes: > > > The following would allow udev a chance at the device. > > > > This would of course work but the approach is a bit hackish and can > still fail on a loaded system. Raising the pause interval would be an > option, but again, probably not the best one. > > Let me try to send an RFC removing the check it dev_change_name() and if > it turns out that it can't be removed we can go back to your patch. But > in case it can we can leave without it. > > Thanks, I don't want to require change of semantics of core networking code for one driver. Changing name of up device has been blocked for so long that allowing it might break existing userspace apps. It might be ok in the future, but netvsc needs to work without that change.
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
Stephen Hemminger writes: > The following would allow udev a chance at the device. > This would of course work but the approach is a bit hackish and can still fail on a loaded system. Raising the pause interval would be an option, but again, probably not the best one. Let me try to send an RFC removing the check it dev_change_name() and if it turns out that it can't be removed we can go back to your patch. But in case it can we can leave without it. Thanks, > From 37fb240a6107834c3dd3f57caede9d73b807f414 Mon Sep 17 00:00:00 2001 > From: Stephen Hemminger > Date: Tue, 8 Aug 2017 08:39:24 -0700 > Subject: [PATCH] netvsc: delay setup of VF device > > When VF device is discovered, delay bring it automatically up in > order to allow userspace to some simple changes (like renaming). > > Signed-off-by: Stephen Hemminger > --- > drivers/net/hyperv/hyperv_net.h | 2 +- > drivers/net/hyperv/netvsc_drv.c | 11 ++- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index d1ea99a12cf2..f620c90307ed 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -723,7 +723,7 @@ struct net_device_context { > /* State to manage the associated VF interface. */ > struct net_device __rcu *vf_netdev; > struct netvsc_vf_pcpu_stats __percpu *vf_stats; > - struct work_struct vf_takeover; > + struct delayed_work vf_takeover; > > /* 1: allocated, serial number is valid. 0: not allocated */ > u32 vf_alloc; > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 7ebf0e10e62b..1dff160368a3 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -47,6 +47,7 @@ > > #define RING_SIZE_MIN 64 > #define LINKCHANGE_INT (2 * HZ) > +#define VF_TAKEOVER_INT (HZ / 10) > > static int ring_size = 128; > module_param(ring_size, int, S_IRUGO); > @@ -1570,7 +1571,7 @@ static int netvsc_vf_join(struct net_device *vf_netdev, > /* set slave flag before open to prevent IPv6 addrconf */ > vf_netdev->flags |= IFF_SLAVE; > > - schedule_work(&ndev_ctx->vf_takeover); > + schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT); > > netdev_info(vf_netdev, "joined to %s\n", ndev->name); > return 0; > @@ -1608,12 +1609,12 @@ static void __netvsc_vf_setup(struct net_device *ndev, > static void netvsc_vf_setup(struct work_struct *w) > { > struct net_device_context *ndev_ctx > - = container_of(w, struct net_device_context, vf_takeover); > + = container_of(w, struct net_device_context, vf_takeover.work); > struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx); > struct net_device *vf_netdev; > > if (!rtnl_trylock()) { > - schedule_work(w); > + schedule_delayed_work(&ndev_ctx->vf_takeover, 0); > return; > } > > @@ -1717,7 +1718,7 @@ static int netvsc_unregister_vf(struct net_device > *vf_netdev) > return NOTIFY_DONE; > > net_device_ctx = netdev_priv(ndev); > - cancel_work_sync(&net_device_ctx->vf_takeover); > + cancel_delayed_work_sync(&net_device_ctx->vf_takeover); > > netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name); > > @@ -1759,7 +1760,7 @@ static int netvsc_probe(struct hv_device *dev, > > spin_lock_init(&net_device_ctx->lock); > INIT_LIST_HEAD(&net_device_ctx->reconfig_events); > - INIT_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup); > + INIT_DELAYED_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup); > > net_device_ctx->vf_stats > = netdev_alloc_pcpu_stats(struct netvsc_vf_pcpu_stats); -- Vitaly
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
The following would allow udev a chance at the device. From 37fb240a6107834c3dd3f57caede9d73b807f414 Mon Sep 17 00:00:00 2001 From: Stephen Hemminger Date: Tue, 8 Aug 2017 08:39:24 -0700 Subject: [PATCH] netvsc: delay setup of VF device When VF device is discovered, delay bring it automatically up in order to allow userspace to some simple changes (like renaming). Signed-off-by: Stephen Hemminger --- drivers/net/hyperv/hyperv_net.h | 2 +- drivers/net/hyperv/netvsc_drv.c | 11 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index d1ea99a12cf2..f620c90307ed 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -723,7 +723,7 @@ struct net_device_context { /* State to manage the associated VF interface. */ struct net_device __rcu *vf_netdev; struct netvsc_vf_pcpu_stats __percpu *vf_stats; - struct work_struct vf_takeover; + struct delayed_work vf_takeover; /* 1: allocated, serial number is valid. 0: not allocated */ u32 vf_alloc; diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 7ebf0e10e62b..1dff160368a3 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -47,6 +47,7 @@ #define RING_SIZE_MIN 64 #define LINKCHANGE_INT (2 * HZ) +#define VF_TAKEOVER_INT (HZ / 10) static int ring_size = 128; module_param(ring_size, int, S_IRUGO); @@ -1570,7 +1571,7 @@ static int netvsc_vf_join(struct net_device *vf_netdev, /* set slave flag before open to prevent IPv6 addrconf */ vf_netdev->flags |= IFF_SLAVE; - schedule_work(&ndev_ctx->vf_takeover); + schedule_delayed_work(&ndev_ctx->vf_takeover, VF_TAKEOVER_INT); netdev_info(vf_netdev, "joined to %s\n", ndev->name); return 0; @@ -1608,12 +1609,12 @@ static void __netvsc_vf_setup(struct net_device *ndev, static void netvsc_vf_setup(struct work_struct *w) { struct net_device_context *ndev_ctx - = container_of(w, struct net_device_context, vf_takeover); + = container_of(w, struct net_device_context, vf_takeover.work); struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx); struct net_device *vf_netdev; if (!rtnl_trylock()) { - schedule_work(w); + schedule_delayed_work(&ndev_ctx->vf_takeover, 0); return; } @@ -1717,7 +1718,7 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev) return NOTIFY_DONE; net_device_ctx = netdev_priv(ndev); - cancel_work_sync(&net_device_ctx->vf_takeover); + cancel_delayed_work_sync(&net_device_ctx->vf_takeover); netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name); @@ -1759,7 +1760,7 @@ static int netvsc_probe(struct hv_device *dev, spin_lock_init(&net_device_ctx->lock); INIT_LIST_HEAD(&net_device_ctx->reconfig_events); - INIT_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup); + INIT_DELAYED_WORK(&net_device_ctx->vf_takeover, netvsc_vf_setup); net_device_ctx->vf_stats = netdev_alloc_pcpu_stats(struct netvsc_vf_pcpu_stats); -- 2.11.0
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
Stephen Hemminger writes: > On Tue, 08 Aug 2017 17:24:03 +0200 > Vitaly Kuznetsov wrote: > >> Stephen Hemminger writes: >> >> > On Tue, 08 Aug 2017 16:03:56 +0200 >> > Vitaly Kuznetsov wrote: >> > >> >> Stephen Hemminger writes: >> >> >> >> > Previous fix was incomplete. >> >> > >> >> >> >> Not really related to this patch series (which btw fixes my issue, >> >> thanks!), but I found one addition issue. Systemd fails to rename VF >> >> interface: >> >> >> >> kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 >> >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF >> >> registering: eth2 >> >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path >> >> switched to VF: eth2 >> >> kernel: mlx4_en: eth2: Link Up >> >> NetworkManager[750]: [1502200557.0821] device (eth2): link >> >> connected >> >> NetworkManager[750]: [1502200557.1004] manager: (eth2): new >> >> Ethernet device (/org/freedesktop/NetworkManager/Devices/6) >> >> systemd-udevd[6942]: Error changing net interface name 'eth2' to >> >> 'enP2p0s2': Device or resource busy >> >> systemd-udevd[6942]: could not rename interface '6' from 'eth2' to >> >> 'enP2p0s2': Device or resource busy >> >> >> >> With some debug added I figured out what's wrong: __netvsc_vf_setup() >> >> does dev_open() which sets IFF_UP flag on the interface. When systemd >> >> tries to rename the interface we get into dev_change_name() and this >> >> fails with -EBUSY when (dev->flags & IFF_UP). >> >> >> >> The issue is of less importance as we're not supposed to configure VF >> >> interface now. However, we may still want to get a stable name for it. >> >> >> >> Any idea how this can be fixed? >> > >> > The problem is Network Manager should ignore the VF device. I don't run NM >> > on these >> > interfaces because it causes more issues than it helps (dueling userspace). >> > >> > The driver needs to have slave track the master interface. Relying on >> > userspace >> > to bring interface up leads to all the issues the bonding script had. >> > >> > One option would be to delay the work of bringing up the slave device to >> > allow >> > small window for renaming to run. >> >> Actually, I tried removing 'if (dev->flags & IFF_UP)' check from >> dev_change_name() and everything seems to work fine. The history of this >> code predates git so I have no idea why it's forbiden to change names of >> IFF_UP interfaces... I can send an RFC removing the check to figure out >> the truth :-) > > That only works because NM by default brings everything up. > Many users don't use NM on servers. I'm not sure NetworkManager is related here: renaming is done by systemd/udev according to the "predictable interface names" scheme: https://www.freedesktop.org/wiki/Software/systemd/PredictableNetworkInterfaceNames/ Basically, systemd/udev tries to rename all new interfaces in the system according to this scheme. And systemd/udev is ubiquitous nowdays. I tried disabling NetworkManager in my VM and the behavior is not any different: kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF registering: eth2 kernel: mlx4_en: eth2: Link Up kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path switched to VF: eth2 systemd-udevd[1785]: Error changing net interface name 'eth2' to 'enP2p0s2': Device or resource busy systemd-udevd[1785]: could not rename interface '5' from 'eth2' to 'enP2p0s2': Device or resource busy The 'if (dev->flags & IFF_UP)' check in dev_change_name() function I mentioned prevents renaming interfaces which are already up but I don't know an obvious reason for it to exist. -- Vitaly
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
On Tue, 08 Aug 2017 17:24:03 +0200 Vitaly Kuznetsov wrote: > Stephen Hemminger writes: > > > On Tue, 08 Aug 2017 16:03:56 +0200 > > Vitaly Kuznetsov wrote: > > > >> Stephen Hemminger writes: > >> > >> > Previous fix was incomplete. > >> > > >> > >> Not really related to this patch series (which btw fixes my issue, > >> thanks!), but I found one addition issue. Systemd fails to rename VF > >> interface: > >> > >> kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 > >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF > >> registering: eth2 > >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path > >> switched to VF: eth2 > >> kernel: mlx4_en: eth2: Link Up > >> NetworkManager[750]: [1502200557.0821] device (eth2): link > >> connected > >> NetworkManager[750]: [1502200557.1004] manager: (eth2): new > >> Ethernet device (/org/freedesktop/NetworkManager/Devices/6) > >> systemd-udevd[6942]: Error changing net interface name 'eth2' to > >> 'enP2p0s2': Device or resource busy > >> systemd-udevd[6942]: could not rename interface '6' from 'eth2' to > >> 'enP2p0s2': Device or resource busy > >> > >> With some debug added I figured out what's wrong: __netvsc_vf_setup() > >> does dev_open() which sets IFF_UP flag on the interface. When systemd > >> tries to rename the interface we get into dev_change_name() and this > >> fails with -EBUSY when (dev->flags & IFF_UP). > >> > >> The issue is of less importance as we're not supposed to configure VF > >> interface now. However, we may still want to get a stable name for it. > >> > >> Any idea how this can be fixed? > > > > The problem is Network Manager should ignore the VF device. I don't run NM > > on these > > interfaces because it causes more issues than it helps (dueling userspace). > > > > The driver needs to have slave track the master interface. Relying on > > userspace > > to bring interface up leads to all the issues the bonding script had. > > > > One option would be to delay the work of bringing up the slave device to > > allow > > small window for renaming to run. > > Actually, I tried removing 'if (dev->flags & IFF_UP)' check from > dev_change_name() and everything seems to work fine. The history of this > code predates git so I have no idea why it's forbiden to change names of > IFF_UP interfaces... I can send an RFC removing the check to figure out > the truth :-) That only works because NM by default brings everything up. Many users don't use NM on servers.
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
Stephen Hemminger writes: > On Tue, 08 Aug 2017 16:03:56 +0200 > Vitaly Kuznetsov wrote: > >> Stephen Hemminger writes: >> >> > Previous fix was incomplete. >> > >> >> Not really related to this patch series (which btw fixes my issue, >> thanks!), but I found one addition issue. Systemd fails to rename VF >> interface: >> >> kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF >> registering: eth2 >> kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path >> switched to VF: eth2 >> kernel: mlx4_en: eth2: Link Up >> NetworkManager[750]: [1502200557.0821] device (eth2): link connected >> NetworkManager[750]: [1502200557.1004] manager: (eth2): new >> Ethernet device (/org/freedesktop/NetworkManager/Devices/6) >> systemd-udevd[6942]: Error changing net interface name 'eth2' to >> 'enP2p0s2': Device or resource busy >> systemd-udevd[6942]: could not rename interface '6' from 'eth2' to >> 'enP2p0s2': Device or resource busy >> >> With some debug added I figured out what's wrong: __netvsc_vf_setup() >> does dev_open() which sets IFF_UP flag on the interface. When systemd >> tries to rename the interface we get into dev_change_name() and this >> fails with -EBUSY when (dev->flags & IFF_UP). >> >> The issue is of less importance as we're not supposed to configure VF >> interface now. However, we may still want to get a stable name for it. >> >> Any idea how this can be fixed? > > The problem is Network Manager should ignore the VF device. I don't run NM on > these > interfaces because it causes more issues than it helps (dueling userspace). > > The driver needs to have slave track the master interface. Relying on > userspace > to bring interface up leads to all the issues the bonding script had. > > One option would be to delay the work of bringing up the slave device to allow > small window for renaming to run. Actually, I tried removing 'if (dev->flags & IFF_UP)' check from dev_change_name() and everything seems to work fine. The history of this code predates git so I have no idea why it's forbiden to change names of IFF_UP interfaces... I can send an RFC removing the check to figure out the truth :-) -- Vitaly
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
On Tue, 08 Aug 2017 16:03:56 +0200 Vitaly Kuznetsov wrote: > Stephen Hemminger writes: > > > Previous fix was incomplete. > > > > Not really related to this patch series (which btw fixes my issue, > thanks!), but I found one addition issue. Systemd fails to rename VF > interface: > > kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 > kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF registering: > eth2 > kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path > switched to VF: eth2 > kernel: mlx4_en: eth2: Link Up > NetworkManager[750]: [1502200557.0821] device (eth2): link connected > NetworkManager[750]: [1502200557.1004] manager: (eth2): new Ethernet > device (/org/freedesktop/NetworkManager/Devices/6) > systemd-udevd[6942]: Error changing net interface name 'eth2' to 'enP2p0s2': > Device or resource busy > systemd-udevd[6942]: could not rename interface '6' from 'eth2' to > 'enP2p0s2': Device or resource busy > > With some debug added I figured out what's wrong: __netvsc_vf_setup() > does dev_open() which sets IFF_UP flag on the interface. When systemd > tries to rename the interface we get into dev_change_name() and this > fails with -EBUSY when (dev->flags & IFF_UP). > > The issue is of less importance as we're not supposed to configure VF > interface now. However, we may still want to get a stable name for it. > > Any idea how this can be fixed? The problem is Network Manager should ignore the VF device. I don't run NM on these interfaces because it causes more issues than it helps (dueling userspace). The driver needs to have slave track the master interface. Relying on userspace to bring interface up leads to all the issues the bonding script had. One option would be to delay the work of bringing up the slave device to allow small window for renaming to run.
Re: [PATCH net-next 0/1] netvsc: another VF datapath fix
Stephen Hemminger writes: > Previous fix was incomplete. > Not really related to this patch series (which btw fixes my issue, thanks!), but I found one addition issue. Systemd fails to rename VF interface: kernel: mlx4_core 0002:00:02.0 eth2: joined to eth1 kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: VF registering: eth2 kernel: hv_netvsc 33b7a6f9-6736-451f-8fce-b382eaa50bee eth1: Data path switched to VF: eth2 kernel: mlx4_en: eth2: Link Up NetworkManager[750]: [1502200557.0821] device (eth2): link connected NetworkManager[750]: [1502200557.1004] manager: (eth2): new Ethernet device (/org/freedesktop/NetworkManager/Devices/6) systemd-udevd[6942]: Error changing net interface name 'eth2' to 'enP2p0s2': Device or resource busy systemd-udevd[6942]: could not rename interface '6' from 'eth2' to 'enP2p0s2': Device or resource busy With some debug added I figured out what's wrong: __netvsc_vf_setup() does dev_open() which sets IFF_UP flag on the interface. When systemd tries to rename the interface we get into dev_change_name() and this fails with -EBUSY when (dev->flags & IFF_UP). The issue is of less importance as we're not supposed to configure VF interface now. However, we may still want to get a stable name for it. Any idea how this can be fixed? -- Vitaly
[PATCH net-next 0/1] netvsc: another VF datapath fix
Previous fix was incomplete. Stephen Hemminger (1): netvsc: make sure and unregister datapath drivers/net/hyperv/hyperv_net.h | 3 -- drivers/net/hyperv/netvsc.c | 2 -- drivers/net/hyperv/netvsc_drv.c | 71 - 3 files changed, 28 insertions(+), 48 deletions(-) -- 2.11.0