1. Patchew
  2. QEMU
  3. [PATCH v1 0/3] virtio-balloon: free page hinting fixes
  4. View patch

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

David Hildenbrand posted 3 patches 5 years, 6 months ago
There is a newer version of this series
[PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by David Hildenbrand 5 years, 6 months ago 
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 <wei.w.wang@intel.com>
Cc: Alexander Duyck <alexander.duyck@gmail.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: David Hildenbrand <david@redhat.com>
---
 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);
     }
-- 
2.25.4
Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by Philippe Mathieu-Daudé 5 years, 6 months ago 
On 5/18/20 10:37 AM, David Hildenbrand 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 <wei.w.wang@intel.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   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);
>       }
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by Alexander Duyck 5 years, 6 months ago 
On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> 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 <wei.w.wang@intel.com>
> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>  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.
Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by David Hildenbrand 5 years, 6 months ago 
On 18.05.20 17:35, Alexander Duyck wrote:
> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> 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 <wei.w.wang@intel.com>
>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  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
Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by Philippe Mathieu-Daudé 5 years, 6 months ago 
+Stefan/Paolo in case they have a better clue.

On 5/18/20 5:41 PM, David Hildenbrand wrote:
> On 18.05.20 17:35, Alexander Duyck wrote:
>> On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> 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 <wei.w.wang@intel.com>
>>> Cc: Alexander Duyck <alexander.duyck@gmail.com>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>> ---
>>>   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).
>
Re: [PATCH v1 3/3] virtio-balloon: unref the iothread when unrealizing
Posted by Alexander Duyck 5 years, 6 months ago 
On Mon, May 18, 2020 at 8:42 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 18.05.20 17:35, Alexander Duyck wrote:
> > On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <david@redhat.com> 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 <wei.w.wang@intel.com>
> >> Cc: Alexander Duyck <alexander.duyck@gmail.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> >> Signed-off-by: David Hildenbrand <david@redhat.com>
> >> ---
> >>  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).

Agreed. This is probably fine as is.

Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.