Re: [RFC] hv_netvsc: safer orderly shutdown
From: Vitaly Kuznetsov <vkuznets@redhat.com>
Date: 2017-11-10 10:42:47
Also in:
lkml
Stephen Hemminger [off-list ref] writes:
quoted hunk
Several types of control operations require that the underlying RNDIS infrastructure be restarted. This patch changes the ordering of the shutdown to avoid race conditions. Stop all transmits before doing RNDIS halt. This involves stopping the network device transmit queues, then waiting for all outstanding sends before informing host to halt. Also, check for successful restart of the device when after the change is done. For review, not tested on Hyper-V yet. Signed-off-by: Stephen Hemminger <redacted> --- drivers/net/hyperv/netvsc_drv.c | 40 ++++++++++++++++++++++++++++++--------- drivers/net/hyperv/rndis_filter.c | 23 +++++++++++----------- 2 files changed, 42 insertions(+), 21 deletions(-)diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index da216ca4f2b2..3afa082e093d 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c@@ -855,8 +855,10 @@ static int netvsc_set_channels(struct net_device *net, orig = nvdev->num_chn; was_opened = rndis_filter_opened(nvdev); - if (was_opened) + if (was_opened) { + netif_tx_disable(net); rndis_filter_close(nvdev); + }
I was also experimenting with this and I think it may also make sense to
add napi_disable() for all queues here.
It also seems that the usual TX disable pattern is
netif_tx_stop_all_queues(net);
netif_tx_disable(net);
not sure why..
quoted hunk
memset(&device_info, 0, sizeof(device_info)); device_info.num_chn = count;@@ -881,8 +883,13 @@ static int netvsc_set_channels(struct net_device *net, } } - if (was_opened) - rndis_filter_open(nvdev); + if (was_opened) { + ret = rndis_filter_open(nvdev); + if (ret) + netdev_err(net, "reopening device failed: %d\n", ret); + else + netif_tx_start_all_queues(net); + } /* We may have missed link change notifications */ net_device_ctx->last_reconfig = 0;@@ -971,8 +978,10 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) netif_device_detach(ndev); was_opened = rndis_filter_opened(nvdev); - if (was_opened) + if (was_opened) { + netif_tx_disable(net); rndis_filter_close(nvdev); + }
Shall we just nove netif_tx_disable() & friends to rndis_filter_close()?
quoted hunk
memset(&device_info, 0, sizeof(device_info)); device_info.ring_size = ring_size;@@ -1004,8 +1013,13 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu) } } - if (was_opened) - rndis_filter_open(nvdev); + if (was_opened) { + ret = rndis_filter_open(nvdev); + if (ret) + netdev_err(net, "reopening device failed: %d\n", ret); + else + netif_tx_start_all_queues(net); + }
Yea, the main problem here is that we can't do much if we fail, the device will be completely unusable. That's not something users exepct doing MTU change...
quoted hunk
netif_device_attach(ndev);@@ -1547,8 +1561,10 @@ static int netvsc_set_ringparam(struct net_device *ndev, netif_device_detach(ndev); was_opened = rndis_filter_opened(nvdev); - if (was_opened) + if (was_opened) { + netif_tx_disable(net); rndis_filter_close(nvdev); + } rndis_filter_device_remove(hdev, nvdev);@@ -1566,8 +1582,14 @@ static int netvsc_set_ringparam(struct net_device *ndev, } } - if (was_opened) - rndis_filter_open(nvdev); + if (was_opened) { + ret = rndis_filter_open(nvdev); + if (ret) + netdev_err(net, "reopening device failed: %d\n", ret); + else + netif_tx_start_all_queues(net); + } + 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 0648eebda829..164f5ffe9c50 100644 --- a/drivers/net/hyperv/rndis_filter.c +++ b/drivers/net/hyperv/rndis_filter.c@@ -948,11 +948,20 @@ static void rndis_filter_halt_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); + /* tell bottom half that deice is being closed */ + 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)); + /* 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;@@ -963,17 +972,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)
I'll give this patch a shot to see if anything blows up. Thanks! -- Vitaly