Re: [Qemu-devel] [RFC 5/5] virtio-balloon: Safely handle BALLOON_PAGE_SIZE < host page size

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4813 bytes --]

On Wed, Oct 17, 2018 at 11:56:09AM +0200, David Hildenbrand wrote:
> >>>> I am pretty sure that you can create misleading warnings in case you
> >>>> migrate at the wrong time. (migrate while half the 64k page is inflated,
> >>>> on the new host the other part is inflated - a warning when switching to
> >>>> another 64k page).
> >>>
> >>> Yes we can get bogus warnings across migration with this.  I was
> >>> considering that an acceptable price, but I'm open to better
> >>> alternatives.
> >>
> >> Is maybe reporting a warning on a 64k host when realizing the better
> >> approach than on every inflation?
> >>
> >> "host page size does not match virtio-balloon page size. If the guest
> >> has a different page size than the host, inflating the balloon might not
> >> effectively free up memory."
> > 
> > That might work - I'll see what I can come up with.  One complication
> > is that theoretically at least, you can have multiple host page sizes
> > (main memory in normal pages, a DIMM in hugepages).  That makes the
> > condition on which the warning should be issued a bit fiddly to work
> > out.
> 
> I assume issuing a warning on these strange systems would not hurt after
> all. ("there is a chance this might not work")

Sure.

> >> Or reporting a warning whenever changing the balloon target size.
> > 
> > I'm not sure what you mean by this.
> 
> I mean when setting e.g. qmp_balloon() to something != 0. This avoids a
> warning when a virtio-balloon device is silently created (e.g. by
> libvirt?) but never used.

Ah, right.  Yeah, that's a good idea.

> Checking in virtio_balloon_to_target would be sufficient I guess.
> 
> > 
> >>
> >>>
> >>>>> +        free(balloon->pbp);
> >>>>> +        balloon->pbp = NULL;
> >>>>>      }
> >>>>>  
> >>>>> -    ram_block_discard_range(rb, ram_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 */
> >>>>> +    if (!balloon->pbp) {
> >>>>> +        /* 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 = host_page_base;
> >>>>> +    }
> >>>>> +
> >>>>> +    bitmap_set(balloon->pbp->bitmap,
> >>>>> +               (ram_offset - balloon->pbp->base) / BALLOON_PAGE_SIZE,
> >>>>> +               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);
> >>>>> +        /* We ignore errors from ram_block_discard_range(), because it
> >>>>> +         * has already reported them, and failing to discard a balloon
> >>>>> +         * page is not fatal */
> >>>>> +
> >>>>> +        free(balloon->pbp);
> >>>>> +        balloon->pbp = NULL;
> >>>>> +    }
> >>>>>  }
> >>>> No, not a fan of this approach.
> >>>
> >>> I can see why, but I really can't see what else to do without breaking
> >>> existing, supported, working (albeit by accident) setups.
> >>
> >> Is there any reason to use this more complicated "allow random freeing"
> >> approach over a simplistic sequential freeing I propose? Then we can
> >> simply migrate the last freed page and should be fine.
> > 
> > Well.. your approach is probably simpler in terms of the calculations
> > that need to be done, though only very slightly.  I think my approach
> > is conceptually clearer though, since we're explicitly checking for
> > exactly the condition we need, rather than something we thing should
> > match up with that condition.
> 
> I prefer to keep it simple where possible. We expect sequential freeing,
> so it's easy to implement with only one additional uint64_t that can be
> easily migrated. Having to use bitmaps + alloc/free is not really needed.
> 
> If you insist, at least try to get rid of the malloc to e.g. simplify
> migration.

I don't think there's really anything to do for migration; we already
effectively lose the state of the balloon across migration.  I think
it's fine to lose a little extra transitional state.

> (otherwise, please add freeing code on unrealize(), I guess
> you are missing that right now)

Ah, good point, I need to fix that.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]