* [RFC 1/2] hv_netvsc: make sure device is idle before changes
2018-01-27 0:18 [RFC 0/2] hv_netvsc shutdown redo Stephen Hemminger
@ 2018-01-27 0:18 ` Stephen Hemminger
2018-01-27 0:18 ` [RFC 2/2] Revert "hv_netvsc: netvsc_teardown_gpadl() split" Stephen Hemminger
2018-01-27 16:21 ` [RFC 0/2] hv_netvsc shutdown redo Vitaly Kuznetsov
2 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2018-01-27 0:18 UTC (permalink / raw)
To: kys, haiyangz, vkuznets, mgamal
Cc: netdev, Stephen Hemminger, Stephen Hemminger
Make sure that device is in detached state before doing ring
and mtu changes. When doing these changes, wait for all outstanding
send completions and ring buffer events.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/net/hyperv/hyperv_net.h | 1 -
drivers/net/hyperv/netvsc.c | 6 +----
drivers/net/hyperv/netvsc_drv.c | 29 +++++++++++++----------
drivers/net/hyperv/rndis_filter.c | 48 ++++++++++++++++++---------------------
4 files changed, 40 insertions(+), 44 deletions(-)
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 0db3bd1ea06f..a846a9c50ddb 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -211,7 +211,6 @@ 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);
struct netvsc_device *rndis_filter_device_add(struct hv_device *dev,
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 17e529af79dc..619a04f98321 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -849,7 +849,7 @@ int netvsc_send(struct net_device *ndev,
bool try_batch, xmit_more;
/* If device is rescinded, return error and packet will get dropped. */
- if (unlikely(!net_device || net_device->destroy))
+ if (unlikely(!net_device))
return -ENODEV;
/* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
@@ -996,10 +996,6 @@ static int send_recv_completions(struct net_device *ndev,
mrc->first = 0;
}
- /* receive completion ring has been emptied */
- if (unlikely(nvdev->destroy))
- wake_up(&nvdev->wait_drain);
-
return 0;
}
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index c5584c2d440e..ef395e379a83 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -820,7 +820,7 @@ static int netvsc_set_channels(struct net_device *net,
channels->rx_count || channels->tx_count || channels->other_count)
return -EINVAL;
- if (!nvdev || nvdev->destroy)
+ if (!nvdev)
return -ENODEV;
if (nvdev->nvsp_version < NVSP_PROTOCOL_VERSION_5)
@@ -830,9 +830,6 @@ static int netvsc_set_channels(struct net_device *net,
return -EINVAL;
orig = nvdev->num_chn;
- was_opened = rndis_filter_opened(nvdev);
- if (was_opened)
- rndis_filter_close(nvdev);
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = count;
@@ -841,6 +838,11 @@ static int netvsc_set_channels(struct net_device *net,
device_info.recv_sections = nvdev->recv_section_cnt;
device_info.recv_section_size = nvdev->recv_section_size;
+ was_opened = netif_running(net);
+ netif_device_detach(net);
+ if (was_opened)
+ rndis_filter_close(nvdev);
+
rndis_filter_device_remove(dev, nvdev);
nvdev = rndis_filter_device_add(dev, &device_info);
@@ -859,6 +861,8 @@ static int netvsc_set_channels(struct net_device *net,
if (was_opened)
rndis_filter_open(nvdev);
+ netif_device_attach(net);
+
/* We may have missed link change notifications */
net_device_ctx->last_reconfig = 0;
schedule_delayed_work(&net_device_ctx->dwork, 0);
@@ -934,7 +938,7 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
bool was_opened;
int ret = 0;
- if (!nvdev || nvdev->destroy)
+ if (!nvdev)
return -ENODEV;
/* Change MTU of underlying VF netdev first. */
@@ -944,11 +948,6 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
return ret;
}
- netif_device_detach(ndev);
- was_opened = rndis_filter_opened(nvdev);
- if (was_opened)
- rndis_filter_close(nvdev);
-
memset(&device_info, 0, sizeof(device_info));
device_info.num_chn = nvdev->num_chn;
device_info.send_sections = nvdev->send_section_cnt;
@@ -956,6 +955,11 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
device_info.recv_sections = nvdev->recv_section_cnt;
device_info.recv_section_size = nvdev->recv_section_size;
+ was_opened = netif_running(ndev);
+ netif_device_detach(ndev);
+ if (was_opened)
+ rndis_filter_close(nvdev);
+
rndis_filter_device_remove(hdev, nvdev);
ndev->mtu = mtu;
@@ -1497,7 +1501,7 @@ static int netvsc_set_ringparam(struct net_device *ndev,
bool was_opened;
int ret = 0;
- if (!nvdev || nvdev->destroy)
+ if (!nvdev)
return -ENODEV;
memset(&orig, 0, sizeof(orig));
@@ -1519,8 +1523,8 @@ static int netvsc_set_ringparam(struct net_device *ndev,
device_info.recv_sections = new_rx;
device_info.recv_section_size = nvdev->recv_section_size;
+ was_opened = netif_running(ndev);
netif_device_detach(ndev);
- was_opened = rndis_filter_opened(nvdev);
if (was_opened)
rndis_filter_close(nvdev);
@@ -1542,6 +1546,7 @@ static int netvsc_set_ringparam(struct net_device *ndev,
if (was_opened)
rndis_filter_open(nvdev);
+
netif_device_attach(ndev);
/* We may have missed link change notifications */
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index c3ca191fea7f..a6c9c9a2973d 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -923,6 +923,7 @@ static int rndis_filter_init_device(struct rndis_device *dev,
return ret;
}
+/* Are there any outstanding sends */
static bool netvsc_device_idle(const struct netvsc_device *nvdev)
{
int i;
@@ -930,8 +931,7 @@ static bool netvsc_device_idle(const struct netvsc_device *nvdev)
for (i = 0; i < nvdev->num_chn; i++) {
const struct netvsc_channel *nvchan = &nvdev->chan_table[i];
- if (nvchan->mrc.first != nvchan->mrc.next)
- return false;
+ napi_synchronize(&nvchan->napi);
if (atomic_read(&nvchan->queue_sends) > 0)
return false;
@@ -944,14 +944,12 @@ static void rndis_filter_halt_device(struct rndis_device *dev)
{
struct rndis_request *request;
struct rndis_halt_request *halt;
- struct net_device_context *net_device_ctx = netdev_priv(dev->ndev);
- struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
/* Attempt to do a rndis device halt */
request = get_rndis_request(dev, RNDIS_MSG_HALT,
RNDIS_MESSAGE_SIZE(struct rndis_halt_request));
if (!request)
- goto cleanup;
+ return;
/* Setup the rndis set */
halt = &request->request_msg.msg.halt_req;
@@ -962,17 +960,7 @@ static void rndis_filter_halt_device(struct rndis_device *dev)
dev->state = RNDIS_DEV_UNINITIALIZED;
-cleanup:
- nvdev->destroy = true;
-
- /* Force flag to be ordered before waiting */
- wmb();
-
- /* Wait for all send completions */
- wait_event(nvdev->wait_drain, netvsc_device_idle(nvdev));
-
- if (request)
- put_rndis_request(dev, request);
+ put_rndis_request(dev, request);
}
static int rndis_filter_open_device(struct rndis_device *dev)
@@ -994,9 +982,11 @@ static int rndis_filter_open_device(struct rndis_device *dev)
static int rndis_filter_close_device(struct rndis_device *dev)
{
+ struct net_device_context *net_device_ctx = netdev_priv(dev->ndev);
+ struct netvsc_device *nvdev = rtnl_dereference(net_device_ctx->nvdev);
int ret;
- if (dev->state != RNDIS_DEV_DATAINITIALIZED)
+ if (!nvdev || dev->state != RNDIS_DEV_DATAINITIALIZED)
return 0;
/* Make sure rndis_set_multicast doesn't re-enable filter! */
@@ -1004,10 +994,23 @@ static int rndis_filter_close_device(struct rndis_device *dev)
ret = rndis_filter_set_packet_filter(dev, 0);
if (ret == -ENODEV)
- ret = 0;
+ ret = 0; /* rescinded is ok */
+
+ if (ret == 0) {
+ /* Indicate that wakeup on send done is desired */
+ nvdev->destroy = true;
+
+ /* Force flag to be ordered before waiting */
+ wmb();
+
+ /* Wait for all completions */
+ wait_event(nvdev->wait_drain, netvsc_device_idle(nvdev));
+
+ /* No more wakeups please */
+ nvdev->destroy = false;
- if (ret == 0)
dev->state = RNDIS_DEV_INITIALIZED;
+ }
return ret;
}
@@ -1364,10 +1367,3 @@ int rndis_filter_close(struct netvsc_device *nvdev)
return rndis_filter_close_device(nvdev->extension);
}
-
-bool rndis_filter_opened(const struct netvsc_device *nvdev)
-{
- const struct rndis_device *dev = nvdev->extension;
-
- return dev->state == RNDIS_DEV_DATAINITIALIZED;
-}
--
2.15.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [RFC 2/2] Revert "hv_netvsc: netvsc_teardown_gpadl() split"
2018-01-27 0:18 [RFC 0/2] hv_netvsc shutdown redo Stephen Hemminger
2018-01-27 0:18 ` [RFC 1/2] hv_netvsc: make sure device is idle before changes Stephen Hemminger
@ 2018-01-27 0:18 ` Stephen Hemminger
2018-01-27 16:21 ` [RFC 0/2] hv_netvsc shutdown redo Vitaly Kuznetsov
2 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2018-01-27 0:18 UTC (permalink / raw)
To: kys, haiyangz, vkuznets, mgamal
Cc: netdev, Stephen Hemminger, Stephen Hemminger
This reverts commit 0cf737808ae7cb25e952be619db46b9147a92f46.
The problem that the previous commit was trying to solve was that
undoing the mapping of the receive buffer after revoke was
problematic. This was because the shutdown logic was not ensuring
that there were no receive and sends in flight. The changes in
commit 30de1885e897 ("hv_netvsc: make sure device is idle before changes")
ensure that device is completely idle.
Windows Server 2012 does not allow the receive buffer mapping to be
undone after the channel is closed. This because it assumes when
channel is closed, guest won't use it.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
drivers/net/hyperv/netvsc.c | 69 ++++++++++++++++++++++-----------------------
1 file changed, 33 insertions(+), 36 deletions(-)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index 619a04f98321..6db9bfb5c595 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -101,11 +101,12 @@ static void free_netvsc_device_rcu(struct netvsc_device *nvdev)
call_rcu(&nvdev->rcu, free_netvsc_device);
}
-static void netvsc_revoke_buf(struct hv_device *device,
- struct netvsc_device *net_device)
+static void netvsc_destroy_buf(struct hv_device *device)
{
struct nvsp_message *revoke_packet;
struct net_device *ndev = hv_get_drvdata(device);
+ struct net_device_context *ndc = netdev_priv(ndev);
+ struct netvsc_device *net_device = rtnl_dereference(ndc->nvdev);
int ret;
/*
@@ -148,6 +149,28 @@ static void netvsc_revoke_buf(struct hv_device *device,
net_device->recv_section_cnt = 0;
}
+ /* Teardown the gpadl on the vsp end */
+ if (net_device->recv_buf_gpadl_handle) {
+ ret = vmbus_teardown_gpadl(device->channel,
+ net_device->recv_buf_gpadl_handle);
+
+ /* If we failed here, we might as well return and have a leak
+ * rather than continue and a bugchk
+ */
+ if (ret != 0) {
+ netdev_err(ndev,
+ "unable to teardown receive buffer's gpadl\n");
+ return;
+ }
+ net_device->recv_buf_gpadl_handle = 0;
+ }
+
+ if (net_device->recv_buf) {
+ /* Free up the receive buffer */
+ vfree(net_device->recv_buf);
+ net_device->recv_buf = NULL;
+ }
+
/* Deal with the send buffer we may have setup.
* If we got a send section size, it means we received a
* NVSP_MSG1_TYPE_SEND_SEND_BUF_COMPLETE msg (ie sent
@@ -188,35 +211,7 @@ static void netvsc_revoke_buf(struct hv_device *device,
}
net_device->send_section_cnt = 0;
}
-}
-
-static void netvsc_teardown_gpadl(struct hv_device *device,
- struct netvsc_device *net_device)
-{
- struct net_device *ndev = hv_get_drvdata(device);
- int ret;
-
- if (net_device->recv_buf_gpadl_handle) {
- ret = vmbus_teardown_gpadl(device->channel,
- net_device->recv_buf_gpadl_handle);
-
- /* If we failed here, we might as well return and have a leak
- * rather than continue and a bugchk
- */
- if (ret != 0) {
- netdev_err(ndev,
- "unable to teardown receive buffer's gpadl\n");
- return;
- }
- net_device->recv_buf_gpadl_handle = 0;
- }
-
- if (net_device->recv_buf) {
- /* Free up the receive buffer */
- vfree(net_device->recv_buf);
- net_device->recv_buf = NULL;
- }
-
+ /* Teardown the gpadl on the vsp end */
if (net_device->send_buf_gpadl_handle) {
ret = vmbus_teardown_gpadl(device->channel,
net_device->send_buf_gpadl_handle);
@@ -431,8 +426,7 @@ static int netvsc_init_buf(struct hv_device *device,
goto exit;
cleanup:
- netvsc_revoke_buf(device, net_device);
- netvsc_teardown_gpadl(device, net_device);
+ netvsc_destroy_buf(device);
exit:
return ret;
@@ -551,6 +545,11 @@ static int netvsc_connect_vsp(struct hv_device *device,
return ret;
}
+static void netvsc_disconnect_vsp(struct hv_device *device)
+{
+ netvsc_destroy_buf(device);
+}
+
/*
* netvsc_device_remove - Callback when the root bus device is removed
*/
@@ -564,7 +563,7 @@ void netvsc_device_remove(struct hv_device *device)
cancel_work_sync(&net_device->subchan_work);
- netvsc_revoke_buf(device, net_device);
+ netvsc_disconnect_vsp(device);
RCU_INIT_POINTER(net_device_ctx->nvdev, NULL);
@@ -577,8 +576,6 @@ void netvsc_device_remove(struct hv_device *device)
/* Now, we can close the channel safely */
vmbus_close(device->channel);
- netvsc_teardown_gpadl(device, net_device);
-
/* And dissassociate NAPI context from device */
for (i = 0; i < net_device->num_chn; i++)
netif_napi_del(&net_device->chan_table[i].napi);
--
2.15.1
^ permalink raw reply related [flat|nested] 5+ messages in thread