Re: [Qemu-devel] [PATCH v1 3/3] virtio-balloon: Rework pbp tracking data

[David Gibson]

On Fri, Jul 19, 2019 at 06:01:20PM +0200, David Hildenbrand wrote:
> Using the address of a RAMBlock to test for a matching pbp is not really
> safe. Instead, let's use the guest physical address of the base page
> along with the page size (via the number of subpages).
> 
> While at it, move "struct PartiallyBalloonedPage" to virtio-balloon.h
> now (previously most probably avoided to te the ramblock), renaming it.
> 
> Also, let's only dynamically allocating the bitmap. This makes the code
> easier to read and maintain - we can reuse bitmap_new().
> 
> Signed-off-by: David Hildenbrand <address@hidden>

This mostly looks good, but one thing bothers me..

> ---
>  hw/virtio/virtio-balloon.c         | 52 +++++++++++++++++-------------
>  include/hw/virtio/virtio-balloon.h | 15 +++++++--
>  2 files changed, 42 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 29cee344b2..8e5f9459c2 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -34,23 +34,31 @@
>  
>  #define BALLOON_PAGE_SIZE  (1 << VIRTIO_BALLOON_PFN_SHIFT)
>  
> -struct PartiallyBalloonedPage {
> -    RAMBlock *rb;
> -    ram_addr_t base;
> -    unsigned long bitmap[];
> -};
> -
>  static void virtio_balloon_reset_pbp(VirtIOBalloon *balloon)
>  {
> -    g_free(balloon->pbp);
> -    balloon->pbp = NULL;
> +    balloon->pbp.base_gpa = 0;
> +    balloon->pbp.subpages = 0;
> +    g_free(balloon->pbp.bitmap);
> +    balloon->pbp.bitmap = NULL;
> +}
> +
> +static bool virtio_balloon_pbp_valid(VirtIOBalloon *balloon)
> +{
> +    return balloon->pbp.bitmap;
> +}
> +
> +static bool virtio_balloon_pbp_matches(VirtIOBalloon *balloon,
> +                                       ram_addr_t base_gpa, long subpages)
> +{
> +    return balloon->pbp.subpages == subpages &&
> +           balloon->pbp.base_gpa == base_gpa;

.. so, I'm trying to think what base_gpa matching, but subpages not
matching means.  I think that can only happen if a pbp is created,
then the ramblock it was in is unplugged, then another ramblock is
plugged in covering the same address and with a different (host) page
size.  Which is unlikely but possible, IIUC.  However, also possible
and marginally less unlikely is to plug a new block of the same page
size, in which case this *will* match, but presumably the pbp should
still be discarded.

>  }
>  
>  static void balloon_inflate_page(VirtIOBalloon *balloon,
>                                   MemoryRegion *mr, hwaddr mr_offset)
>  {
>      void *addr = memory_region_get_ram_ptr(mr) + mr_offset;
> -    ram_addr_t rb_offset, rb_aligned_offset;
> +    ram_addr_t rb_offset, rb_aligned_offset, base_gpa;
>      RAMBlock *rb;
>      size_t rb_page_size;
>      int subpages;
> @@ -81,32 +89,32 @@ static void balloon_inflate_page(VirtIOBalloon *balloon,
>  
>      rb_aligned_offset = QEMU_ALIGN_DOWN(rb_offset, rb_page_size);
>      subpages = rb_page_size / BALLOON_PAGE_SIZE;
> +    base_gpa = memory_region_get_ram_addr(mr) + mr_offset -
> +               (rb_offset - rb_aligned_offset);
>  
> -    if (balloon->pbp
> -        && (rb != balloon->pbp->rb
> -            || rb_aligned_offset != balloon->pbp->base)) {
> +    if (virtio_balloon_pbp_valid(balloon) &&
> +        !virtio_balloon_pbp_matches(balloon, base_gpa, subpages)) {
>          /* 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_reset_pbp(balloon);
>      }
>  
> -    if (!balloon->pbp) {
> +    if (!virtio_balloon_pbp_valid(balloon)) {
>          /* Starting on a new host page */
> -        size_t bitlen = BITS_TO_LONGS(subpages) * sizeof(unsigned long);
> -        balloon->pbp = g_malloc0(sizeof(PartiallyBalloonedPage) + bitlen);
> -        balloon->pbp->rb = rb;
> -        balloon->pbp->base = rb_aligned_offset;
> +        balloon->pbp.base_gpa = base_gpa;
> +        balloon->pbp.subpages = subpages;
> +        balloon->pbp.bitmap = bitmap_new(subpages);
>      }
>  
> -    set_bit((rb_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> -            balloon->pbp->bitmap);
> +    set_bit((rb_offset - rb_aligned_offset) / BALLOON_PAGE_SIZE,
> +            balloon->pbp.bitmap);
>  
> -    if (bitmap_full(balloon->pbp->bitmap, subpages)) {
> +    if (bitmap_full(balloon->pbp.bitmap, subpages)) {
>          /* We've accumulated a full host page, we can actually discard
>           * it now */
>  
> -        ram_block_discard_range(rb, balloon->pbp->base, rb_page_size);
> +        ram_block_discard_range(rb, rb_aligned_offset, rb_page_size);
>          /* We ignore errors from ram_block_discard_range(), because it
>           * has already reported them, and failing to discard a balloon
>           * page is not fatal */
> @@ -130,7 +138,7 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>      rb = qemu_ram_block_from_host(addr, false, &rb_offset);
>      rb_page_size = qemu_ram_pagesize(rb);
>  
> -    if (balloon->pbp) {
> +    if (virtio_balloon_pbp_valid(balloon)) {
>          /* Let's play safe and always reset the pbp on deflation requests. */
>          virtio_balloon_reset_pbp(balloon);
>      }
> diff --git a/include/hw/virtio/virtio-balloon.h 
> b/include/hw/virtio/virtio-balloon.h
> index 5a99293a45..0190d78d71 100644
> --- a/include/hw/virtio/virtio-balloon.h
> +++ b/include/hw/virtio/virtio-balloon.h
> @@ -33,8 +33,6 @@ typedef struct virtio_balloon_stat_modern {
>         uint64_t val;
>  } VirtIOBalloonStatModern;
>  
> -typedef struct PartiallyBalloonedPage PartiallyBalloonedPage;
> -
>  enum virtio_balloon_free_page_report_status {
>      FREE_PAGE_REPORT_S_STOP = 0,
>      FREE_PAGE_REPORT_S_REQUESTED = 1,
> @@ -42,6 +40,12 @@ enum virtio_balloon_free_page_report_status {
>      FREE_PAGE_REPORT_S_DONE = 3,
>  };
>  
> +typedef struct VirtIOPartiallyBalloonedPage {
> +    ram_addr_t base_gpa;
> +    long subpages;
> +    unsigned long *bitmap;
> +} VirtIOPartiallyBalloonedPage;
> +
>  typedef struct VirtIOBalloon {
>      VirtIODevice parent_obj;
>      VirtQueue *ivq, *dvq, *svq, *free_page_vq;
> @@ -70,7 +74,12 @@ typedef struct VirtIOBalloon {
>      int64_t stats_last_update;
>      int64_t stats_poll_interval;
>      uint32_t host_features;
> -    PartiallyBalloonedPage *pbp;
> +
> +    /*
> +     * Information about a partially ballooned page - does not have to be
> +     * migrated and has to be reset on every device reset.
> +     */
> +    VirtIOPartiallyBalloonedPage pbp;
>  
>      bool qemu_4_0_config_size;
>  } VirtIOBalloon;

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature