Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing

[David Hildenbrand]

On 18.05.20 17:35, Alexander Duyck wrote:
> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <address@hidden> wrote:
>>
>> We took a reference when realizing, so let's drop that reference when
>> unrealizing.
>>
>> Fixes: c13c4153f76d ("virtio-balloon: VIRTIO_BALLOON_F_FREE_PAGE_HINT")
>> Cc: Wei Wang <address@hidden>
>> Cc: Alexander Duyck <address@hidden>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Cc: Philippe Mathieu-Daudé <address@hidden>
>> Signed-off-by: David Hildenbrand <address@hidden>
>> ---
>>  hw/virtio/virtio-balloon.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
>> index a4fcf2d777..3f8fc50be0 100644
>> --- a/hw/virtio/virtio-balloon.c
>> +++ b/hw/virtio/virtio-balloon.c
>> @@ -820,6 +820,7 @@ static void virtio_balloon_device_unrealize(DeviceState 
>> *dev)
>>
>>      if (s->free_page_bh) {
>>          qemu_bh_delete(s->free_page_bh);
>> +        object_unref(OBJECT(s->iothread));
>>          virtio_balloon_free_page_stop(s);
>>          precopy_remove_notifier(&s->free_page_report_notify);
>>      }
> 
> I'm not entirely sure about this order of operations. It seems like it
> would make more sense to remove the notifier, stop the hinting, delete
> the bh, and then release the IO thread.

This is the reverse order of the steps in
virtio_balloon_device_realize(). And I guess it should be fine. The
notifier cannot really be active/trigger while we are removing devices
(cannot happen with concurrent migration). After qemu_bh_delete(), the
iothread is effectively unused.

I am unsure about many things regarding free page hinting (e.g., if the
virtio_balloon_free_page_stop() is of any use while we are ripping out
the device and it will be gone in a second).

-- 
Thanks,

David / dhildenb