RE: [PATCH net-next] hv_netvsc: Add error handling while switching data path

[Haiyang Zhang <haiyangz@microsoft.com>]

> -----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