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

[Barry Song]

On Wed, Mar 11, 2026 at 1:58 AM Breno Leitao <leitao@xxxxxxxxxx> 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@xxxxxxxxxx>
> Reviewed-by: Lorenzo Stoakes (Oracle) <ljs@xxxxxxxxxx>
> ---
>  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..e19dda5aaf195 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",
> +};

Do we really need anon_enabled_mode here? Would it be ok
to just use ...

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

Nobody else cares about the values like ANON_ENABLED_ALWAYS
except the sysfs interface, right? Maybe we don't need to
force people to understand the enum.

> +
>  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 set_anon_enabled_mode(int order, enum anon_enabled_mode mode)
> +{
> +       static unsigned long *enabled_orders[] = {
> +               &huge_anon_orders_always,
> +               &huge_anon_orders_madvise,
> +               &huge_anon_orders_inherit,
> +       };

Is there a reason enabled_orders needs to be static?
Could it instead be allocated on the stack?

Thanks
Barry