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

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

[Vitaly Kuznetsov]

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@redhat.com>
---
 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;
 	}
 	return ret;
 }
diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
index a5b4442..7d9c31b 100644
--- a/drivers/hv/hyperv_vmbus.h
+++ b/drivers/hv/hyperv_vmbus.h
@@ -683,7 +683,7 @@ void vmbus_free_channels(void);
 int vmbus_connect(void);
 void vmbus_disconnect(void);
 
-int vmbus_post_msg(void *buffer, size_t buflen);
+int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep);
 
 void vmbus_on_event(unsigned long data);
 void vmbus_on_msg_dpc(unsigned long data);
-- 
2.7.4

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

[Haiyang Zhang]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, October 26, 2016 7:12 AM
> To: devel@linuxdriverproject.org
> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>
> 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@redhat.com>
> ---
>  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@microsoft.com>

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, October 26, 2016 4:12 AM
> To: devel@linuxdriverproject.org
> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> Haiyang Zhang <haiyangz@microsoft.com>
> 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.

Vitaly,

One of the reasons why the delay was in microseconds was to make sure that the boot time
was not adversely affected by the delay we had in setting up the channel. The change to microsecond
delay and other changes in this code reduced the time it took to initialize netvsc from 
200 milliseconds to about 12 milliseconds. This is important for us as we look at achieving sub-second
boot times.
The situation you are trying to address are test cases where you are hitting the host with
requests that triggers hosts DOS prevention code. Perhaps we could have a hybrid approach: we
retain microsecond wait until we hit a threshold and then we use millisecond delays. This way, the normal boot
path is still fast while we can handle some of the other cases where the host DOS prevention code kicks in.

Regards,

K. Y
 
> 
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  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;
>  	}
>  	return ret;
>  }
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index a5b4442..7d9c31b 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -683,7 +683,7 @@ void vmbus_free_channels(void);
>  int vmbus_connect(void);
>  void vmbus_disconnect(void);
> 
> -int vmbus_post_msg(void *buffer, size_t buflen);
> +int vmbus_post_msg(void *buffer, size_t buflen, bool can_sleep);
> 
>  void vmbus_on_event(unsigned long data);
>  void vmbus_on_msg_dpc(unsigned long data);
> --
> 2.7.4

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

[Vitaly Kuznetsov]

KY Srinivasan <kys@microsoft.com> writes:

>> -----Original Message-----
>> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
>> Sent: Wednesday, October 26, 2016 4:12 AM
>> To: devel@linuxdriverproject.org
>> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
>> Haiyang Zhang <haiyangz@microsoft.com>
>> 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.
>
> Vitaly,
>
> One of the reasons why the delay was in microseconds was to make sure that the boot time
> was not adversely affected by the delay we had in setting up the channel. The change to microsecond
> delay and other changes in this code reduced the time it took to initialize netvsc from 
> 200 milliseconds to about 12 milliseconds. This is important for us as we look at achieving sub-second
> boot times.
> The situation you are trying to address are test cases where you are hitting the host with
> requests that triggers hosts DOS prevention code. Perhaps we could have a hybrid approach: we
> retain microsecond wait until we hit a threshold and then we use millisecond delays. This way, the normal boot
> path is still fast while we can handle some of the other cases where the host DOS prevention code kicks in.
>

Ok,

I actually tested boot time with my patch and didn't see a difference
(so I guess our first attempt to send messages usually succeeds) but if
we're concearned about less-than-a-second boot time we'd rather keep the
microseonds delay for first several attempts. I'll do v2.

Thanks,


-- 
  Vitaly

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

[Van De Ven, Arjan]

> Ok,
> 
> I actually tested boot time with my patch and didn't see a difference
> (so I guess our first attempt to send messages usually succeeds) but if
> we're concearned about less-than-a-second boot time we'd rather keep the
> microseonds delay for first several attempts. I'll do v2.

of course we care about less-than-a-second boot time..
it's a virtual machine.. there's no reason the kernel should ever take more than 100 msec total
(including all drivers)

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

[KY Srinivasan]

> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Monday, October 31, 2016 3:05 AM
> To: KY Srinivasan <kys@microsoft.com>
> Cc: devel@linuxdriverproject.org; Van De Ven, Arjan
> <arjan.van.de.ven@intel.com>; linux-kernel@vger.kernel.org; Haiyang Zhang
> <haiyangz@microsoft.com>
> Subject: Re: [PATCH] Drivers: hv: vmbus: Raise retry/wait limits in
> vmbus_post_msg()
> 
> KY Srinivasan <kys@microsoft.com> writes:
> 
> >> -----Original Message-----
> >> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> >> Sent: Wednesday, October 26, 2016 4:12 AM
> >> To: devel@linuxdriverproject.org
> >> Cc: linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>;
> >> Haiyang Zhang <haiyangz@microsoft.com>
> >> 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.
> >
> > Vitaly,
> >
> > One of the reasons why the delay was in microseconds was to make sure
> that the boot time
> > was not adversely affected by the delay we had in setting up the channel.
> The change to microsecond
> > delay and other changes in this code reduced the time it took to initialize
> netvsc from
> > 200 milliseconds to about 12 milliseconds. This is important for us as we look
> at achieving sub-second
> > boot times.
> > The situation you are trying to address are test cases where you are hitting
> the host with
> > requests that triggers hosts DOS prevention code. Perhaps we could have a
> hybrid approach: we
> > retain microsecond wait until we hit a threshold and then we use
> millisecond delays. This way, the normal boot
> > path is still fast while we can handle some of the other cases where the host
> DOS prevention code kicks in.
> >
> 
> Ok,
> 
> I actually tested boot time with my patch and didn't see a difference
> (so I guess our first attempt to send messages usually succeeds) but if
> we're concearned about less-than-a-second boot time we'd rather keep the
> microseonds delay for first several attempts. I'll do v2.

Thank you.

K. Y
> 
> Thanks,
> 
> 
> --
>   Vitaly