Re: [PATCH 1/2] mm: thp: avoid calling start_stop_khugepaged() in anon_enabled_store()

[Breno Leitao <leitao@debian.org>]

On Wed, Mar 04, 2026 at 04:40:22PM +0000, Lorenzo Stoakes (Oracle) wrote:
> On Wed, Mar 04, 2026 at 02:22:33AM -0800, Breno Leitao wrote:
> > Writing "never" (or any other value) multiple times to
> > /sys/kernel/mm/transparent_hugepage/hugepages-*/enabled calls
> > start_stop_khugepaged() each time, even when nothing actually changed.
> > This causes set_recommended_min_free_kbytes() to run unconditionally,
> > which is unnecessary and floods the printk buffer with "raising
> > min_free_kbytes" messages. Example:
> >
> >   # for i in $(seq 100); do
> >   #       echo never > /sys/kernel/mm/transparent_hugepage/enabled
> >   # done
> >
> >   # dmesg | grep "min_free_kbytes is not updated" | wc -l
> >   100
> >
> > Use test_and_set_bit()/test_and_clear_bit() instead of the plain
> > variants to detect whether any bit actually flipped, and skip the
> > start_stop_khugepaged() call entirely when the configuration is
> > unchanged.
> >
> > With this patch, redoing the same operation becomes a no-op.
> >
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> General concept is sensible, but let's improve this code please.

Ack! Thanks for the suggestions.

> >  		spin_unlock(&huge_anon_orders_lock);
> >  	} else
> >  		ret = -EINVAL;
> >
> > -	if (ret > 0) {
> > +	if (ret > 0 && changed) {
> >  		int err;
> >
> >  		err = start_stop_khugepaged();
> 
> There's a caveat here as mentioned in reply to Kiryl - I'm concerned users
> might rely on the set recommended min kbytes even when things don't change.
> 
> Not sure how likely that is, but it's a user-visible change in how this behaves.

Is there any motivation that users are retouching
/sys/kernel/mm/transparent_hugepage just to trigger
set_recommended_min_free_kbytes() ? That seems weird, but, I will keep it in
the change.

> From cb2c4c8bf183ef0d10068cfd12c12d19cb17a241 Mon Sep 17 00:00:00 2001
> From: "Lorenzo Stoakes (Oracle)" <ljs@kernel.org>
> Date: Wed, 4 Mar 2026 16:37:20 +0000
> Subject: [PATCH] idea
> 
> Signed-off-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>

Thanks for the idea. Let me hack on top of it, and propose a v2.

> ---
>  mm/huge_memory.c | 74 ++++++++++++++++++++++++++++++------------------
>  1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 0df1f4a17430..97dabbeb9112 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -515,46 +515,64 @@ static ssize_t anon_enabled_show(struct kobject *kobj,
>  	return sysfs_emit(buf, "%s\n", output);
>  }
> 
> +enum huge_mode {
> +	HUGE_ALWAYS,
> +	HUGE_INHERIT,
> +	HUGE_MADVISE,
> +	HUGE_NUM_MODES,
> +	HUGE_NEVER,
> +};
> +
> +static bool change_anon_orders(int order, enum huge_mode mode)
> +{
> +	static unsigned long *orders[] = {
> +		&huge_anon_orders_always,
> +		&huge_anon_orders_inherit,
> +		&huge_anon_orders_madvise,
> +	};
> +	bool changed = false;
> +	int i;
> +
> +	spin_lock(&huge_anon_orders_lock);
> +	for (i = 0; i < HUGE_NUM_MODES; i++) {

> +		if (i == mode)
> +			changed |= !test_and_set_bit(order, orders[mode]);
> +		else
> +			changed |= test_and_clear_bit(order, orders[mode]);

I suppose we want s/mode/i in the test_and_{clear,set}_bit() here:

		if (i == mode)
			// set for mode
			changed |= !test_and_set_bit(order, orders[i]);
		else
			// clear for !mode
			changed |= test_and_clear_bit(order, orders[i]);

For two reasons:
	* you want to unset "i" when i != mode.
	* you would have an OOB when accessing orders[HUGE_NEVER == 4]


>  static ssize_t anon_enabled_store(struct kobject *kobj,
>  				 struct kobj_attribute *attr,
>  				 const char *buf, size_t count)
>  {
>  	int order = to_thpsize(kobj)->order;
>  	ssize_t ret = count;
> +	bool changed;
> +
> +	if (sysfs_streq(buf, "always"))
> +		changed = change_anon_orders(order, HUGE_ALWAYS);
> +	else if (sysfs_streq(buf, "inherit"))
> +		changed = change_anon_orders(order, HUGE_INHERIT);
> +	else if (sysfs_streq(buf, "madvise"))
> +		changed = change_anon_orders(order, HUGE_MADVISE);
> +	else if (sysfs_streq(buf, "never"))
> +		changed = change_anon_orders(order, HUGE_NEVER);
> +	else
> +		return -EINVAL;

I think we can simplify anon_enabled_store() even more, by leveraging sysfs_match_string().
Something like:

	static const char *const anon_mode_strings[] = {
		[HUGE_ALWAYS]   = "always",
		[HUGE_INHERIT]  = "inherit",
		[HUGE_MADVISE]  = "madvise",
		[HUGE_NEVER]    = "never",
		NULL,
	};

and then

	static ssize_t anon_enabled_store(struct kobject *kobj,
					struct kobj_attribute *attr,
					const char *buf, size_t count)
	{
		int order = to_thpsize(kobj)->order;
		int mode;

		mode = sysfs_match_string(enabled_mode_strings, buf);
		if (mode < 0)
			return mode;

		if (change_anon_orders(order, mode)) {
			int err = start_stop_khugepaged();

			if (err)
				return err;
		} else {
			/* Users expect this even if unchanged. TODO: Put in header... */
			//set_recommended_min_free_kbytes();
		}
		return count;
	}



Anyway, I like this approach, thanks!. Let me hack a v2 based on it.

--breno