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

[Konstantin Khorenko]

On 07.09.2022 10:00, Alexander Atanasov wrote:
> 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.

Thank you for the explanation, you are right.


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

# vzkernel: git grep -w "kB" | wc -l
472
# vzkernel: git grep -w "KB" | wc -l
703

But yes, /proc/meminfo output uses "kB", so let it be.