Re: [PATCH v5 1/1] Create debugfs file with virtio balloon usage information

[Alexander Atanasov <alexander.atanasov@virtuozzo.com>]

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@virtuozzo.com>
>> ---
>>   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.

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

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?


>> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
>> index ddaa45e723c4..f3ff7c4e5884 100644
>> --- a/include/uapi/linux/virtio_balloon.h
>> +++ b/include/uapi/linux/virtio_balloon.h
>> @@ -40,6 +40,7 @@
>>   
>>   /* Size of a PFN in the balloon interface. */
>>   #define VIRTIO_BALLOON_PFN_SHIFT 12
>> +#define VIRTIO_BALLOON_PAGE_SIZE (1<<VIRTIO_BALLOON_PFN_SHIFT)
> We prefer extra spacing
>
>   (1 << VIRTIO_BALLOON_PFN_SHIFT)


Ok, i am working on my  bad habits - thanks! I'll address the style 
issues and variable naming in the next version along with the future 
feedback.

-- 
Regards,
Alexander Atanasov