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

[Alexander Duyck]

On Mon, May 18, 2020 at 8:42 AM David Hildenbrand <address@hidden> wrote:
>
> 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).

Agreed. This is probably fine as is.

Reviewed-by: Alexander Duyck <address@hidden>