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

[Paolo Bonzini <pbonzini@redhat.com>]

Il 26/06/2013 11:44, liu ping fan ha scritto:
> On Wed, Jun 26, 2013 at 4:38 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 26/06/2013 10:20, liu ping fan ha scritto:
>>>>> 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!
>>
>> But you may know that the BH is not scheduled after removal, too.
>>
> No, the removal can run in parallel with the other mmio-dispatch which
> can resort to schedule a bh.

But then behavior is more or less undefined.  Of course the device must
ensure that something sane happens, but that's the responsibility of the
device, not of the generic layer.

> I.e, the removal can not call
> _bh_delete(), so it do not know whether bh will be scheduled or not.

It can call qemu_bh_delete(), then all concurrent qemu_bh_schedule()
will be no-ops.

>> If you look at the memory work, for example, the owner patches happen to
>> be useful for BQL-less dispatch too, but they are solving existing
>> hot-unplug bugs.
>>
> Oh, I will read it again, since I had thought the owner is only used
> for the purpose of refcnt.

It is.  But it goes beyond BQL-less dispatch.  Anything that gives away
the BQL between a map and unmap (including asynchronous I/O) needs an
owner.  We have ignored that until now because we do not have memory unplug.

>>>> 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.
>>
>> acpi_piix_eject_slot() is removal, not reclamation.  The plan there is
>> that qdev_free calls the exit callback immediately (which can do
>> qemu_bh_delete), and schedules an unref after the next RCU grace period.
>>  At the next RCU grace period the BH will not be running, thus it is
>> safe to finalize the object.
>>
> I have two question:
> 1st, who trigger qdev_free?  //Not figuring out the total design, but
> I feel it is quite different from kernel's design, where refcnt->0,
> then exit is called.

qdev_free is triggered by the guest, but free is a misnomer.  It is
really "make it inaccessible from the guest and management" (the kernel
equivalent would be removal of /dev and /sys entries, for example).  The
actual "free" will happen later.

> 2nd, _delete_bh() means even there is any not-severed request, the
> request will be canceled. Is it legal, and will not lose data(not
> sure, since I do not know who will call qdev_free)?

That's something that should be ensured by the device.  For example it
is not a problem to cancel virtio-net's tx_bh.  If it is a problem, the
device must do something else.  It could even be a bdrv_drain_all() in
the worst case.

Paolo