> -----Original Message----- > From: Jeremiah Mahler [mailto:jmmah...@gmail.com] > Sent: Wednesday, December 10, 2014 6:10 PM > To: KY Srinivasan > Cc: gre...@linuxfoundation.org; linux-ker...@vger.kernel.org; > de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; > jasow...@redhat.com; mc...@ipxe.org > 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 <mc...@ipxe.org> for seeing this bug. > > > > Signed-off-by: K. Y. Srinivasan <k...@microsoft.com> > > Reported-by: Michael Brown <mc...@ipxe.org> > > --- > > 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 majord...@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > -- > - Jeremiah Mahler _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
