On Mon, May 18, 2020 at 8:42 AM David Hildenbrand <da...@redhat.com> wrote:
>
> On 18.05.20 17:35, Alexander Duyck wrote:
> > On Mon, May 18, 2020 at 1:37 AM David Hildenbrand <da...@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.w...@intel.com>
> >> Cc: Alexander Duyck <alexander.du...@gmail.com>
> >> Cc: Michael S. Tsirkin <m...@redhat.com>
> >> Cc: Philippe Mathieu-Daudé <phi...@redhat.com>
> >> Signed-off-by: David Hildenbrand <da...@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.du...@linux.intel.com>
