Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Haiyang Zhang <haiyangz@microsoft.com>,
        linux-hyperv@vger.kernel.org, netdev@vger.kernel.org
Cc:     haiyangz@microsoft.com, kys@microsoft.com, 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
In-Reply-To: <1617060095-31582-1-git-send-email-haiyangz@microsoft.com>
References: <1617060095-31582-1-git-send-email-haiyangz@microsoft.com>
Date:   Tue, 30 Mar 2021 13:43:09 +0200
Message-ID: <87lfa4uasy.fsf@vitty.brq.redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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

-- 
Vitaly