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

David Hildenbrand (Arm)
2026-03-09T13:43:24Z

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