Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32;
From:   Vitaly Kuznetsov <vkuznets@redhat.com>
To:     "Michael Kelley (LINUX)" <mikelley@microsoft.com>,
        Angelina Vu <angelinavu@linux.microsoft.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>
Cc:     KY Srinivasan <kys@microsoft.com>,
        Haiyang Zhang <haiyangz@microsoft.com>,
        "wei.liu@kernel.org" <wei.liu@kernel.org>,
        Dexuan Cui <decui@microsoft.com>
Subject: RE: [PATCH] hv_balloon: Add module parameter to configure balloon
 floor value
In-Reply-To: <BYAPR21MB1688AD7F27C9CA40F7FB4EF1D7D6A@BYAPR21MB1688.namprd21.prod.outlook.com>
References: <1696978087-4421-1-git-send-email-angelinavu@linux.microsoft.com>
 <87jzrl71me.fsf@redhat.com>
 <BYAPR21MB1688AD7F27C9CA40F7FB4EF1D7D6A@BYAPR21MB1688.namprd21.prod.outlook.com>
Date:   Wed, 18 Oct 2023 10:27:30 +0200
Message-ID: <878r8072tp.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

"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