RE: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in vmbus_post_msg()

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Wednesday, October 26, 2016 7:12 AM
> To: devel@xxxxxxxxxxxxxxxxxxxxxx
> Cc: linux-kernel@xxxxxxxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>;
> Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>
> Subject: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in
> vmbus_post_msg()
> 
> DoS protection conditions were altered in WS2016 and now it's easy to
> get
> -EAGAIN returned from vmbus_post_msg() (e.g. when we try changing MTU on
> a
> netvsc device in a loop). All vmbus_post_msg() callers don't retry the
> operation and we usually end up with a non-functional device or crash.
> 
> While host's DoS protection conditions are unknown to me my tests show
> that
> it can take up to 46 attempts to send a message after changing udelay()
> to
> mdelay() and caping msec at '256', this means we can wait up to 10
> seconds
> before the message is sent so we need to use msleep() instead. Almost
> all
> vmbus_post_msg() callers are ready to sleep but there is one special
> case:
> vmbus_initiate_unload() which can be called from interrupt/NMI context
> and
> we can't sleep there. I'm also not sure about the lonely
> vmbus_send_tl_connect_request() which has no in-tree users but its
> external
> users are most likely waiting for the host to reply so sleeping there is
> also appropriate.
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> ---
>  drivers/hv/channel.c      | 17 +++++++++--------
>  drivers/hv/channel_mgmt.c | 10 ++++++----
>  drivers/hv/connection.c   | 19 ++++++++++++-------
>  drivers/hv/hyperv_vmbus.h |  2 +-
>  4 files changed, 28 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index 16f91c8..28ca66e 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -180,7 +180,7 @@ int vmbus_open(struct vmbus_channel *newchannel, u32
> send_ringbuffer_size,
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> 
>  	ret = vmbus_post_msg(open_msg,
> -			       sizeof(struct vmbus_channel_open_channel));
> +			     sizeof(struct vmbus_channel_open_channel), true);
> 
>  	if (ret != 0) {
>  		err = ret;
> @@ -232,7 +232,7 @@ int vmbus_send_tl_connect_request(const uuid_le
> *shv_guest_servie_id,
>  	conn_msg.guest_endpoint_id = *shv_guest_servie_id;
>  	conn_msg.host_service_id = *shv_host_servie_id;
> 
> -	return vmbus_post_msg(&conn_msg, sizeof(conn_msg));
> +	return vmbus_post_msg(&conn_msg, sizeof(conn_msg), true);
>  }
>  EXPORT_SYMBOL_GPL(vmbus_send_tl_connect_request);
> 
> @@ -418,7 +418,7 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> 
>  	ret = vmbus_post_msg(gpadlmsg, msginfo->msgsize -
> -			       sizeof(*msginfo));
> +			     sizeof(*msginfo), true);
>  	if (ret != 0)
>  		goto cleanup;
> 
> @@ -432,8 +432,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
>  		gpadl_body->gpadl = next_gpadl_handle;
> 
>  		ret = vmbus_post_msg(gpadl_body,
> -				     submsginfo->msgsize -
> -				     sizeof(*submsginfo));
> +				     submsginfo->msgsize - sizeof(*submsginfo),
> +				     true);
>  		if (ret != 0)
>  			goto cleanup;
> 
> @@ -484,8 +484,8 @@ int vmbus_teardown_gpadl(struct vmbus_channel
> *channel, u32 gpadl_handle)
>  	list_add_tail(&info->msglistentry,
>  		      &vmbus_connection.chn_msg_list);
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> -	ret = vmbus_post_msg(msg,
> -			       sizeof(struct vmbus_channel_gpadl_teardown));
> +	ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_gpadl_teardown),
> +			     true);
> 
>  	if (ret)
>  		goto post_msg_err;
> @@ -556,7 +556,8 @@ static int vmbus_close_internal(struct vmbus_channel
> *channel)
>  	msg->header.msgtype = CHANNELMSG_CLOSECHANNEL;
>  	msg->child_relid = channel->offermsg.child_relid;
> 
> -	ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_close_channel));
> +	ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_close_channel),
> +			     true);
> 
>  	if (ret) {
>  		pr_err("Close failed: close post msg return is %d\n", ret);
> diff --git a/drivers/hv/channel_mgmt.c b/drivers/hv/channel_mgmt.c
> index 96a85cd..160d92e 100644
> --- a/drivers/hv/channel_mgmt.c
> +++ b/drivers/hv/channel_mgmt.c
> @@ -321,7 +321,8 @@ static void vmbus_release_relid(u32 relid)
>  	memset(&msg, 0, sizeof(struct vmbus_channel_relid_released));
>  	msg.child_relid = relid;
>  	msg.header.msgtype = CHANNELMSG_RELID_RELEASED;
> -	vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released));
> +	vmbus_post_msg(&msg, sizeof(struct vmbus_channel_relid_released),
> +		       true);
>  }
> 
>  void hv_event_tasklet_disable(struct vmbus_channel *channel)
> @@ -728,7 +729,8 @@ void vmbus_initiate_unload(bool crash)
>  	init_completion(&vmbus_connection.unload_event);
>  	memset(&hdr, 0, sizeof(struct vmbus_channel_message_header));
>  	hdr.msgtype = CHANNELMSG_UNLOAD;
> -	vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header));
> +	vmbus_post_msg(&hdr, sizeof(struct vmbus_channel_message_header),
> +		       !crash);
> 
>  	/*
>  	 * vmbus_initiate_unload() is also called on crash and the crash
> can be
> @@ -1116,8 +1118,8 @@ int vmbus_request_offers(void)
>  	msg->msgtype = CHANNELMSG_REQUESTOFFERS;
> 
> 
> -	ret = vmbus_post_msg(msg,
> -			       sizeof(struct vmbus_channel_message_header));
> +	ret = vmbus_post_msg(msg, sizeof(struct
> vmbus_channel_message_header),
> +			     true);
>  	if (ret != 0) {
>  		pr_err("Unable to request offers - %d\n", ret);
> 
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 78e6368..da1feb4 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -110,7 +110,8 @@ static int vmbus_negotiate_version(struct
> vmbus_channel_msginfo *msginfo,
>  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> 
>  	ret = vmbus_post_msg(msg,
> -			       sizeof(struct vmbus_channel_initiate_contact));
> +			     sizeof(struct vmbus_channel_initiate_contact),
> +			     true);
>  	if (ret != 0) {
>  		spin_lock_irqsave(&vmbus_connection.channelmsg_lock, flags);
>  		list_del(&msginfo->msglistentry);
> @@ -434,12 +435,12 @@ void vmbus_on_event(unsigned long data)
>  /*
>   * vmbus_post_msg - Send a msg on the vmbus's message connection
>   */
> -int vmbus_post_msg(void *buffer, size_t buflen)
> +int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep)
>  {
>  	union hv_connection_id conn_id;
>  	int ret = 0;
>  	int retries = 0;
> -	u32 usec = 1;
> +	u32 msec = 1;
> 
>  	conn_id.asu32 = 0;
>  	conn_id.u.id = VMBUS_MESSAGE_CONNECTION_ID;
> @@ -449,7 +450,7 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  	 * insufficient resources. Retry the operation a couple of
>  	 * times before giving up.
>  	 */
> -	while (retries < 20) {
> +	while (retries < 100) {
>  		ret = hv_post_message(conn_id, 1, buffer, buflen);
> 
>  		switch (ret) {
> @@ -472,9 +473,13 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  		}
> 
>  		retries++;
> -		udelay(usec);
> -		if (usec < 2048)
> -			usec *= 2;
> +		if (can_sleep)
> +			msleep(msec);
> +		else
> +			mdelay(msec);
> +
> +		if (msec < 256)
> +			msec *= 2;

The change looks fine.
I knew, in case of initializing large trunk of receive/send buffers, vmbus_establish_gpadl() is called many times, and retrying of 1ms or more is needed often. So, using milliseconds delay/sleep is good.

Thanks,

Reviewed-by: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>