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

[Alexander Atanasov]

On 01/08/2022 18:18, David Hildenbrand wrote:
> On 26.07.22 16:08, Alexander Atanasov wrote:
> > Allow the guest to know how much it is ballooned by the host
> > and how that memory is accounted.
> >
> > It is useful when debugging out of memory conditions,
> > as well for userspace processes that monitor the memory pressure
> > and for nested virtualization.
> >
> > Depending on the deflate on oom flag memory is accounted in two ways.
> > If the flag is set the inflated pages are accounted as used memory.
> > If the flag is not set the inflated pages are substracted from totalram.
> > To make clear how are inflated pages accounted sign prefix the value.
> > Where negative means substracted from totalram and positive means
> > they are accounted as used.
> >
> > Signed-off-by: Alexander Atanasov <alexander.atanasov@xxxxxxxxxxxxx>
> > ---
> >   drivers/virtio/virtio_balloon.c | 59 +++++++++++++++++++++++++++++++++
> >   1 file changed, 59 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
> > V6:
> >    - reduced the file to minimum
> >    - unify accounting
> >
> > I didn't understand if you agree to change the accounting to be the same
> > so following part 2 is about it.
> >
> >
> > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > index f4c34a2a6b8e..97d3b29cb9f1 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,59 @@ static void report_free_page_func(struct work_struct *work)
> >   	}
> >   }
> >   
> > +/*
> > + * DEBUGFS Interface
> > + */
> > +#ifdef CONFIG_DEBUG_FS
> > +
> > +/**
> > + * 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.
> > + */
> > +
> Superfluous empty line. Personally, I'd just get rid of this
> (comparatively obvious) documentation completely.
Ok.
> > +static int virtio_balloon_debug_show(struct seq_file *f, void *offset)
> > +{
> > +	struct virtio_balloon *vb = f->private;
> > +	s64 num_pages = vb->num_pages << (VIRTIO_BALLOON_PFN_SHIFT - 10);
> Rather call this "inflated_kb" then, it's no longer "pages".
Ok.
> > +
> > +	if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > +		num_pages = -num_pages;
> With VIRTIO_BALLOON_F_DEFLATE_ON_OOM this will now always report "0".
>
> which would be the same as "num_pages = 0;" and would deserve a comment
> explaining why we don't indicate that as "inflated: ".
>
> Thanks.

Except if "now"  refers to some commit that i am missing it does not report 0.


with qemu-system-x86_64  -enable-kvm -device virtio-balloon,id=balloon0,deflate-on-oom=on,notify_on_empty=on,page-per-vq=on -m 3G ....

/ # cat /sys/kernel/debug/virtio-balloon
inflated: 0 kB
/ # QEMU 4.2.1 monitor - type 'help' for more information
(qemu) balloon 1024
(qemu)

/ # cat /sys/kernel/debug/virtio-balloon
inflated: 2097152 kB
/ #

with qemu-system-x86_64  -enable-kvm -device 
virtio-balloon,id=balloon0,deflate-on-oom=off,notify_on_empty=on,page-per-vq=on 
-m 3G ...

/ # cat /sys/kernel/debug/virtio-balloon
inflated: 0 kB
/ # QEMU 4.2.1 monitor - type 'help' for more information
(qemu) balloon 1024
(qemu)
/ # cat /sys/kernel/debug/virtio-balloon
inflated: -2097152 kB

To join the threads:

> > Always account inflated memory as used for both cases - with and
> > without deflate on oom. Do not change total ram which can confuse
> > userspace and users.

> Sorry, but NAK.

Ok.

> This would affect existing users / user space / balloon stats. For example
> HV just recently switch to properly using adjust_managed_page_count()


I am wondering what's the rationale behind this i have never seen such users
that expect it to work like this. Do you have any pointers to such users, so
i can understood why they do so ?

--

Regards,
Alexander Atanasov