On 25.07.22 13:27, Alexander Atanasov wrote:
> Hi,
>
> On 18/07/2022 14:35, David Hildenbrand wrote:
>> On 14.07.22 15:20, Alexander Atanasov wrote:
>>> Allow the guest to know how much it is ballooned by the host.
>>> It is useful when debugging out of memory conditions.
>>>
>>> When host gets back memory from the guest it is accounted
>>> as used memory in the guest but the guest have no way to know
>>> how much it is actually ballooned.
>>>
>>> Signed-off-by: Alexander Atanasov <alexander.atanasov@xxxxxxxxxxxxx>
>>> ---
>>> drivers/virtio/virtio_balloon.c | 79 +++++++++++++++++++++++++++++
>>> include/uapi/linux/virtio_balloon.h | 1 +
>>> 2 files changed, 80 insertions(+)
>>>
>>> V2:
>>> - fixed coding style
>>> - removed pretty print
>>> V3:
>>> - removed dublicate of features
>>> - comment about balooned_pages more clear
>>> - convert host pages to balloon pages
>>> V4:
>>> - added a define for BALLOON_PAGE_SIZE to make things clear
>>> V5:
>>> - Made the calculatons work properly for both ways of memory accounting
>>> with or without deflate_on_oom
>>> - dropped comment
>>>
> [....]
>>> + u32 num_pages, total_pages, current_pages;
>>> + struct sysinfo i;
>>> +
>>> + si_meminfo(&i);
>>> +
>>> + seq_printf(f, "%-22s: %d\n", "page_size", VIRTIO_BALLOON_PAGE_SIZE);
>> Why? Either export all in ordinary page size or in kB. No need to
>> over-complicate the interface with a different page size that users
>> don't actually care about.
>>
>> I'd just stick to /proc/meminfo and use kB.
>
> The point is to expose some internal balloon data. Balloon works with
> pages not with kB and users can easily calculate kB.
Pages translate to KB. kB are easy to consume by humans instead of pages
with variable apge sizes
>
>>> +
>>> + seq_printf(f, "%-22s: %u\n", "ballooned_pages", num_pages);
>>> +
>>> + if (virtio_has_feature(b->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM)) {
>>> + total_pages = guest_to_balloon_pages(i.totalram);
>>> + current_pages = guest_to_balloon_pages(i.totalram) - num_pages;
>>> + } else {
>>> + total_pages = guest_to_balloon_pages(i.totalram) + num_pages;
>>> + current_pages = guest_to_balloon_pages(i.totalram);
>>> + }
>>> +
>>> + /* Total Memory for the guest from host */
>>> + seq_printf(f, "%-22s: %u\n", "total_pages", total_pages);
>>> +
>>> + /* Current memory for the guest */
>>> + seq_printf(f, "%-22s: %u\n", "current_pages", current_pages);
>> The think I detest about that interface (total/current) is that it's
>> simply not correct -- because i.totalram for example doesn't include
>> things like (similar to MemTotal in /proc/meminfo)
>>
>> a) crashkernel
>> b) early boot allocations (e.g., memmap)
>> c) any kind of possible memory (un)hotplug in the future
>>
>> I'd really suggest to just KIS and not mess with i.totalram.
>>
>> Exposing how much memory is inflated and some way to identify how that
>> memory is accounted in /proc/meminfo should be good enough.
>>
>> E.g., the output here could simply be
>>
>> "Inflated: 1024 kB"
>> "MemTotalReduced: 1024 kB"
>>
>> That would even allow in the future for flexibility when it comes to how
>> much/what was actually subtracted from MemTotal.
>
>
> The point of the debug interface is to expose some of the balloon driver
> internals to the guest.
>
> The users of this are user space processes that try to work according to
> the memory pressure and nested virtualization.
>
> I haven't seen one userspace software that expects total ram to change,
> it should be constant with one exception hotplug. But if you do hotplug
> RAM you know that and you can restart what you need to restart.
>
> So instead of messing with totalram with adding or removing pages /it
> would even be optimization since now it is done page by page/ i suggest
> to just account the inflated memory as used as in the deflate_on_oom
> case now. It is confusing and inconsistent as it is now. How to explain
> to a vps user why his RAM is constantly changing?
>
> And the file can go just to
>
> inflated: <pages>
>
> ballon_page_size: 4096
>
> or
>
> inflated: kB
>
> I prefer pages because on theory guest and host can different size and
> if at some point guest starts to make requests to the host for pages it
> must somehow know what the host/balloon page is. Since you have all the
> information at one place it is easy to calculate kB. But you can not
> calculate pages from only kB.
The host can only inflate in guest-page base sizes. How that translates
to host-page base sizes is absolutely irrelevant.
Who should care about pages? Absolutely irrelevant.
Guest pages: kB / getpagesize()
Logical balloon pages: kB / 4k
Host pages: ???
>
> Later on when hotplug comes it can add it's own data to the file so it
> can be used to see the amount that is added to the total ram.
>
> It can add
>
> hotplugged: <pages>
>
>
> What do you think?
Let's keep this interface simple, please.
--
Thanks,
David / dhildenb
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
