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