| Message ID | 20190725115914.26983-1-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-balloon: free pbp more aggressively | expand |
> -static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp) > +static void virtio_balloon_pbp_free(PartiallyBalloonedPage **pbp) > { > - if (!pbp) { > + if (!*pbp) { > return; > } > - g_free(pbp->bitmap); > - g_free(pbp); > + g_free(*pbp->bitmap); This has to be g_free((*pbp)->bitmap); to compile. > + g_free(*pbp); > + *pbp = NULL; > } > > static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa, > @@ -108,8 +109,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, > /* We've partially ballooned part of a host page, but now > * we're trying to balloon part of a different one. Too hard, > * give up on the old partial page */ > - virtio_balloon_pbp_free(*pbp); > - *pbp = NULL; > + virtio_balloon_pbp_free(pbp); > } > > if (!*pbp) { > @@ -127,8 +127,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, > /* We ignore errors from ram_block_discard_range(), because it > * has already reported them, and failing to discard a balloon > * page is not fatal */ > - virtio_balloon_pbp_free(*pbp); > - *pbp = NULL; > + virtio_balloon_pbp_free(pbp); > } > } > > @@ -379,9 +378,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) > virtqueue_push(vq, elem, offset); > virtio_notify(vdev, vq); > g_free(elem); > + virtio_balloon_pbp_free(&pbp); You could move the "PartiallyBalloonedPage *pbp = NULL;" now into the loop. > } > - > - virtio_balloon_pbp_free(pbp); > } > > static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq) > Gave it a quick test with a hugepage backing on x86 - still works. Would be good if somebody could test with 64k PPC guest. Reviewed-by: David Hildenbrand <david@redhat.com>
diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c index fe9664e42c..460a702463 100644 --- a/hw/virtio/virtio-balloon.c +++ b/hw/virtio/virtio-balloon.c @@ -39,13 +39,14 @@ typedef struct PartiallyBalloonedPage { unsigned long *bitmap; } PartiallyBalloonedPage; -static void virtio_balloon_pbp_free(PartiallyBalloonedPage *pbp) +static void virtio_balloon_pbp_free(PartiallyBalloonedPage **pbp) { - if (!pbp) { + if (!*pbp) { return; } - g_free(pbp->bitmap); - g_free(pbp); + g_free(*pbp->bitmap); + g_free(*pbp); + *pbp = NULL; } static PartiallyBalloonedPage *virtio_balloon_pbp_alloc(ram_addr_t base_gpa, @@ -108,8 +109,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, /* We've partially ballooned part of a host page, but now * we're trying to balloon part of a different one. Too hard, * give up on the old partial page */ - virtio_balloon_pbp_free(*pbp); - *pbp = NULL; + virtio_balloon_pbp_free(pbp); } if (!*pbp) { @@ -127,8 +127,7 @@ static void balloon_inflate_page(VirtIOBalloon *balloon, /* We ignore errors from ram_block_discard_range(), because it * has already reported them, and failing to discard a balloon * page is not fatal */ - virtio_balloon_pbp_free(*pbp); - *pbp = NULL; + virtio_balloon_pbp_free(pbp); } } @@ -379,9 +378,8 @@ static void virtio_balloon_handle_output(VirtIODevice *vdev, VirtQueue *vq) virtqueue_push(vq, elem, offset); virtio_notify(vdev, vq); g_free(elem); + virtio_balloon_pbp_free(&pbp); } - - virtio_balloon_pbp_free(pbp); } static void virtio_balloon_receive_stats(VirtIODevice *vdev, VirtQueue *vq)
Previous patches switched to a temporary pbp but that does not go far enough: after device uses a buffer, guest is free to reuse it, so tracking the page and freeing it later is wrong. Free and reset the pbp after we push each element. Fixes: ed48c59875b6 ("virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size") Cc: qemu-stable@nongnu.org #v4.0.0 Cc: David Hildenbrand <david@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- hw/virtio/virtio-balloon.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-)