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

[Konstantin Khorenko]

On 05.09.2022 16: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.
> 
>> 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"?

Talked to Den.
1. Let's print both "num_pages" and "actual"
2. Let's print also vdev->features (may be with clarification the meaning of bits. Or at least - the 
value itself, so we can check bits meaning manually later if needed)
3. Let's do it in an additional patch and send it to mainstream later as well as an additional patch 
because otherwise if we try to send one more iteration to mainstream, we again will slowdown the code 
inclusion.

>> +	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" ?

AFAIS, these names are stoned and approved by ms, so let's leave them as is.


> s/kB/KB/
> 
>> +
>> +	return 0;
>> +}
>> +
>> +DEFINE_SHOW_ATTRIBUTE(virtio_balloon_debug);
>> +
>> +static void  virtio_balloon_debugfs_init(struct virtio_balloon *b)
>> +{
>> +	debugfs_create_file("virtio-balloon", 0444, NULL, b,
>> +				&virtio_balloon_debug_fops);
>> +}
>> +
>> +static void  virtio_balloon_debugfs_exit(struct virtio_balloon *b)
>> +{
>> +	debugfs_remove(debugfs_lookup("virtio-balloon", NULL));
>> +}
>> +
>> +#else
>> +
>> +static inline void virtio_balloon_debugfs_init(struct virtio_balloon *b)
>> +{
>> +}
>> +
>> +static inline void virtio_balloon_debugfs_exit(struct virtio_balloon *b)
>> +{
>> +}
>> +
>> +#endif /* CONFIG_DEBUG_FS */
>> +
>>    #ifdef CONFIG_BALLOON_COMPACTION
>>    /*
>>     * virtballoon_migratepage - perform the balloon page migration on behalf of
>> @@ -568,6 +617,8 @@ static int virtballoon_probe(struct virtio_device *vdev)
>>    
>>    	if (towards_target(vb))
>>    		virtballoon_changed(vdev);
>> +	virtio_balloon_debugfs_init(vb);
>> +
>>    	return 0;
>>    
>>    out_oom_notify:
>> @@ -595,6 +646,7 @@ static void virtballoon_remove(struct virtio_device *vdev)
>>    {
>>    	struct virtio_balloon *vb = vdev->priv;
>>    
>> +	virtio_balloon_debugfs_exit(vb);
>>    	unregister_oom_notifier(&vb->nb);
>>    
>>    	spin_lock_irq(&vb->stop_update_lock);
> _______________________________________________
> Devel mailing list
> Devel at openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
> .