[PATCH] hv_balloon: Add module parameter to configure balloon floor value

[PATCH] hv_balloon: Add module parameter to configure balloon floor value

[Angelina Vu]

Currently, the balloon floor value is automatically computed, but may be
too small depending on app usage of memory. This patch adds a balloon_floor
value as a module parameter that can be used to manually configure the
balloon floor value.

Signed-off-by: Angelina Vu <angelinavu@linux.microsoft.com>
---
 drivers/hv/hv_balloon.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
index 64ac5bdee3a6..87b312f99b2e 100644
--- a/drivers/hv/hv_balloon.c
+++ b/drivers/hv/hv_balloon.c
@@ -1101,6 +1101,10 @@ static void process_info(struct hv_dynmem_device *dm, struct dm_info_msg *msg)
 	}
 }
 
+unsigned long balloon_floor;
+module_param(balloon_floor, ulong, 0644);
+MODULE_PARM_DESC(balloon_floor, "Memory level (in megabytes) that ballooning will not remove");
+
 static unsigned long compute_balloon_floor(void)
 {
 	unsigned long min_pages;
@@ -1117,6 +1121,9 @@ static unsigned long compute_balloon_floor(void)
 	 *    8192       744    (1/16)
 	 *   32768      1512	(1/32)
 	 */
+	if (balloon_floor)
+		return MB2PAGES(balloon_floor);
+
 	if (nr_pages < MB2PAGES(128))
 		min_pages = MB2PAGES(8) + (nr_pages >> 1);
 	else if (nr_pages < MB2PAGES(512))
-- 
2.34.1

Re: [PATCH] hv_balloon: Add module parameter to configure balloon floor value

[Wei Liu]

Hi Angelina,

On Tue, Oct 10, 2023 at 03:48:07PM -0700, Angelina Vu wrote:
> Currently, the balloon floor value is automatically computed, but may be
> too small depending on app usage of memory. This patch adds a balloon_floor
> value as a module parameter that can be used to manually configure the
> balloon floor value.
> 
> Signed-off-by: Angelina Vu <angelinavu@linux.microsoft.com>

Out of interest, will there be a case that the balloon floor value is
misconfigured, hence too small?

Why isn't the larger of the two values (computed and manually set)
returned instead?

Thanks,
Wei.

> ---
>  drivers/hv/hv_balloon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 64ac5bdee3a6..87b312f99b2e 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1101,6 +1101,10 @@ static void process_info(struct hv_dynmem_device *dm, struct dm_info_msg *msg)
>  	}
>  }
>  
> +unsigned long balloon_floor;
> +module_param(balloon_floor, ulong, 0644);
> +MODULE_PARM_DESC(balloon_floor, "Memory level (in megabytes) that ballooning will not remove");
> +
>  static unsigned long compute_balloon_floor(void)
>  {
>  	unsigned long min_pages;
> @@ -1117,6 +1121,9 @@ static unsigned long compute_balloon_floor(void)
>  	 *    8192       744    (1/16)
>  	 *   32768      1512	(1/32)
>  	 */
> +	if (balloon_floor)
> +		return MB2PAGES(balloon_floor);
> +
>  	if (nr_pages < MB2PAGES(128))
>  		min_pages = MB2PAGES(8) + (nr_pages >> 1);
>  	else if (nr_pages < MB2PAGES(512))
> -- 
> 2.34.1
> 
> 

Re: [PATCH] hv_balloon: Add module parameter to configure balloon floor value

[kernel test robot]

Hi Angelina,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.6-rc5 next-20231013]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Angelina-Vu/hv_balloon-Add-module-parameter-to-configure-balloon-floor-value/20231011-064921
base:   linus/master
patch link:    https://lore.kernel.org/r/1696978087-4421-1-git-send-email-angelinavu%40linux.microsoft.com
patch subject: [PATCH] hv_balloon: Add module parameter to configure balloon floor value
config: i386-randconfig-061-20231014 (https://download.01.org/0day-ci/archive/20231015/202310150425.g4PXZjIU-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231015/202310150425.g4PXZjIU-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310150425.g4PXZjIU-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/hv/hv_balloon.c:1100:15: sparse: sparse: symbol 'balloon_floor' was not declared. Should it be static?
   drivers/hv/hv_balloon.c:2079:9: sparse: sparse: context imbalance in 'balloon_remove' - wrong count at exit

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] hv_balloon: Add module parameter to configure balloon floor value

[Vitaly Kuznetsov]

Angelina Vu <angelinavu@linux.microsoft.com> writes:

> Currently, the balloon floor value is automatically computed, but may be
> too small depending on app usage of memory. This patch adds a balloon_floor
> value as a module parameter that can be used to manually configure the
> balloon floor value.
>
> Signed-off-by: Angelina Vu <angelinavu@linux.microsoft.com>
> ---
>  drivers/hv/hv_balloon.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> index 64ac5bdee3a6..87b312f99b2e 100644
> --- a/drivers/hv/hv_balloon.c
> +++ b/drivers/hv/hv_balloon.c
> @@ -1101,6 +1101,10 @@ static void process_info(struct hv_dynmem_device *dm, struct dm_info_msg *msg)
>  	}
>  }
>  
> +unsigned long balloon_floor;
> +module_param(balloon_floor, ulong, 0644);
> +MODULE_PARM_DESC(balloon_floor, "Memory level (in megabytes) that ballooning will not remove");
> +
>  static unsigned long compute_balloon_floor(void)
>  {
>  	unsigned long min_pages;
> @@ -1117,6 +1121,9 @@ static unsigned long compute_balloon_floor(void)
>  	 *    8192       744    (1/16)
>  	 *   32768      1512	(1/32)
>  	 */
> +	if (balloon_floor)
> +		return MB2PAGES(balloon_floor);
> +
>  	if (nr_pages < MB2PAGES(128))
>  		min_pages = MB2PAGES(8) + (nr_pages >> 1);
>  	else if (nr_pages < MB2PAGES(512))

A module parameter is probably useful for debugging but it can hardly be
applied in production environments as it must be 'one size fits all' and
e.g. for different VM sizes it can be different (that's the purpose of
compute_balloon_floor() heuristics). 

In fact, does it has to be statically set? Can we have a sysfs entity so
this can be a policy (userspace decision)? We can keep the current
compute_balloon_floor() as the default but users will be able to adjust
accordingly.

-- 
Vitaly

RE: [PATCH] hv_balloon: Add module parameter to configure balloon floor value

[Michael Kelley (LINUX)]

From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, October 17, 2023 7:41 AM
> 
> Angelina Vu <angelinavu@linux.microsoft.com> writes:
> 
> > Currently, the balloon floor value is automatically computed, but may be
> > too small depending on app usage of memory. This patch adds a balloon_floor
> > value as a module parameter that can be used to manually configure the
> > balloon floor value.
> >
> > Signed-off-by: Angelina Vu <angelinavu@linux.microsoft.com>
> > ---
> >  drivers/hv/hv_balloon.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
> > index 64ac5bdee3a6..87b312f99b2e 100644
> > --- a/drivers/hv/hv_balloon.c
> > +++ b/drivers/hv/hv_balloon.c
> > @@ -1101,6 +1101,10 @@ static void process_info(struct hv_dynmem_device *dm,
> struct dm_info_msg *msg)
> >  	}
> >  }
> >
> > +unsigned long balloon_floor;
> > +module_param(balloon_floor, ulong, 0644);
> > +MODULE_PARM_DESC(balloon_floor, "Memory level (in megabytes) that ballooning
> will not remove");
> > +
> >  static unsigned long compute_balloon_floor(void)
> >  {
> >  	unsigned long min_pages;
> > @@ -1117,6 +1121,9 @@ static unsigned long compute_balloon_floor(void)
> >  	 *    8192       744    (1/16)
> >  	 *   32768      1512	(1/32)
> >  	 */
> > +	if (balloon_floor)
> > +		return MB2PAGES(balloon_floor);
> > +
> >  	if (nr_pages < MB2PAGES(128))
> >  		min_pages = MB2PAGES(8) + (nr_pages >> 1);
> >  	else if (nr_pages < MB2PAGES(512))
> 
> A module parameter is probably useful for debugging but it can hardly be
> applied in production environments as it must be 'one size fits all' and
> e.g. for different VM sizes it can be different (that's the purpose of
> compute_balloon_floor() heuristics).
> 
> In fact, does it has to be statically set? Can we have a sysfs entity so
> this can be a policy (userspace decision)? We can keep the current
> compute_balloon_floor() as the default but users will be able to adjust
> accordingly.
> 

The module parameter can also be set or changed at runtime via
/sys/module/balloon/parameters/balloon_floor.  Changes made by
writing to that path are immediately reflected in the value of
the balloon_floor variable.  I think that accomplishes everything
you've outlined while also allowing a value to be set on the
kernel boot line.

Michael

RE: [PATCH] hv_balloon: Add module parameter to configure balloon floor value

[Vitaly Kuznetsov]

"Michael Kelley (LINUX)" <mikelley@microsoft.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com> Sent: Tuesday, October 17, 2023 7:41 AM
>> 
>> Angelina Vu <angelinavu@linux.microsoft.com> writes:
>> 
>> > Currently, the balloon floor value is automatically computed, but may be
>> > too small depending on app usage of memory. This patch adds a balloon_floor
>> > value as a module parameter that can be used to manually configure the
>> > balloon floor value.
>> >
>> > Signed-off-by: Angelina Vu <angelinavu@linux.microsoft.com>
>> > ---
>> >  drivers/hv/hv_balloon.c | 7 +++++++
>> >  1 file changed, 7 insertions(+)
>> >
>> > diff --git a/drivers/hv/hv_balloon.c b/drivers/hv/hv_balloon.c
>> > index 64ac5bdee3a6..87b312f99b2e 100644
>> > --- a/drivers/hv/hv_balloon.c
>> > +++ b/drivers/hv/hv_balloon.c
>> > @@ -1101,6 +1101,10 @@ static void process_info(struct hv_dynmem_device *dm,
>> struct dm_info_msg *msg)
>> >  	}
>> >  }
>> >
>> > +unsigned long balloon_floor;
>> > +module_param(balloon_floor, ulong, 0644);
>> > +MODULE_PARM_DESC(balloon_floor, "Memory level (in megabytes) that ballooning
>> will not remove");
>> > +
>> >  static unsigned long compute_balloon_floor(void)
>> >  {
>> >  	unsigned long min_pages;
>> > @@ -1117,6 +1121,9 @@ static unsigned long compute_balloon_floor(void)
>> >  	 *    8192       744    (1/16)
>> >  	 *   32768      1512	(1/32)
>> >  	 */
>> > +	if (balloon_floor)
>> > +		return MB2PAGES(balloon_floor);
>> > +
>> >  	if (nr_pages < MB2PAGES(128))
>> >  		min_pages = MB2PAGES(8) + (nr_pages >> 1);
>> >  	else if (nr_pages < MB2PAGES(512))
>> 
>> A module parameter is probably useful for debugging but it can hardly be
>> applied in production environments as it must be 'one size fits all' and
>> e.g. for different VM sizes it can be different (that's the purpose of
>> compute_balloon_floor() heuristics).
>> 
>> In fact, does it has to be statically set? Can we have a sysfs entity so
>> this can be a policy (userspace decision)? We can keep the current
>> compute_balloon_floor() as the default but users will be able to adjust
>> accordingly.
>> 
>
> The module parameter can also be set or changed at runtime via
> /sys/module/balloon/parameters/balloon_floor.  Changes made by
> writing to that path are immediately reflected in the value of
> the balloon_floor variable.  I think that accomplishes everything
> you've outlined while also allowing a value to be set on the
> kernel boot line.

Oh, thanks, I've actually forgot it is r/w. What's IMO not ideal with
this approach is that if you don't pass any value on the kernel command
line, '/sys/module/balloon/parameters/balloon_floor' will contain '0' so
the user will have to guess the actual current value. Would it make
sense to do:

  if (!balloon_floor)
          balloon_floor = compute_balloon_floor();

on boot/whenever(if) totalram_pages() changes? Personally, I'm not sure
it's a good idea as I've never seen kernel module parameters which would
behave this way :-) Another thing is that I'm not sure to which extent
'/sys/module/*/parameters/' can be considered a stable ABI, i.e. it is
not documented like Documentation/ABI/stable/*.

-- 
Vitaly