[Devel] [PATCH vz7] Create debugfs file with virtio balloon usage information

[Alexander Atanasov]

Hello,

On 5.09.22 17:01, Konstantin Khorenko wrote:
> Subject: [PATCH vz7] virtio_balloon: Create debugfs file with the usage 
> information
> 
> On 02.09.2022 13:19, Alexander Atanasov wrote:
>> Allow the guest to know how much it is ballooned by the host.
>> Depending on options the ballooned memory is accounted in two ways.
>> If deflate on oom is enabled - ballooned memory is accounted as used.
>> If deflate on oom is not enabled - ballooned memory is subtracted
>> from total ram.
> 
> Allow the guest to know how much memory is ballooned by the Host.
> 
> Depending on options the ballooned memory is accounted in two ways:
>   1. If deflate on OOM is enabled - ballooned memory is accounted as used.
>   2. If deflate on OOM is not enabled - ballooned memory is subtracted
>      from total RAM.
> 

Ok, i will update the commit message.

>> https://jira.sw.ru/browse/PSBM-140407
>> Signed-off-by: Alexander Atanasov <alexander.atanasov at virtuozzo.com>
>> ---
>>   drivers/virtio/virtio_balloon.c | 52 +++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c 
>> b/drivers/virtio/virtio_balloon.c
>> index 142719d05dc5..e8cefeea4f22 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -23,6 +23,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>
>> @@ -447,6 +448,54 @@ static int init_vqs(struct virtio_balloon *vb)
>>       return 0;
>>   }
>> +/*
>> + * DEBUGFS Interface
>> + */
>> +#ifdef CONFIG_DEBUG_FS
>> +
>> +static int virtio_balloon_debug_show(struct seq_file *f, void *offset)
>> +{
>> +    struct virtio_balloon *vb = f->private;
>> +    u64 inflated_kb = vb->num_pages << (VIRTIO_BALLOON_PFN_SHIFT - 10);
> 
> /*
>   * Balloon device works in 4K page units.  So each page is pointed to by
>   * multiple balloon pages.  All memory counters in this driver are in 
> balloon
>   * page units.
>   */
> #define VIRTIO_BALLOON_PAGES_PER_PAGE (unsigned)(PAGE_SIZE >> 
> VIRTIO_BALLOON_PFN_SHIFT)
> 
> /* Size of a PFN in the balloon interface. */
> #define VIRTIO_BALLOON_PFN_SHIFT 12
> 
> Taking into account defines above, i think the proper code should be:
>    /* "+2" here because we want the value in kilobytes */
>    u64 inflated_kb = vb->num_pages << (PAGE_SHIFT - 
> VIRTIO_BALLOON_PFN_SHIFT + 2);
> 
> As it's said VIRTIO_BALLOON_PAGE cannot be larger than PAGE_SIZE,
> so if we imagine VIRTIO_BALLOON_PFN_SHIFT is defined to, say, 10, the 
> original code
> "<< (VIRTIO_BALLOON_PFN_SHIFT - 10)" will definitely work wrong.
> 
> ===========================
> 
> Another question: why we report "num_pages", but not "actual"?
> 
> struct virtio_balloon_config {
>          /* Number of pages host wants Guest to give up. */
>          __u32 num_pages;
>          /* Number of pages we've actually got in balloon. */
>          __u32 actual;
> };
> 
> If we have a VM with, say, 4GB RAM and balloon is configured to grab, 
> say, 4GB, we need to know the memory _actually_ grabbed by balloon i think.
> 
> Or may be to report both "num_pages" and "actual"?
>

It is a bit missleading that header.
static void update_balloon_size(struct virtio_balloon *vb)
{
         u32 actual = vb->num_pages;

         /* Legacy balloon config space is LE, unlike all other devices. */
         virtio_cwrite_le(vb->vdev, struct virtio_balloon_config, actual,
                          &actual);
}

num_pages that are reported are stored as actual. there are no two 
values in the code. if the host requests too much memory it efectively 
kills the guest - oom triggers up to the init process.


> 
>> +    u64 inflated_total = 0;
>> +    u64 inflated_free = 0;
>> +
>> +    if (virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
>> +        inflated_free = inflated_kb;
>> +    else
>> +        inflated_total = inflated_kb;
>> +
>> +    seq_printf(f, "InflatedTotal: %lld kB\n", inflated_total);
>> +    seq_printf(f, "InflatedFree: %lld kB\n", inflated_free);
> 
> Are those namings are stoned already?
> May be to rename them to "DeflatedTotal" and "InflatedUsed" ?

    used boundary\           /total ram
|-------|--b1 ---|------|b2|


To deflate something you need to inflate it first , if you deflate to 
create negative pressure it is called a vaccum.
So when you inflate b1 it pushes the used boundary up.
When you inflate b2 it pushes the total ram down.
In both cases we are inflating the balloon.
The confusion comes from the deflate on oom option which does different 
thing - if guest goes OOM it requests the host to deflate what he have 
inflated.

> 
> s/kB/KB/
> 

No, please, see /proc/meminfo.


-- 
Regards,
Alexander Atanasov