Messages in this thread | | | Date | Mon, 1 Aug 2022 19:34:36 +0300 | Subject | Re: [PATCH v6 1/2] Create debugfs file with virtio balloon usage information | From | 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@virtuozzo.com> >> --- >> 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
| |