Re: [PATCH v4 2/4] mm: huge_memory: refactor anon_enabled_store() with change_anon_orders()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 3/9/26 12:07, Breno Leitao wrote:
> Consolidate the repeated spin_lock/set_bit/clear_bit pattern in
> anon_enabled_store() into a new change_anon_orders() helper that
> loops over an orders[] array, setting the bit for the selected mode
> and clearing the others.
> 
> Introduce enum anon_enabled_mode and anon_enabled_mode_strings[]
> for the per-order anon THP setting.
> 
> Use sysfs_match_string() with the anon_enabled_mode_strings[] table
> to replace the if/else chain of sysfs_streq() calls.
> 
> The helper uses test_and_set_bit()/test_and_clear_bit() to track
> whether the state actually changed, so start_stop_khugepaged() is
> only called when needed. When the mode is unchanged,
> set_recommended_min_free_kbytes() is called directly to preserve
> the watermark recalculation behavior of the original code.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>
> Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@kernel.org>
> ---
>  mm/huge_memory.c | 84 +++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 32 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 8e2746ea74adf..2d5b05a416dab 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -316,6 +316,20 @@ static ssize_t enabled_show(struct kobject *kobj,
>  	return sysfs_emit(buf, "%s\n", output);
>  }
>  
> +enum anon_enabled_mode {
> +	ANON_ENABLED_ALWAYS	= 0,
> +	ANON_ENABLED_MADVISE	= 1,
> +	ANON_ENABLED_INHERIT	= 2,
> +	ANON_ENABLED_NEVER	= 3,
> +};
> +
> +static const char * const anon_enabled_mode_strings[] = {
> +	[ANON_ENABLED_ALWAYS]	= "always",
> +	[ANON_ENABLED_MADVISE]	= "madvise",
> +	[ANON_ENABLED_INHERIT]	= "inherit",
> +	[ANON_ENABLED_NEVER]	= "never",
> +};
> +
>  static ssize_t enabled_store(struct kobject *kobj,
>  			     struct kobj_attribute *attr,
>  			     const char *buf, size_t count)
> @@ -515,48 +529,54 @@ static ssize_t anon_enabled_show(struct kobject *kobj,
>  	return sysfs_emit(buf, "%s\n", output);
>  }
>  
> +static bool change_anon_orders(int order, enum anon_enabled_mode mode)

I would suggest something a bit longer but clearer

"set_anon_enabled_mode_for_order()"

Or shorter

"set_anon_enabled_mode"

1) set vs. change. the function returns whether actually something
   changed.

2) We're not really changing "anon_orders". Yeah, we're updating
   variables that are named "huge_anon_orders_XXX", but that's more an
   implementation detail when setting the anon_enabled mode for a
   specific order.

> +{
> +	static unsigned long *orders[] = {
> +		&huge_anon_orders_always,
> +		&huge_anon_orders_madvise,
> +		&huge_anon_orders_inherit,
> +	};

Having a "order" and "orders" variable that have different semantics is
a bit confusing. Don't really have a better suggestion. "enabled_orders"
? hm.


> +	enum anon_enabled_mode m;
> +	bool changed = false;
> +
> +	spin_lock(&huge_anon_orders_lock);
> +	for (m = 0; m < ARRAY_SIZE(orders); m++) {
> +		if (m == mode)
> +			changed |= !test_and_set_bit(order, orders[m]);
> +		else
> +			changed |= test_and_clear_bit(order, orders[m]);
> +	}

Can we use the non-atomic variant here? __test_and_set_bit(). Just
wondering as the lock protects concurrent modifications.


> +	spin_unlock(&huge_anon_orders_lock);
> +
> +	return changed;
> +}
> +

Apart from that LGTM.

-- 
Cheers,

David