| Message ID | 20200518083704.52646-4-david@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-balloon: free page hinting fixes | expand |
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>
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.
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).
+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). >
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>
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); }
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(+)