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

[David Hildenbrand <david@redhat.com>]

On 14.07.22 15:20, Alexander Atanasov wrote:
> Hello,
> 
> On 14/07/2022 14:35, David Hildenbrand wrote:
>> On 05.07.22 10:36, 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     | 77 +++++++++++++++++++++++++++++
>>>   include/uapi/linux/virtio_balloon.h |  1 +
>>>   2 files changed, 78 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
>>>
>>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>>> index b9737da6c4dd..dc4ad584b947 100644
>>> --- a/drivers/virtio/virtio_balloon.c
>>> +++ b/drivers/virtio/virtio_balloon.c
>>> @@ -10,6 +10,7 @@
>>>   #include <linux/virtio_balloon.h>
>>>   #include <linux/swap.h>
>>>   #include <linux/workqueue.h>
>>> +#include <linux/debugfs.h>
>>>   #include <linux/delay.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/module.h>
>>> @@ -731,6 +732,77 @@ static void report_free_page_func(struct work_struct *work)
>>>   	}
>>>   }
>>>   
>>> +/*
>>> + * DEBUGFS Interface
>>> + */
>>> +#ifdef CONFIG_DEBUG_FS
>>> +
>>> +#define guest_to_balloon_pages(i) ((i)*VIRTIO_BALLOON_PAGES_PER_PAGE)
>>> +/**
>>> + * virtio_balloon_debug_show - shows statistics of balloon operations.
>>> + * @f: pointer to the &struct seq_file.
>>> + * @offset: ignored.
>>> + *
>>> + * Provides the statistics that can be accessed in virtio-balloon in the debugfs.
>>> + *
>>> + * Return: zero on success or an error code.
>>> + */
>>> +
>>> +static int virtio_balloon_debug_show(struct seq_file *f, void *offset)
>>> +{
>>> +	struct virtio_balloon *b = f->private;
>>> +	u32 num_pages;
>>> +	struct sysinfo i;
>>> +
>>> +	si_meminfo(&i);
>>> +
>>> +	seq_printf(f, "%-22s: %d\n", "page_size", VIRTIO_BALLOON_PAGE_SIZE);
>>> +
>>> +	virtio_cread_le(b->vdev, struct virtio_balloon_config, actual,
>>> +			&num_pages);
>>> +	/*
>>> +	 * Pages allocated by host from the guest memory.
>>> +	 * Host inflates the balloon to get more memory.
>>> +	 * Guest needs to deflate the balloon to get more memory.
>>> +	 */
>> Please drop that comment. This is basic virtio-balloon operation that
>> must not be explained at this point.
> 
> Ok
> 
>>> +	seq_printf(f, "%-22s: %u\n", "ballooned_pages", num_pages);
>>> +
>>> +	/* Total Memory for the guest from host */
>>> +	seq_printf(f, "%-22s: %lu\n", "total_pages",
>>> +			guest_to_balloon_pages(i.totalram));
>> totalram is calculated from totalram_pages().
>>
>> When we inflate/deflate, we adjust totalram as well via
>> adjust_managed_page_count().
> 
> That is true only when not using DEFLATE_ON_OOM.
> 
> Otherwise inflated memory is accounted as used and total ram stays the same.
>> Consequently, this doesn't calculate what you actually want?
>> Total memory would be totalram+inflated, current would be totalram.
> 
> My calculations are correct for the case deflate_on_oom is enabled.
> 

Which is the corner cases. You'd have to special case on DEFLATE_ON_OOM
availability.

>> But, TBH, only export num_pages. User space can just lookup the other
>> information (totalram) via /proc/meminfo.
> 
> I have missed that the memory accounting is made differently depending 
> on a flag.
> 
> Since the calculations are different i'd prefer to have the values 
> calculate and printed there.

What about an indication instead, whether or not inflated pages are
accounted into total or not? That would be slightly cleaner IMHO.

-- 
Thanks,

David / dhildenb