| Message ID | 1617060095-31582-1-git-send-email-haiyangz@microsoft.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d0922bf7981799fd86e248de330fb4152399d6c2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] hv_netvsc: Add error handling while switching data path | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 2 maintainers not CCed: wei.liu@kernel.org kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | CHECK: Lines should not end with a '(' WARNING: else is not generally useful after a break or return |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
Hello: This patch was applied to netdev/net-next.git (refs/heads/master): On Mon, 29 Mar 2021 16:21:35 -0700 you wrote: > Add error handling in case of failure to send switching data path message > to the host. > > Reported-by: Shachar Raindel <shacharr@microsoft.com> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > > [...] Here is the summary with links: - [net-next] hv_netvsc: Add error handling while switching data path https://git.kernel.org/netdev/net-next/c/d0922bf79817 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
Haiyang Zhang <haiyangz@microsoft.com> writes: > Add error handling in case of failure to send switching data path message > to the host. > > Reported-by: Shachar Raindel <shacharr@microsoft.com> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > --- > drivers/net/hyperv/hyperv_net.h | 6 +++++- > drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++---- > drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------ > 3 files changed, 48 insertions(+), 11 deletions(-) > > diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h > index 59ac04a610ad..442c520ab8f3 100644 > --- a/drivers/net/hyperv/hyperv_net.h > +++ b/drivers/net/hyperv/hyperv_net.h > @@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev, > int rndis_filter_set_device_mac(struct netvsc_device *ndev, > const char *mac); > > -void netvsc_switch_datapath(struct net_device *nv_dev, bool vf); > +int netvsc_switch_datapath(struct net_device *nv_dev, bool vf); > > #define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF) > > @@ -1718,4 +1718,8 @@ struct rndis_message { > #define TRANSPORT_INFO_IPV6_TCP 0x10 > #define TRANSPORT_INFO_IPV6_UDP 0x20 > > +#define RETRY_US_LO 5000 > +#define RETRY_US_HI 10000 > +#define RETRY_MAX 2000 /* >10 sec */ > + > #endif /* _HYPERV_NET_H */ > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > index 5bce24731502..9d07c9ce4be2 100644 > --- a/drivers/net/hyperv/netvsc.c > +++ b/drivers/net/hyperv/netvsc.c > @@ -31,12 +31,13 @@ > * Switch the data path from the synthetic interface to the VF > * interface. > */ > -void netvsc_switch_datapath(struct net_device *ndev, bool vf) > +int netvsc_switch_datapath(struct net_device *ndev, bool vf) > { > struct net_device_context *net_device_ctx = netdev_priv(ndev); > struct hv_device *dev = net_device_ctx->device_ctx; > struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx->nvdev); > struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt; > + int ret, retry = 0; > > /* Block sending traffic to VF if it's about to be gone */ > if (!vf) > @@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf) > init_pkt->msg.v4_msg.active_dp.active_datapath = > NVSP_DATAPATH_SYNTHETIC; > > +again: > trace_nvsp_send(ndev, init_pkt); > > - vmbus_sendpacket(dev->channel, init_pkt, > + ret = vmbus_sendpacket(dev->channel, init_pkt, > sizeof(struct nvsp_message), > - (unsigned long)init_pkt, > - VM_PKT_DATA_INBAND, > + (unsigned long)init_pkt, VM_PKT_DATA_INBAND, > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > + > + /* If failed to switch to/from VF, let data_path_is_vf stay false, > + * so we use synthetic path to send data. > + */ > + if (ret) { > + if (ret != -EAGAIN) { > + netdev_err(ndev, > + "Unable to send sw datapath msg, err: %d\n", > + ret); > + return ret; > + } > + > + if (retry++ < RETRY_MAX) { > + usleep_range(RETRY_US_LO, RETRY_US_HI); > + goto again; > + } else { > + netdev_err( > + ndev, > + "Retry failed to send sw datapath msg, err: %d\n", > + ret); err is always -EAGAIN here, right? > + return ret; > + } Nitpicking: I think we can simplify the above a bit: if (ret) { if (ret == -EAGAIN && retry++ < RETRY_MAX) { usleep_range(RETRY_US_LO, RETRY_US_HI); goto again; } netdev_err(ndev, "Unable to send sw datapath msg, err: %d\n", ret); return ret; } > + } > + > wait_for_completion(&nv_dev->channel_init_wait); > net_device_ctx->data_path_is_vf = vf; > + > + return 0; > } > > /* Worker to setup sub channels on initial setup > diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c > index 97b5c9b60503..7349a70af083 100644 > --- a/drivers/net/hyperv/netvsc_drv.c > +++ b/drivers/net/hyperv/netvsc_drv.c > @@ -38,9 +38,6 @@ > #include "hyperv_net.h" > > #define RING_SIZE_MIN 64 > -#define RETRY_US_LO 5000 > -#define RETRY_US_HI 10000 > -#define RETRY_MAX 2000 /* >10 sec */ > > #define LINKCHANGE_INT (2 * HZ) > #define VF_TAKEOVER_INT (HZ / 10) > @@ -2402,6 +2399,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event) > struct netvsc_device *netvsc_dev; > struct net_device *ndev; > bool vf_is_up = false; > + int ret; > > if (event != NETDEV_GOING_DOWN) > vf_is_up = netif_running(vf_netdev); > @@ -2418,9 +2416,17 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event) > if (net_device_ctx->data_path_is_vf == vf_is_up) > return NOTIFY_OK; > > - netvsc_switch_datapath(ndev, vf_is_up); > - netdev_info(ndev, "Data path switched %s VF: %s\n", > - vf_is_up ? "to" : "from", vf_netdev->name); > + ret = netvsc_switch_datapath(ndev, vf_is_up); > + > + if (ret) { > + netdev_err(ndev, > + "Data path failed to switch %s VF: %s, err: %d\n", > + vf_is_up ? "to" : "from", vf_netdev->name, ret); > + return NOTIFY_DONE; > + } else { > + netdev_info(ndev, "Data path switched %s VF: %s\n", > + vf_is_up ? "to" : "from", vf_netdev->name); > + } > > return NOTIFY_OK; > }
> -----Original Message----- > From: Vitaly Kuznetsov <vkuznets@redhat.com> > Sent: Tuesday, March 30, 2021 7:43 AM > To: Haiyang Zhang <haiyangz@microsoft.com>; linux- > hyperv@vger.kernel.org; netdev@vger.kernel.org > Cc: Haiyang Zhang <haiyangz@microsoft.com>; KY Srinivasan > <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>; > olaf@aepfle.de; davem@davemloft.net; linux-kernel@vger.kernel.org > Subject: Re: [PATCH net-next] hv_netvsc: Add error handling while switching > data path > > Haiyang Zhang <haiyangz@microsoft.com> writes: > > > Add error handling in case of failure to send switching data path message > > to the host. > > > > Reported-by: Shachar Raindel <shacharr@microsoft.com> > > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> > > > > --- > > drivers/net/hyperv/hyperv_net.h | 6 +++++- > > drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++- > --- > > drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------ > > 3 files changed, 48 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/net/hyperv/hyperv_net.h > b/drivers/net/hyperv/hyperv_net.h > > index 59ac04a610ad..442c520ab8f3 100644 > > --- a/drivers/net/hyperv/hyperv_net.h > > +++ b/drivers/net/hyperv/hyperv_net.h > > @@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev, > > int rndis_filter_set_device_mac(struct netvsc_device *ndev, > > const char *mac); > > > > -void netvsc_switch_datapath(struct net_device *nv_dev, bool vf); > > +int netvsc_switch_datapath(struct net_device *nv_dev, bool vf); > > > > #define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF) > > > > @@ -1718,4 +1718,8 @@ struct rndis_message { > > #define TRANSPORT_INFO_IPV6_TCP 0x10 > > #define TRANSPORT_INFO_IPV6_UDP 0x20 > > > > +#define RETRY_US_LO 5000 > > +#define RETRY_US_HI 10000 > > +#define RETRY_MAX 2000 /* >10 sec */ > > + > > #endif /* _HYPERV_NET_H */ > > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c > > index 5bce24731502..9d07c9ce4be2 100644 > > --- a/drivers/net/hyperv/netvsc.c > > +++ b/drivers/net/hyperv/netvsc.c > > @@ -31,12 +31,13 @@ > > * Switch the data path from the synthetic interface to the VF > > * interface. > > */ > > -void netvsc_switch_datapath(struct net_device *ndev, bool vf) > > +int netvsc_switch_datapath(struct net_device *ndev, bool vf) > > { > > struct net_device_context *net_device_ctx = netdev_priv(ndev); > > struct hv_device *dev = net_device_ctx->device_ctx; > > struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx- > >nvdev); > > struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt; > > + int ret, retry = 0; > > > > /* Block sending traffic to VF if it's about to be gone */ > > if (!vf) > > @@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device > *ndev, bool vf) > > init_pkt->msg.v4_msg.active_dp.active_datapath = > > NVSP_DATAPATH_SYNTHETIC; > > > > +again: > > trace_nvsp_send(ndev, init_pkt); > > > > - vmbus_sendpacket(dev->channel, init_pkt, > > + ret = vmbus_sendpacket(dev->channel, init_pkt, > > sizeof(struct nvsp_message), > > - (unsigned long)init_pkt, > > - VM_PKT_DATA_INBAND, > > + (unsigned long)init_pkt, VM_PKT_DATA_INBAND, > > > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > > + > > + /* If failed to switch to/from VF, let data_path_is_vf stay false, > > + * so we use synthetic path to send data. > > + */ > > + if (ret) { > > + if (ret != -EAGAIN) { > > + netdev_err(ndev, > > + "Unable to send sw datapath msg, > err: %d\n", > > + ret); > > + return ret; > > + } > > + > > + if (retry++ < RETRY_MAX) { > > + usleep_range(RETRY_US_LO, RETRY_US_HI); > > + goto again; > > + } else { > > + netdev_err( > > + ndev, > > + "Retry failed to send sw datapath msg, > err: %d\n", > > + ret); > > err is always -EAGAIN here, right? > > > + return ret; > > + } > > Nitpicking: I think we can simplify the above a bit: > > if (ret) { > if (ret == -EAGAIN && retry++ < RETRY_MAX) { > usleep_range(RETRY_US_LO, RETRY_US_HI); > goto again; > } > netdev_err(ndev, "Unable to send sw datapath msg, > err: %d\n", ret); > return ret; > } > Yes this looks cleaner. Thanks, Haiyang
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h index 59ac04a610ad..442c520ab8f3 100644 --- a/drivers/net/hyperv/hyperv_net.h +++ b/drivers/net/hyperv/hyperv_net.h @@ -269,7 +269,7 @@ int rndis_filter_receive(struct net_device *ndev, int rndis_filter_set_device_mac(struct netvsc_device *ndev, const char *mac); -void netvsc_switch_datapath(struct net_device *nv_dev, bool vf); +int netvsc_switch_datapath(struct net_device *nv_dev, bool vf); #define NVSP_INVALID_PROTOCOL_VERSION ((u32)0xFFFFFFFF) @@ -1718,4 +1718,8 @@ struct rndis_message { #define TRANSPORT_INFO_IPV6_TCP 0x10 #define TRANSPORT_INFO_IPV6_UDP 0x20 +#define RETRY_US_LO 5000 +#define RETRY_US_HI 10000 +#define RETRY_MAX 2000 /* >10 sec */ + #endif /* _HYPERV_NET_H */ diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 5bce24731502..9d07c9ce4be2 100644 --- a/drivers/net/hyperv/netvsc.c +++ b/drivers/net/hyperv/netvsc.c @@ -31,12 +31,13 @@ * Switch the data path from the synthetic interface to the VF * interface. */ -void netvsc_switch_datapath(struct net_device *ndev, bool vf) +int netvsc_switch_datapath(struct net_device *ndev, bool vf) { struct net_device_context *net_device_ctx = netdev_priv(ndev); struct hv_device *dev = net_device_ctx->device_ctx; struct netvsc_device *nv_dev = rtnl_dereference(net_device_ctx->nvdev); struct nvsp_message *init_pkt = &nv_dev->channel_init_pkt; + int ret, retry = 0; /* Block sending traffic to VF if it's about to be gone */ if (!vf) @@ -51,15 +52,41 @@ void netvsc_switch_datapath(struct net_device *ndev, bool vf) init_pkt->msg.v4_msg.active_dp.active_datapath = NVSP_DATAPATH_SYNTHETIC; +again: trace_nvsp_send(ndev, init_pkt); - vmbus_sendpacket(dev->channel, init_pkt, + ret = vmbus_sendpacket(dev->channel, init_pkt, sizeof(struct nvsp_message), - (unsigned long)init_pkt, - VM_PKT_DATA_INBAND, + (unsigned long)init_pkt, VM_PKT_DATA_INBAND, VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); + + /* If failed to switch to/from VF, let data_path_is_vf stay false, + * so we use synthetic path to send data. + */ + if (ret) { + if (ret != -EAGAIN) { + netdev_err(ndev, + "Unable to send sw datapath msg, err: %d\n", + ret); + return ret; + } + + if (retry++ < RETRY_MAX) { + usleep_range(RETRY_US_LO, RETRY_US_HI); + goto again; + } else { + netdev_err( + ndev, + "Retry failed to send sw datapath msg, err: %d\n", + ret); + return ret; + } + } + wait_for_completion(&nv_dev->channel_init_wait); net_device_ctx->data_path_is_vf = vf; + + return 0; } /* Worker to setup sub channels on initial setup diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c index 97b5c9b60503..7349a70af083 100644 --- a/drivers/net/hyperv/netvsc_drv.c +++ b/drivers/net/hyperv/netvsc_drv.c @@ -38,9 +38,6 @@ #include "hyperv_net.h" #define RING_SIZE_MIN 64 -#define RETRY_US_LO 5000 -#define RETRY_US_HI 10000 -#define RETRY_MAX 2000 /* >10 sec */ #define LINKCHANGE_INT (2 * HZ) #define VF_TAKEOVER_INT (HZ / 10) @@ -2402,6 +2399,7 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event) struct netvsc_device *netvsc_dev; struct net_device *ndev; bool vf_is_up = false; + int ret; if (event != NETDEV_GOING_DOWN) vf_is_up = netif_running(vf_netdev); @@ -2418,9 +2416,17 @@ static int netvsc_vf_changed(struct net_device *vf_netdev, unsigned long event) if (net_device_ctx->data_path_is_vf == vf_is_up) return NOTIFY_OK; - netvsc_switch_datapath(ndev, vf_is_up); - netdev_info(ndev, "Data path switched %s VF: %s\n", - vf_is_up ? "to" : "from", vf_netdev->name); + ret = netvsc_switch_datapath(ndev, vf_is_up); + + if (ret) { + netdev_err(ndev, + "Data path failed to switch %s VF: %s, err: %d\n", + vf_is_up ? "to" : "from", vf_netdev->name, ret); + return NOTIFY_DONE; + } else { + netdev_info(ndev, "Data path switched %s VF: %s\n", + vf_is_up ? "to" : "from", vf_netdev->name); + } return NOTIFY_OK; }
Add error handling in case of failure to send switching data path message to the host. Reported-by: Shachar Raindel <shacharr@microsoft.com> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com> --- drivers/net/hyperv/hyperv_net.h | 6 +++++- drivers/net/hyperv/netvsc.c | 35 +++++++++++++++++++++++++++++---- drivers/net/hyperv/netvsc_drv.c | 18 +++++++++++------ 3 files changed, 48 insertions(+), 11 deletions(-)