Date:   Fri, 8 Jul 2022 19:24:14 +0300
From:   Alexander Atanasov <alexander.atanasov@...tuozzo.com>
To:     "Michael Kelley (LINUX)" <mikelley@...rosoft.com>,
        KY Srinivasan <kys@...rosoft.com>,
        Haiyang Zhang <haiyangz@...rosoft.com>,
        Stephen Hemminger <sthemmin@...rosoft.com>,
        Wei Liu <wei.liu@...nel.org>, Dexuan Cui <decui@...rosoft.com>
Cc:     "kernel@...nvz.org" <kernel@...nvz.org>,
        "linux-hyperv@...r.kernel.org" <linux-hyperv@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 1/1] Create debugfs file with hyper-v balloon usage
 information

Hi,

On 05/07/2022 20:12, Michael Kelley (LINUX) wrote:
> From: Alexander Atanasov <alexander.atanasov@...tuozzo.com> Sent: Tuesday, July 5, 2022 2:44 AM
>> Allow the guest to know how much it is ballooned by the host.
>> It is useful when debugging out of memory conditions.
>>
>> When host gets back memory from the guest it is accounted
>> as used memory in the guest but the guest have no way to know
>> how much it is actually ballooned.
>>
>> Expose current state, flags and max possible memory to the guest.
> Thanks for the contribution!  I can see it being useful.  But I'd note
> that the existing code has a trace function that outputs pretty much all
> the same information when it is reported to the Hyper-V host in
> post_status() every 1 second.  However,  the debugfs interface might be
> a bit easier to use for ongoing sampling.  Related, I see that the VMware
> balloon driver use the debugfs interface, but no tracing.  The virtio
> balloon driver has neither.  I'm not against adding this debugfs interface,
> but I wanted to make sure there's real value over the existing tracing.


Yes it is reported to the host but this is for the guest. The value is 
that the user space can track more accurate the memory pressure.

For example userspace cache size calculation based  on total and used 
ram can came out very wrong without balloon into account. If you have a 
nested virtualization/containers things can get confusing too.

VMWare have it already.  Virtio patches are waiting for response.


> See also some minor comments below.

Sorry, i missed the email.

>
> Michael
>
>> While at it - fix a 10+ years old typo.
>>
>> Signed-off-by: Alexander Atanasov <alexander.atanasov@...tuozzo.com>
>> ---
>>   drivers/hv/hv_balloon.c | 127 +++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 126 insertions(+), 1 deletion(-)
>>
>>
>> Note - no attempt to handle guest vs host page size difference
>> is made - see ballooning_enabled.
>> Basicly if balloon page size != guest page size balloon is off.
>>
>> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> index 91e8a72eee14..b7b87d168d46 100644
>> --- a/drivers/hv/hv_balloon.c
>> +++ b/drivers/hv/hv_balloon.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/jiffies.h>
>>   #include <linux/mman.h>
>> +#include <linux/debugfs.h>
>>   #include <linux/delay.h>
>>   #include <linux/init.h>
>>   #include <linux/module.h>
>> @@ -248,7 +249,7 @@ struct dm_capabilities_resp_msg {
>>    * num_committed: Committed memory in pages.
>>    * page_file_size: The accumulated size of all page files
>>    *		   in the system in pages.
>> - * zero_free: The nunber of zero and free pages.
>> + * zero_free: The number of zero and free pages.
>>    * page_file_writes: The writes to the page file in pages.
>>    * io_diff: An indicator of file cache efficiency or page file activity,
>>    *	    calculated as File Cache Page Fault Count - Page Read Count.
>> @@ -567,6 +568,14 @@ struct hv_dynmem_device {
>>   	__u32 version;
>>
>>   	struct page_reporting_dev_info pr_dev_info;
>> +
>> +#ifdef CONFIG_DEBUG_FS
>> +	/*
>> +	 * Maximum number of pages that can be hot_add-ed
>> +	 */
>> +	__u64 max_dynamic_page_count;
>> +#endif
>> +
>>   };
>>
>>   static struct hv_dynmem_device dm_device;
>> @@ -1078,6 +1087,9 @@ static void process_info(struct hv_dynmem_device *dm,
>> struct dm_info_msg *msg)
>>
>>   			pr_info("Max. dynamic memory size: %llu MB\n",
>>   				(*max_page_count) >> (20 - HV_HYP_PAGE_SHIFT));
>> +#ifdef CONFIG_DEBUG_FS
>> +			dm->max_dynamic_page_count = *max_page_count;
>> +#endif
> Arguably, you could drop the #ifdef's in the above two places, to reduce the code
> clutter.  The extra memory and CPU overhead of always saving max_dynamic_page_count
> is negligible.  What I don't know for sure is if the compiler or other static checking
> tools will complain about a field being set but not used.

Ok, i will drop them. If the tools complain i will have to fix it.

There is also a min dynamic memory size in the Hyper-V setup , which is 
interesting but the driver doesn't know about it.

>
>>   		}
>>
>>   		break;
>> @@ -1807,6 +1819,115 @@ static int balloon_connect_vsp(struct hv_device *dev)
>>   	return ret;
>>   }
>>
>> +/*
>> + * 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.
>> + */
>> +static int hv_balloon_debug_show(struct seq_file *f, void *offset)
>> +{
>> +	struct hv_dynmem_device *dm = f->private;
>> +	unsigned long num_pages_committed;
>> +	char *sname;
>> +
>> +	seq_printf(f, "%-22s: %u.%u\n", "host_version",
>> +				DYNMEM_MAJOR_VERSION(dm->version),
>> +				DYNMEM_MINOR_VERSION(dm->version));
>> +
>> +	seq_printf(f, "%-22s:", "capabilities");
>> +	if (ballooning_enabled())
>> +		seq_puts(f, " enabled");
>> +
>> +	if (hot_add_enabled())
>> +		seq_puts(f, " hot_add");
>> +
>> +	seq_puts(f, "\n");
>> +
>> +	seq_printf(f, "%-22s: %u", "state", dm->state);
>> +	switch (dm->state) {
>> +	case DM_INITIALIZING:
>> +			sname = "Initializing";
>> +			break;
>> +	case DM_INITIALIZED:
>> +			sname = "Initialized";
>> +			break;
>> +	case DM_BALLOON_UP:
>> +			sname = "Balloon Up";
>> +			break;
>> +	case DM_BALLOON_DOWN:
>> +			sname = "Balloon Down";
>> +			break;
>> +	case DM_HOT_ADD:
>> +			sname = "Hot Add";
>> +			break;
>> +	case DM_INIT_ERROR:
>> +			sname = "Error";
>> +			break;
>> +	default:
>> +			sname = "Unknown";
>> +	}
>> +	seq_printf(f, " (%s)\n", sname);
>> +
>> +	/* HV Page Size */
>> +	seq_printf(f, "%-22s: %ld\n", "page_size", HV_HYP_PAGE_SIZE);
>> +
>> +	/* Pages added with hot_add */
>> +	seq_printf(f, "%-22s: %u\n", "pages_added", dm->num_pages_added);
>> +
>> +	/* pages that are "onlined"/used from pages_added */
>> +	seq_printf(f, "%-22s: %u\n", "pages_onlined", dm->num_pages_onlined);
>> +
>> +	/* pages we have given back to host */
>> +	seq_printf(f, "%-22s: %u\n", "pages_ballooned", dm->num_pages_ballooned);
>> +
>> +	num_pages_committed = vm_memory_committed();
>> +	num_pages_committed += dm->num_pages_ballooned +
>> +				(dm->num_pages_added > dm->num_pages_onlined ?
>> +				dm->num_pages_added - dm->num_pages_onlined : 0) +
>> +				compute_balloon_floor();
> Duplicating this calculation that also appears in post_status() is not ideal.  Maybe
> post_status() should store the value in a field in the struct hv_dynmem_device, and
> this function would just output the field.  Again, there's the potential for a code
> checker to complain about a field being written but not read.  Alternatively,
> the calculation could go into a helper function that is called here and in
> post_status().  I'm not sure if it is more useful to report the last value that
> was reported by the Hyper-V host, or the currently calculated value.  There is a
> trace point that records the values reported to the host, so maybe the latter
> is more useful here.

I will extract it in a function. The calculation is cheap and 
considering that the debugfs file will be rarely read it is better with 
function.

It came out this way because i initially tried to add calculations of 
page sizes in the case where balloon and host have different page sizes.

But later on i figured out that in that case the balloon is not working. 
This can of course be improved.

-- 

Regards,
Alexander Atanasov

Powered by blists - more mailing lists