Re: [PATCH] Retry infinitely for hypercall

Re: [PATCH] Retry infinitely for hypercall

[Greg KH]

On Wed, Jan 04, 2017 at 02:39:31PM -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> Hyper-v host guarantees that a hypercall will succeed. Retry infinitely to avoid returning transient failures to upper layer.

Please wrap your changelog at the proper column.

And what happens when the hypercall does not succeed?  How is the kernel
going to recover from that?

> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/hv/connection.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 6ce8b87..4bcb099 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -439,7 +439,6 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  {
>  	union hv_connection_id conn_id;
>  	int ret = 0;
> -	int retries = 0;
>  	u32 usec = 1;
>  
>  	conn_id.asu32 = 0;
> @@ -447,10 +446,10 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  
>  	/*
>  	 * hv_post_message() can have transient failures because of
> -	 * insufficient resources. Retry the operation a couple of
> -	 * times before giving up.
> +	 * insufficient resources. We retry infinitely on these failures
> +	 * because host guarantees hypercall will eventually succeed.
>  	 */
> -	while (retries < 20) {
> +	while (1) {
>  		ret = hv_post_message(conn_id, 1, buffer, buflen);
>  
>  		switch (ret) {
> @@ -459,11 +458,11 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  			 * We could get this if we send messages too
>  			 * frequently.
>  			 */
> -			ret = -EAGAIN;
> -			break;

Document you are falling through please, otherwise someone will "fix"
this later.

>  		case HV_STATUS_INSUFFICIENT_MEMORY:
>  		case HV_STATUS_INSUFFICIENT_BUFFERS:
> -			ret = -ENOMEM;
> +			/*
> +			 * Temporary failure out of resources
> +			 */
>  			break;
>  		case HV_STATUS_SUCCESS:
>  			return ret;
> @@ -472,12 +471,12 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  			return -EINVAL;
>  		}
>  
> -		retries++;
>  		udelay(usec);
>  		if (usec < 2048)
>  			usec *= 2;
>  	}
> -	return ret;
> +	/* Impossible to get here */
> +	BUG_ON(1);

If it is impossible, why do you have this line at all?

What is this trying to solve?  Do you need to increase the time spent
waiting?  We all know things break, please allow the kernel to stay
alive if at all possible.

thanks,

greg k-h

Re: [PATCH] Retry infinitely for hypercall

[Dan Carpenter]

Fix the subsystem prefix in the subject.

On Wed, Jan 04, 2017 at 02:39:31PM -0800, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> Hyper-v host guarantees that a hypercall will succeed. Retry infinitely to avoid returning transient failures to upper layer.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>  drivers/hv/connection.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
> index 6ce8b87..4bcb099 100644
> --- a/drivers/hv/connection.c
> +++ b/drivers/hv/connection.c
> @@ -439,7 +439,6 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  {
>  	union hv_connection_id conn_id;
>  	int ret = 0;

Btw, when you disable GCC's uninitialized variable checking by storing
bogus values in "ret", it's eventually going to bite you in the bum.
Eventually you're going to get a bug that should have been detected
through static analysis if only you hadn't disabled it.

> -	int retries = 0;
>  	u32 usec = 1;
>  
>  	conn_id.asu32 = 0;
> @@ -447,10 +446,10 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  
>  	/*
>  	 * hv_post_message() can have transient failures because of
> -	 * insufficient resources. Retry the operation a couple of
> -	 * times before giving up.
> +	 * insufficient resources. We retry infinitely on these failures
> +	 * because host guarantees hypercall will eventually succeed.
>  	 */
> -	while (retries < 20) {
> +	while (1) {
>  		ret = hv_post_message(conn_id, 1, buffer, buflen);
>  
>  		switch (ret) {
> @@ -459,11 +458,11 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  			 * We could get this if we send messages too
>  			 * frequently.
>  			 */

Move the comment above the code it's commenting about.

		/* 
		 * We could get INVALID_CONNECTION_ID if we flood the
		 * host with too many messages.
		 */
		case HV_STATUS_INVALID_CONNECTION_ID:
		case HV_STATUS_INSUFFICIENT_MEMORY:
		case HV_STATUS_INSUFFICIENT_BUFFERS:
			break;



> -			ret = -EAGAIN;
> -			break;
>  		case HV_STATUS_INSUFFICIENT_MEMORY:
>  		case HV_STATUS_INSUFFICIENT_BUFFERS:
> -			ret = -ENOMEM;
> +			/*
> +			 * Temporary failure out of resources
> +			 */
>  			break;
>  		case HV_STATUS_SUCCESS:
>  			return ret;

			return 0;

Better to be more explicit.  When I looked at this I got briefly
confused if this function was supposed to return HV_ statuses or
standard kernel error codes.  It turns out that HV_STATUS_SUCCESS is
zero the success returns map directly to linux kernel code for success
but it's clearer to be explicit.

> @@ -472,12 +471,12 @@ int vmbus_post_msg(void *buffer, size_t buflen)
>  			return -EINVAL;
>  		}
  
> -		retries++;
>  		udelay(usec);
>  		if (usec < 2048)
>  			usec *= 2;
>  	}
> -	return ret;
> +	/* Impossible to get here */
> +	BUG_ON(1);

Remove the comment and the BUG_ON().

regards,
dan carpenter

RE: [PATCH] Retry infinitely for hypercall

[Long Li]

> -----Original Message-----
> From: Greg KH [mailto:greg@kroah.com]
> Sent: Wednesday, January 4, 2017 12:51 PM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] Retry infinitely for hypercall
> 
> On Wed, Jan 04, 2017 at 02:39:31PM -0800, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > Hyper-v host guarantees that a hypercall will succeed. Retry infinitely to
> avoid returning transient failures to upper layer.
> 
> Please wrap your changelog at the proper column.

Will do in V2.
> 
> And what happens when the hypercall does not succeed?  How is the kernel
> going to recover from that?

Sorry I should have used better wording in the patch. It should be "Retry infinitely on transient failures for hypercall". The host guarantees that it will return something other than transient failures in a reasonable small time frame. I will fix the comment in V2.

> 
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  drivers/hv/connection.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c index
> > 6ce8b87..4bcb099 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -439,7 +439,6 @@ int vmbus_post_msg(void *buffer, size_t buflen)  {
> >  	union hv_connection_id conn_id;
> >  	int ret = 0;
> > -	int retries = 0;
> >  	u32 usec = 1;
> >
> >  	conn_id.asu32 = 0;
> > @@ -447,10 +446,10 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >
> >  	/*
> >  	 * hv_post_message() can have transient failures because of
> > -	 * insufficient resources. Retry the operation a couple of
> > -	 * times before giving up.
> > +	 * insufficient resources. We retry infinitely on these failures
> > +	 * because host guarantees hypercall will eventually succeed.
> >  	 */
> > -	while (retries < 20) {
> > +	while (1) {
> >  		ret = hv_post_message(conn_id, 1, buffer, buflen);
> >
> >  		switch (ret) {
> > @@ -459,11 +458,11 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >  			 * We could get this if we send messages too
> >  			 * frequently.
> >  			 */
> > -			ret = -EAGAIN;
> > -			break;
> 
> Document you are falling through please, otherwise someone will "fix"
> this later.
Will add comment in V2.

> 
> >  		case HV_STATUS_INSUFFICIENT_MEMORY:
> >  		case HV_STATUS_INSUFFICIENT_BUFFERS:
> > -			ret = -ENOMEM;
> > +			/*
> > +			 * Temporary failure out of resources
> > +			 */
> >  			break;
> >  		case HV_STATUS_SUCCESS:
> >  			return ret;
> > @@ -472,12 +471,12 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >  			return -EINVAL;
> >  		}
> >
> > -		retries++;
> >  		udelay(usec);
> >  		if (usec < 2048)
> >  			usec *= 2;
> >  	}
> > -	return ret;
> > +	/* Impossible to get here */
> > +	BUG_ON(1);
> 
> If it is impossible, why do you have this line at all?

I will remove this line. There is no way for the code to get here.

> 
> What is this trying to solve?  Do you need to increase the time spent waiting?
> We all know things break, please allow the kernel to stay alive if at all
> possible.

The purpose is to wait until the host returns a non-transient status code for a hypercall. However, we don't know how many transient failures we are getting before the host returns a final status code. So use the infinite loop to wait until the host returns the final status code.

Thanks for reviewing. I will send V2 to address the comment.

Long

> 
> thanks,
> 
> greg k-h

RE: [PATCH] Retry infinitely for hypercall

[Long Li]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Wednesday, January 4, 2017 1:48 PM
> To: Long Li <longli@microsoft.com>
> Cc: KY Srinivasan <kys@microsoft.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; devel@linuxdriverproject.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] Retry infinitely for hypercall
> 
> Fix the subsystem prefix in the subject.
> 
> On Wed, Jan 04, 2017 at 02:39:31PM -0800, Long Li wrote:
> > From: Long Li <longli@microsoft.com>
> >
> > Hyper-v host guarantees that a hypercall will succeed. Retry infinitely to
> avoid returning transient failures to upper layer.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >  drivers/hv/connection.c | 17 ++++++++---------
> >  1 file changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c index
> > 6ce8b87..4bcb099 100644
> > --- a/drivers/hv/connection.c
> > +++ b/drivers/hv/connection.c
> > @@ -439,7 +439,6 @@ int vmbus_post_msg(void *buffer, size_t buflen)  {
> >  	union hv_connection_id conn_id;
> >  	int ret = 0;
> 
> Btw, when you disable GCC's uninitialized variable checking by storing bogus
> values in "ret", it's eventually going to bite you in the bum.
> Eventually you're going to get a bug that should have been detected through
> static analysis if only you hadn't disabled it.
> 
> > -	int retries = 0;
> >  	u32 usec = 1;
> >
> >  	conn_id.asu32 = 0;
> > @@ -447,10 +446,10 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >
> >  	/*
> >  	 * hv_post_message() can have transient failures because of
> > -	 * insufficient resources. Retry the operation a couple of
> > -	 * times before giving up.
> > +	 * insufficient resources. We retry infinitely on these failures
> > +	 * because host guarantees hypercall will eventually succeed.
> >  	 */
> > -	while (retries < 20) {
> > +	while (1) {
> >  		ret = hv_post_message(conn_id, 1, buffer, buflen);
> >
> >  		switch (ret) {
> > @@ -459,11 +458,11 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >  			 * We could get this if we send messages too
> >  			 * frequently.
> >  			 */
> 
> Move the comment above the code it's commenting about.
> 
> 		/*
> 		 * We could get INVALID_CONNECTION_ID if we flood the
> 		 * host with too many messages.
> 		 */
> 		case HV_STATUS_INVALID_CONNECTION_ID:
> 		case HV_STATUS_INSUFFICIENT_MEMORY:
> 		case HV_STATUS_INSUFFICIENT_BUFFERS:
> 			break;
> 
> 
> 
> > -			ret = -EAGAIN;
> > -			break;
> >  		case HV_STATUS_INSUFFICIENT_MEMORY:
> >  		case HV_STATUS_INSUFFICIENT_BUFFERS:
> > -			ret = -ENOMEM;
> > +			/*
> > +			 * Temporary failure out of resources
> > +			 */
> >  			break;
> >  		case HV_STATUS_SUCCESS:
> >  			return ret;
> 
> 			return 0;
> 
> Better to be more explicit.  When I looked at this I got briefly confused if this
> function was supposed to return HV_ statuses or standard kernel error
> codes.  It turns out that HV_STATUS_SUCCESS is zero the success returns
> map directly to linux kernel code for success but it's clearer to be explicit.
> 
> > @@ -472,12 +471,12 @@ int vmbus_post_msg(void *buffer, size_t buflen)
> >  			return -EINVAL;
> >  		}
> 
> > -		retries++;
> >  		udelay(usec);
> >  		if (usec < 2048)
> >  			usec *= 2;
> >  	}
> > -	return ret;
> > +	/* Impossible to get here */
> > +	BUG_ON(1);
> 
> Remove the comment and the BUG_ON().
> 
> regards,
> dan carpenter

Thanks, I will fix those in V2.

Long

[PATCH] Retry infinitely for hypercall

[Long Li]

From: Long Li <longli@microsoft.com>

Hyper-v host guarantees that a hypercall will succeed. Retry infinitely to avoid returning transient failures to upper layer.

Signed-off-by: Long Li <longli@microsoft.com>
---
 drivers/hv/connection.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/hv/connection.c b/drivers/hv/connection.c
index 6ce8b87..4bcb099 100644
--- a/drivers/hv/connection.c
+++ b/drivers/hv/connection.c
@@ -439,7 +439,6 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 {
 	union hv_connection_id conn_id;
 	int ret = 0;
-	int retries = 0;
 	u32 usec = 1;
 
 	conn_id.asu32 = 0;
@@ -447,10 +446,10 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 
 	/*
 	 * hv_post_message() can have transient failures because of
-	 * insufficient resources. Retry the operation a couple of
-	 * times before giving up.
+	 * insufficient resources. We retry infinitely on these failures
+	 * because host guarantees hypercall will eventually succeed.
 	 */
-	while (retries < 20) {
+	while (1) {
 		ret = hv_post_message(conn_id, 1, buffer, buflen);
 
 		switch (ret) {
@@ -459,11 +458,11 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 			 * We could get this if we send messages too
 			 * frequently.
 			 */
-			ret = -EAGAIN;
-			break;
 		case HV_STATUS_INSUFFICIENT_MEMORY:
 		case HV_STATUS_INSUFFICIENT_BUFFERS:
-			ret = -ENOMEM;
+			/*
+			 * Temporary failure out of resources
+			 */
 			break;
 		case HV_STATUS_SUCCESS:
 			return ret;
@@ -472,12 +471,12 @@ int vmbus_post_msg(void *buffer, size_t buflen)
 			return -EINVAL;
 		}
 
-		retries++;
 		udelay(usec);
 		if (usec < 2048)
 			usec *= 2;
 	}
-	return ret;
+	/* Impossible to get here */
+	BUG_ON(1);
 }
 
 /*
-- 
2.7.4