Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.31 as permitted sender) client-ip=23.128.96.31;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     Angelina Vu <angelinavu@linux.microsoft.com>,
        linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org
Cc:     kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org,
        decui@microsoft.com
Subject: Re: [PATCH] hv_balloon: Add module parameter to configure balloon
 floor value
In-Reply-To: <1696978087-4421-1-git-send-email-angelinavu@linux.microsoft.com>
References: <1696978087-4421-1-git-send-email-angelinavu@linux.microsoft.com>
Date:   Tue, 17 Oct 2023 16:41:13 +0200
Message-ID: <87jzrl71me.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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