Skip to contentSkip to conversation

hv_netvsc: sub channel initialization fixes

PATCH Applied Stephen Hemminger 2 participants 5 messages Sep 6, 2017 – Sep 11, 2017

Subsystems (3)
Networking Network drivers Hyper-V/Azure core and drivers
lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

v2 cover hv_netvsc: sub channel initialization fixes

One serious deadlock, and one minor optimization.

Stephen Hemminger (2):
hv_netvsc: fix deadlock on hotplug
hv_netvsc: avoid unnecessary wakeups on subchannel creation

 drivers/net/hyperv/hyperv_net.h   |   3 +
 drivers/net/hyperv/netvsc.c       |   3 +
 drivers/net/hyperv/netvsc_drv.c   |  11 +---
 drivers/net/hyperv/rndis_filter.c | 126 ++++++++++++++++++++++++++------------
 4 files changed, 96 insertions(+), 47 deletions(-)
Signature · 1 line
2.11.0
author
#1 lore raw

PATCH v2 1/2 hv_netvsc: fix deadlock on hotplug

When a virtual device is added dynamically (via host console), then the vmbus sends an offer message for the primary channel. The processing of this message for networking causes the network device to then initialize the sub channels.

The problem is that setting up the sub channels needs to wait until the subsequent subchannel offers have been processed. These offers come in on the same ring buffer and work queue as where the primary offer is being processed; leading to a deadlock.

This did not happen in older kernels, because the sub channel waiting logic was broken (it wasn't really waiting).

The solution is to do the sub channel setup in its own work queue context that is scheduled by the primary channel setup; and then happens later.

Fixes 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by Dexuan Cui <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>

---
v2 - fix module removal race with new work queue

 drivers/net/hyperv/hyperv_net.h   |   3 +
 drivers/net/hyperv/netvsc.c       |   3 +
 drivers/net/hyperv/netvsc_drv.c   |  11 +---
 drivers/net/hyperv/rndis_filter.c | 122 ++++++++++++++++++++++++++------------
 4 files changed, 94 insertions(+), 45 deletions(-)
drivers/net/hyperv/hyperv_net.h +3 -0
index ec546da86683..d98cdfb1536b 100644
@@ -204,6 +204,8 @@ int netvsc_recv_callback(struct net_device *net,
 			 const struct ndis_pkt_8021q_info *vlan);
 void netvsc_channel_cb(void *context);
 int netvsc_poll(struct napi_struct *napi, int budget);
+
+void rndis_set_subchannel(struct work_struct *w);
 bool rndis_filter_opened(const struct netvsc_device *nvdev);
 int rndis_filter_open(struct netvsc_device *nvdev);
 int rndis_filter_close(struct netvsc_device *nvdev);

@@ -782,6 +784,7 @@ struct netvsc_device {
 	u32 num_chn;
 
 	atomic_t open_chn;
+	struct work_struct subchan_work;
 	wait_queue_head_t subchan_open;
 
 	struct rndis_device *extension;
drivers/net/hyperv/netvsc.c +3 -0
index 0062b802676f..a5511b7326af 100644
@@ -81,6 +81,7 @@ static struct netvsc_device *alloc_net_device(void)
 
 	init_completion(&net_device->channel_init_wait);
 	init_waitqueue_head(&net_device->subchan_open);
+	INIT_WORK(&net_device->subchan_work, rndis_set_subchannel);
 
 	return net_device;
 }

@@ -557,6 +558,8 @@ void netvsc_device_remove(struct hv_device *device)
 		= rtnl_dereference(net_device_ctx->nvdev);
 	int i;
 
+	cancel_work_sync(&net_device->subchan_work);
+
 	netvsc_disconnect_vsp(device);
 
 	RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
drivers/net/hyperv/netvsc_drv.c +3 -8
index 165ba4b3b423..c538a4f15f3b 100644
@@ -853,10 +853,7 @@ static int netvsc_set_channels(struct net_device *net,
 	rndis_filter_device_remove(dev, nvdev);
 
 	nvdev = rndis_filter_device_add(dev, &device_info);
-	if (!IS_ERR(nvdev)) {
-		netif_set_real_num_tx_queues(net, nvdev->num_chn);
-		netif_set_real_num_rx_queues(net, nvdev->num_chn);
-	} else {
+	if (IS_ERR(nvdev)) {
 		ret = PTR_ERR(nvdev);
 		device_info.num_chn = orig;
 		nvdev = rndis_filter_device_add(dev, &device_info);

@@ -1954,9 +1951,6 @@ static int netvsc_probe(struct hv_device *dev,
 		NETIF_F_HW_VLAN_CTAG_TX | NETIF_F_HW_VLAN_CTAG_RX;
 	net->vlan_features = net->features;
 
-	netif_set_real_num_tx_queues(net, nvdev->num_chn);
-	netif_set_real_num_rx_queues(net, nvdev->num_chn);
-
 	netdev_lockdep_set_classes(net);
 
 	/* MTU range: 68 - 1500 or 65521 */

@@ -2012,9 +2006,10 @@ static int netvsc_remove(struct hv_device *dev)
 	if (vf_netdev)
 		netvsc_unregister_vf(vf_netdev);
 
+	unregister_netdevice(net);
+
 	rndis_filter_device_remove(dev,
 				   rtnl_dereference(ndev_ctx->nvdev));
-	unregister_netdevice(net);
 	rtnl_unlock();
 
 	hv_set_drvdata(dev, NULL);
drivers/net/hyperv/rndis_filter.c +85 -37

169 lines not on this page · raw message

Signature · 1 line
2.11.0
maintainer Applied
#2 lore raw

Re: [PATCH v2 net-next 1/2] hv_netvsc: fix deadlock on hotplug

From: Stephen Hemminger <[email protected]>
Date: Wed, 6 Sep 2017 13:53:05 -0700

Quoted 17 lines

When a virtual device is added dynamically (via host console), then the vmbus sends an offer message for the primary channel. The processing of this message for networking causes the network device to then initialize the sub channels.

The problem is that setting up the sub channels needs to wait until the subsequent subchannel offers have been processed. These offers come in on the same ring buffer and work queue as where the primary offer is being processed; leading to a deadlock.

This did not happen in older kernels, because the sub channel waiting logic was broken (it wasn't really waiting).

The solution is to do the sub channel setup in its own work queue context that is scheduled by the primary channel setup; and then happens later.

Quoted 5 lines
Fixes 732e49850c5e ("netvsc: fix race on sub channel creation")
Reported-by Dexuan Cui <[email protected]>
Signed-off-by Stephen Hemminger <[email protected]>

---
v2 - fix module removal race with new work queue

Applied.

author
#3 lore raw

PATCH v2 2/2 hv_netvsc: avoid unnecessary wakeups on subchannel creation

In reply to Stephen Hemminger

Only need to wakeup the initiator after all sub-channels are opened.

Signed-off-by Stephen Hemminger <[email protected]>
---
 drivers/net/hyperv/rndis_filter.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/hyperv/rndis_filter.c +2 -2
index 731bc7cc6f43..065b204d8e17 100644
@@ -1048,8 +1048,8 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
 	else
 		netdev_notice(ndev, "sub channel open failed: %d\n", ret);
 
-	atomic_inc(&nvscdev->open_chn);
-	wake_up(&nvscdev->subchan_open);
+	if (atomic_inc_return(&nvscdev->open_chn) == nvscdev->num_chn)
+		wake_up(&nvscdev->subchan_open);
 }
 
 /* Open sub-channels after completing the handling of the device probe.
Signature · 1 line
2.11.0