Re: [Qemu-devel] [PATCH 0/3] make bh safe with hot-unplug

[liu ping fan]

On Wed, Jun 26, 2013 at 2:34 PM, Paolo Bonzini <address@hidden> wrote:
> Il 26/06/2013 04:59, liu ping fan ha scritto:
>>> The latter part could be the hard one in a multi-threaded context, but I
>>> think it's up to the device to ensure it.  It doesn't _have_ to be hard.
>>>  For example, joining the data-plane thread would do that as well.
>>>
>> It seems not easy, take consideration of the sequence:
>>     _bh_delete(), // ensure not scheduled, ie, no further access to 
>> DeviceState
>>     wait for rcu grace period to come, // ensure not running
>>     free DeviceState in rcu-reclaimer
>> But  in current code, _delete() can be placed in DeviceState
>> finalization path(in reclaimer), which means while reclaiming, there
>> still exist access path to the DeviceState.
>
> It can be placed there, but it doesn't mean it is a good idea. :)
>
Unfortunately , these is what current code does, otherwise, they
should _bh_new() each time when scheduling bh, and _bh_delete() in
bh's callback, i.e. callback is the exact place to know we have done
with bh. (The other place is the destroy of DeviceState)

>> On the other hand, pushing _delete() out of  finalization path is not
>> easy, since we do not what time the DeviceState has done with its bh.
>
> See above:
>
> - if the BH will run in the iothread, the BH is definitely not running
> (because the BH runs under the BQL, and the reclaimer owns it)
>
It works for the case in which the DeviceState's reclaimer calls
_bh_delete(). But in another case(also exist in current code), where
we call _bh_delete() in callback, the bh will be scheduled, and oops!

> - if the BH is running in another thread, waiting for that thread to
> terminate obviously makes the BH not running.
>
This imply that except for qemu_aio_context, AioContext can be only
shared by one device, right? Otherwise we can not just terminate the
thread. If it is true, why can not we have more than one just like
qemu_aio_context?

> What we need is separation of removal and reclamation.  Without that,
> any effort to make things unplug-safe is going to be way way more
> complicated than necessary.
>
Agree, but when I tried for bh, I found the separation of removal and
reclamation are not easy.  We can not _bh_delete() in
acpi_piix_eject_slot(), because mmio-dispatch can schedule a bh at the
same time. And as explained, only two places can hold the
_bh_delete().
In a short word, with rcu, we need to constrain the calling idiom of
bh, i.e., putting them in reclaimer.  On the other hand, my patch try
to leave the calling idiom as it is, and handle this issue inside bh.

Regards,
Pingfan

> Paolo