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

[David Hildenbrand <david@redhat.com>]

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

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

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. (otherwise, please add freeing code on unrealize(), I guess
you are missing that right now)

-- 

Thanks,

David / dhildenb