[PATCH net-next 0/1] netvsc: another VF datapath fix

[PATCH net-next 0/1] netvsc: another VF datapath fix

[Stephen Hemminger]

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

[PATCH net-next 1/1] netvsc: make sure and unregister datapath

[Stephen Hemminger]

Go back to switching datapath directly in the notifier callback.
Otherwise datapath might not get switched on unregister.

No need for calling the NOTIFY_PEERS notifier since that is only for
a gratitious ARP/ND packet; but that is not required with Hyper-V
because both VF and synthetic NIC have the same MAC address.

Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 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(-)

diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index c701b059c5ac..d1ea99a12cf2 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -724,14 +724,11 @@ struct net_device_context {
 	struct net_device __rcu *vf_netdev;
 	struct netvsc_vf_pcpu_stats __percpu *vf_stats;
 	struct work_struct vf_takeover;
-	struct work_struct vf_notify;
 
 	/* 1: allocated, serial number is valid. 0: not allocated */
 	u32 vf_alloc;
 	/* Serial number of the VF to team with */
 	u32 vf_serial;
-
-	bool datapath;	/* 0 - synthetic, 1 - VF nic */
 };
 
 /* Per channel data */
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 9598220b3bcc..208f03aa83de 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -60,8 +60,6 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf)
 			       sizeof(struct nvsp_message),
 			       (unsigned long)init_pkt,
 			       VM_PKT_DATA_INBAND, 0);
-
-	net_device_ctx->datapath = vf;
 }
 
 static struct netvsc_device *alloc_net_device(void)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index e75c0f852a63..eb0023f55fe1 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1649,55 +1649,35 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
 	return NOTIFY_OK;
 }
 
-/* Change datapath */
-static void netvsc_vf_update(struct work_struct *w)
+static int netvsc_vf_up(struct net_device *vf_netdev)
 {
-	struct net_device_context *ndev_ctx
-		= container_of(w, struct net_device_context, vf_notify);
-	struct net_device *ndev = hv_get_drvdata(ndev_ctx->device_ctx);
+	struct net_device_context *net_device_ctx;
 	struct netvsc_device *netvsc_dev;
-	struct net_device *vf_netdev;
-	bool vf_is_up;
-
-	if (!rtnl_trylock()) {
-		schedule_work(w);
-		return;
-	}
+	struct net_device *ndev;
 
-	vf_netdev = rtnl_dereference(ndev_ctx->vf_netdev);
-	if (!vf_netdev)
-		goto unlock;
+	ndev = get_netvsc_byref(vf_netdev);
+	if (!ndev)
+		return NOTIFY_DONE;
 
-	netvsc_dev = rtnl_dereference(ndev_ctx->nvdev);
+	net_device_ctx = netdev_priv(ndev);
+	netvsc_dev = rtnl_dereference(net_device_ctx->nvdev);
 	if (!netvsc_dev)
-		goto unlock;
-
-	vf_is_up = netif_running(vf_netdev);
-	if (vf_is_up != ndev_ctx->datapath) {
-		if (vf_is_up) {
-			netdev_info(ndev, "VF up: %s\n", vf_netdev->name);
-			rndis_filter_open(netvsc_dev);
-			netvsc_switch_datapath(ndev, true);
-			netdev_info(ndev, "Data path switched to VF: %s\n",
-				    vf_netdev->name);
-		} else {
-			netdev_info(ndev, "VF down: %s\n", vf_netdev->name);
-			netvsc_switch_datapath(ndev, false);
-			rndis_filter_close(netvsc_dev);
-			netdev_info(ndev, "Data path switched from VF: %s\n",
-				    vf_netdev->name);
-		}
+		return NOTIFY_DONE;
 
-		/* Now notify peers through VF device. */
-		call_netdevice_notifiers(NETDEV_NOTIFY_PEERS, ndev);
-	}
-unlock:
-	rtnl_unlock();
+	/* Bump refcount when datapath is acvive - Why? */
+	rndis_filter_open(netvsc_dev);
+
+	/* notify the host to switch the data path. */
+	netvsc_switch_datapath(ndev, true);
+	netdev_info(ndev, "Data path switched to VF: %s\n", vf_netdev->name);
+
+	return NOTIFY_OK;
 }
 
-static int netvsc_vf_notify(struct net_device *vf_netdev)
+static int netvsc_vf_down(struct net_device *vf_netdev)
 {
 	struct net_device_context *net_device_ctx;
+	struct netvsc_device *netvsc_dev;
 	struct net_device *ndev;
 
 	ndev = get_netvsc_byref(vf_netdev);
@@ -1705,7 +1685,13 @@ static int netvsc_vf_notify(struct net_device *vf_netdev)
 		return NOTIFY_DONE;
 
 	net_device_ctx = netdev_priv(ndev);
-	schedule_work(&net_device_ctx->vf_notify);
+	netvsc_dev = rtnl_dereference(net_device_ctx->nvdev);
+	if (!netvsc_dev)
+		return NOTIFY_DONE;
+
+	netvsc_switch_datapath(ndev, false);
+	netdev_info(ndev, "Data path switched from VF: %s\n", vf_netdev->name);
+	rndis_filter_close(netvsc_dev);
 
 	return NOTIFY_OK;
 }
@@ -1721,7 +1707,6 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
 
 	net_device_ctx = netdev_priv(ndev);
 	cancel_work_sync(&net_device_ctx->vf_takeover);
-	cancel_work_sync(&net_device_ctx->vf_notify);
 
 	netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
 
@@ -1764,7 +1749,6 @@ 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_WORK(&net_device_ctx->vf_notify, netvsc_vf_update);
 
 	net_device_ctx->vf_stats
 		= netdev_alloc_pcpu_stats(struct netvsc_vf_pcpu_stats);
@@ -1915,8 +1899,9 @@ static int netvsc_netdev_event(struct notifier_block *this,
 	case NETDEV_UNREGISTER:
 		return netvsc_unregister_vf(event_dev);
 	case NETDEV_UP:
+		return netvsc_vf_up(event_dev);
 	case NETDEV_DOWN:
-		return netvsc_vf_notify(event_dev);
+		return netvsc_vf_down(event_dev);
 	default:
 		return NOTIFY_DONE;
 	}
-- 
2.11.0

Re: [PATCH net-next 1/1] netvsc: make sure and unregister datapath

[David Miller]

From: Stephen Hemminger <stephen@networkplumber.org>
Date: Mon,  7 Aug 2017 11:30:00 -0700

> Go back to switching datapath directly in the notifier callback.
> Otherwise datapath might not get switched on unregister.
> 
> No need for calling the NOTIFY_PEERS notifier since that is only for
> a gratitious ARP/ND packet; but that is not required with Hyper-V
> because both VF and synthetic NIC have the same MAC address.
> 
> Reported-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Fixes: 0c195567a8f6 ("netvsc: transparent VF management")
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>

Applied, thanks Stephen.

Re: [PATCH net-next 0/1] netvsc: another VF datapath fix

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> 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]: <info>  [1502200557.0821] device (eth2): link connected
 NetworkManager[750]: <info>  [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

Re: [PATCH net-next 0/1] netvsc: another VF datapath fix

[Stephen Hemminger]

On Tue, 08 Aug 2017 16:03:56 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> 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]: <info>  [1502200557.0821] device (eth2): link connected
>  NetworkManager[750]: <info>  [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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 08 Aug 2017 16:03:56 +0200
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> 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]: <info>  [1502200557.0821] device (eth2): link connected
>>  NetworkManager[750]: <info>  [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

[Stephen Hemminger]

On Tue, 08 Aug 2017 17:24:03 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> writes:
> 
> > On Tue, 08 Aug 2017 16:03:56 +0200
> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >  
> >> Stephen Hemminger <stephen@networkplumber.org> 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]: <info>  [1502200557.0821] device (eth2): link connected
> >>  NetworkManager[750]: <info>  [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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> writes:

> On Tue, 08 Aug 2017 17:24:03 +0200
> Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
>> Stephen Hemminger <stephen@networkplumber.org> writes:
>> 
>> > On Tue, 08 Aug 2017 16:03:56 +0200
>> > Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> >  
>> >> Stephen Hemminger <stephen@networkplumber.org> 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]: <info>  [1502200557.0821] device (eth2): link connected
>> >>  NetworkManager[750]: <info>  [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

[Stephen Hemminger]

The following would allow udev a chance at the device.



From 37fb240a6107834c3dd3f57caede9d73b807f414 Mon Sep 17 00:00:00 2001
From: Stephen Hemminger <sthemmin@microsoft.com>
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 <sthemmin@microsoft.com>
---
 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

[Vitaly Kuznetsov]

Stephen Hemminger <stephen@networkplumber.org> 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 <sthemmin@microsoft.com>
> 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 <sthemmin@microsoft.com>
> ---
>  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

[Stephen Hemminger]

On Wed, 09 Aug 2017 11:03:05 +0200
Vitaly Kuznetsov <vkuznets@redhat.com> wrote:

> Stephen Hemminger <stephen@networkplumber.org> 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.