RE: [PATCH V2 10/12] Drivers: hv: vmbus: channge vmbus_connection.channel_lock to mutex

[Dexuan Cui]

> From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx]
> Sent: Thursday, November 12, 2015 18:41
> To: Dexuan Cui <decui@xxxxxxxxxxxxx>
> Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx; apw@xxxxxxxxxxxxx;
> jasowang@xxxxxxxxxx; KY Srinivasan <kys@xxxxxxxxxxxxx>
> Subject: Re: [PATCH V2 10/12] Drivers: hv: vmbus: channge
> vmbus_connection.channel_lock to mutex
>
> "K. Y. Srinivasan" <kys@xxxxxxxxxxxxx> writes:
>
> > From: Dexuan Cui <decui@xxxxxxxxxxxxx>
> >
> > spinlock is unnecessary here.
> > mutex is enough.
>
> Hm, mutex is usually required when we need to sleep and a spinlock is
> enough otherwise :-)

Sorry, I should have written a better changelog. :-)

 > Or are you trying to say we don't need to disable interrupts here? In

Yes.
Here we try to protect vmbus_connection.chn_list and the related
channel pointer (see relid2channel()) from being updated in parallel.

The parallel paths, e.g., vmbus_process_offer() and
vmbus_onoffer_rescind(), are in process context and no irq context is
involved, so we don't need disable interrupts at all.

> that can maybe we can just switch to spin_lock()/spin_unlock()?

Switching to mutex actually makes preparation for a later patch (which
will be posted to LKML once this pachset is accepted):

Drivers: hv: vmbus: add an API vmbus_hvsock_device_unregister()
https://github.com/dcui/linux/commit/185afe8394a9bdae2be11ee1ea2a38d05e373025
(on branch decui/vmsock_1020)

For a vmsock socket connection, the host and the guest can be closing
the connection at the same time.

When the host tries to close the connection, a rescind offer is received
in the VM.

When the guest tries to close the connection, a new vmbus API
vmbus_hvsock_device_unregister(channel) is invoked, so
vmbus_hvsock_device_unregister() -> vmbus_device_unregister() is
invoked and this can be running in parallel with
vmbus_onoffer_rescind() -> vmbus_device_unregister().

The issue of "vmbus_onoffer_rescind () -> relid2channel()" is:
it returns a channel pointer without the spinlock held, so actually
there is no real protection for the channel pointer.

So IMO we need to serialize vmbus_onoffer_rescind() and
vmbus_hvsock_device_unregister().
Here I use mutex (rather than spinlock) because the critical section
can sleep, due to vmbus_device_unregister().

Thanks,
-- Dexuan
--
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/