> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpen...@oracle.com] > Sent: Wednesday, January 4, 2017 1:48 PM > To: Long Li <lon...@microsoft.com> > Cc: KY Srinivasan <k...@microsoft.com>; Haiyang Zhang > <haiya...@microsoft.com>; de...@linuxdriverproject.org; linux- > ker...@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 <lon...@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 <lon...@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
