RE: [PATCH 0/6] Drivers: hv: vmbus

[KY Srinivasan]

> -----Original Message-----
> From: Dexuan Cui
> Sent: Sunday, February 15, 2015 7:19 PM
> To: KY Srinivasan; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; vkuznets@xxxxxxxxxx
> Subject: RE: [PATCH 0/6] Drivers: hv: vmbus
> 
> > -----Original Message-----
> > From: devel [mailto:driverdev-devel-bounces@xxxxxxxxxxxxxxxxxxxxxx] On
> > Behalf Of K. Y. Srinivasan
> > Sent: Monday, February 16, 2015 4:11 AM
> > To: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> > vkuznets@xxxxxxxxxx
> > Subject: [PATCH 0/6] Drivers: hv: vmbus
> >
> > The host can rescind an offer any time after the offer has been made
> > to the guest. This patch-set cleans up how we handle rescind messages
> > from the host.
> >
> >
> > K. Y. Srinivasan (6):
> >   Drivers: hv: vmbus: Properly handle child device remove
> >   Drivers: hv: vmbus: Introduce a function to remove a rescinded offer
> >   Drivers: hv: vmbus: Handle both rescind and offer messages in the
> >     same context
> >   Drivers: hv: vmbus: Remove the channel from the channel list(s) on
> >     failure
> >   Drivers: hv: util: On device remove, close the channel after
> >     de-initializing the service
> >   Drivers: hv: vmbus: Get rid of some unnecessary messages
> >
> >  drivers/hv/channel.c      |    9 ++++
> >  drivers/hv/channel_mgmt.c |   95 ++++++++++++++++++++-----------------
> -------
> >  drivers/hv/connection.c   |    7 +---
> >  drivers/hv/hv_util.c      |    2 +-
> >  drivers/hv/vmbus_drv.c    |   26 +++++++++---
> >  include/linux/hyperv.h    |    1 +
> >  6 files changed, 74 insertions(+), 66 deletions(-)
> >
> > --
> 
> The patchset seems good to me.
> Reviewed-by: Dexuan Cui <decui@xxxxxxxxxxxxx>

Dexuan,

Thank you for the review.
> 
> BTW, IMO we need one more patch to remove the queue_work() in
> free_channel() -- just make it an ordinary synchronous function call:
> 
> vmbus_process_offer() can invoke free_channel() on error path, and
> vmbus_process_rescind() can invoke free_channel() too.
> We should exclude the possible race.


I don't see the race; free_channel is only called after ensuring the channel cannot be discovered
by any other context. Note that we are now executing both rescind and the offer message in the 
same work context. With this patch-set, there are only 3 call sites for free_channel:
1.  hv_process_channel_removal()
2. vmbus_free_channels()
3. vmbus_process_offer()

If vmbus_process_offer() calls free_channel, the channel cannot be discovered via its ID as we remove
The chanel from the global as well as the per-cpu lists. In this case, the channel is destroyed and nobody
can get a reference to it.
> 
> And now the controlwq and work fields of struct vmbus_channel are useless
> now.

Yes; we can get rid of this now. I will have that in a separate patch.

Regards,

K. Y


--
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/