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

[Alexander Atanasov]

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

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

Where no matter what the flags is:

 - ballooned_pages     -> how much is balloon inflated
 - total_pages             -> total ram the guest can get
 - current_pages         -> current ram the guest have

Patch to unify both cases with or without deflate on oom coming next.

--

Regards,
Alexander Atanasov