RE: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in vmbus_establish_gpadl()

[KY Srinivasan]

> -----Original Message-----
> From: Jeremiah Mahler [mailto:jmmahler@xxxxxxxxx]
> Sent: Wednesday, December 10, 2014 6:10 PM
> To: KY Srinivasan
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> jasowang@xxxxxxxxxx; mcb30@xxxxxxxx
> Subject: Re: [PATCH V3 1/1] Drivers: hv: vmbus: Fix a bug in
> vmbus_establish_gpadl()
> 
> K. Y. Srinivasan,
> 
> On Wed, Dec 10, 2014 at 05:13:00PM -0800, K. Y. Srinivasan wrote:
> > Correctly compute the local (gpadl) handle.
> 
> This description is still too sparse for me.  How was it computed before and
> why was this incorrect?  Pretend like you are trying to explain your patch to
> someone who has no idea what you did.
> 
> > I would like to thank Michael Brown <mcb30@xxxxxxxx> for seeing this bug.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > Reported-by: Michael Brown <mcb30@xxxxxxxx>
> > ---
> > Changes in V2: Added the Reported-by tag.
> > Changes in V3: Cleaned up the commit log.
> >
> >  drivers/hv/channel.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index
> > 433f72a..c76ffbe 100644
> > --- a/drivers/hv/channel.c
> > +++ b/drivers/hv/channel.c
> > @@ -366,8 +366,8 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
> >  	unsigned long flags;
> >  	int ret = 0;
> >
> > -	next_gpadl_handle =
> atomic_read(&vmbus_connection.next_gpadl_handle);
> > -	atomic_inc(&vmbus_connection.next_gpadl_handle);
> > +	next_gpadl_handle =
> > +
> 	(atomic_inc_return(&vmbus_connection.next_gpadl_handle) - 1);
> >
> Tell me if I understand this correctly.
> 
> Before it read the handle and incremented it.
> 
>   y = x + 1
> 
> Now it reads the handle, increments it, then decrements it.
> 
>   y = (x + 1) - 1 = x

This code can be executed concurrently on multiple CPUs. We want to ensure that each call to
establish gpadl gets a unique local handle. The earlier code was buggy in that we would read the
handle and then atomically increment it. Thus, multiple CPUs could read the identical current
value which would be their local handle. What we want is the ability to atomically read and increment
the value - this would ensure that each caller got a unique value even if they executed the code
concurrently on multiple CPUs. The API atomic_inc_return(), atomically increments and returns the
incremented value. We locally decrement this value to emulate the logic of "read the current value and
atomically increment the value.

Hope this helps,

K. Y
> 
> >  	ret = create_gpadl_header(kbuffer, size, &msginfo, &msgcount);
> >  	if (ret)
> > --
> > 1.7.4.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe
> > linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> - Jeremiah Mahler
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/