Re: [PATCH v2 3/3] mm: huge_memory: refactor enabled_store() with change_enabled()

["Lorenzo Stoakes (Oracle)" <ljs@kernel.org>]

On Thu, Mar 05, 2026 at 06:04:55AM -0800, Breno Leitao wrote:
> Refactor enabled_store() to use a new change_enabled() helper that
> reuses the shared enum enabled_mode and enabled_mode_strings[]
> introduced in the previous commit.
>
> The helper uses the same loop pattern as change_anon_orders(),
> iterating over an array of flag bit positions and using
> test_and_set_bit()/test_and_clear_bit() to track whether the state
> actually changed. ENABLED_INHERIT is rejected since it is not a
> valid mode for the global THP setting.
>
> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  mm/huge_memory.c | 51 ++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 36 insertions(+), 15 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 19619213f54d1..dd5011cf36839 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -330,30 +330,51 @@ static const char * const enabled_mode_strings[] = {
>  	[ENABLED_NEVER]		= "never",
>  };
>
> +static bool change_enabled(enum enabled_mode mode)
> +{
> +	static const unsigned long thp_flags[] = {
> +		TRANSPARENT_HUGEPAGE_FLAG,
> +		TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG,
> +	};
> +	bool changed = false;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(thp_flags); i++) {
> +		if (i == mode)
> +			changed |= !test_and_set_bit(thp_flags[i],
> +						     &transparent_hugepage_flags);
> +		else
> +			changed |= test_and_clear_bit(thp_flags[i],
> +						      &transparent_hugepage_flags);
> +	}
> +
> +	return changed;
> +}
> +
>  static ssize_t enabled_store(struct kobject *kobj,
>  			     struct kobj_attribute *attr,
>  			     const char *buf, size_t count)
>  {
> -	ssize_t ret = count;
> +	int mode;
>
> -	if (sysfs_streq(buf, "always")) {
> -		clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> -		set_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> -	} else if (sysfs_streq(buf, "madvise")) {
> -		clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> -		set_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> -	} else if (sysfs_streq(buf, "never")) {
> -		clear_bit(TRANSPARENT_HUGEPAGE_FLAG, &transparent_hugepage_flags);
> -		clear_bit(TRANSPARENT_HUGEPAGE_REQ_MADV_FLAG, &transparent_hugepage_flags);
> -	} else
> -		ret = -EINVAL;
> +	mode = sysfs_match_string(enabled_mode_strings, buf);
> +	if (mode < 0 || mode == ENABLED_INHERIT)
> +		return -EINVAL;

The mode == ENABLED_INHERIT check is weird, it reads like 'oh a user CAN specify
this, but if they do we error out', but in reality
/sys/kernel/mm/transparent_hugepage/enabled explicitly outputs only always,
inherit, maddvise.

So, even though it's duplicative, I think it's probably saner to have:

static const char * const anon_enabled_mode_strings[] = {
	[ENABLED_ALWAYS]	= "always",
	[ENABLED_MADVISE]	= "madvise",
	[ENABLED_INHERIT]	= "inherit",
	[ENABLED_NEVER]		= "never",
};

static const char * const global_enabled_mode_strings[] = {
	[ENABLED_ALWAYS]	= "always",
	[ENABLED_MADVISE]	= "madvise",
	[ENABLED_NEVER]		= "never",
};

Just to make it clear what you're doing.

And if you add something for shmem add one for that too.

With that fixed, feel free to add:

Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>

>
> -	if (ret > 0) {
> +	if (change_enabled(mode)) {
>  		int err = start_stop_khugepaged();
> +
>  		if (err)
> -			ret = err;
> +			return err;
> +	} else {
> +		/*
> +		 * Recalculate watermarks even when the mode didn't
> +		 * change, as the previous code always called
> +		 * start_stop_khugepaged() which does this internally.
> +		 */
> +		set_recommended_min_free_kbytes();
>  	}
> -	return ret;
> +	return count;
>  }
>
>  static struct kobj_attribute enabled_attr = __ATTR_RW(enabled);
>
> --
> 2.47.3
>

Cheers, Lorenzo