Re: [PATCH v2 3/4] mm: generalize the implementation of ``thp_anon=``

[Barry Song <21cnbao@gmail.com>]

On Tue, Oct 29, 2024 at 8:24 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> Currently, the function ``setup_thp_anon()`` is extremely tied to the
> policies and bitmaps that currently exist for anon THP. This means that
> we can't reuse the function if we implement a ``thp_shmem=``.
>
> This commit extracts the behavior of the function to a new generic
> function. The new function is exposed in common headers for future use
> by mm/shmem.c.
>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Sorry, Maíra, my mistake. I don't see much value in making
get_order_from_str() a common
API since bootcmd such a small thing. I'd prefer to stick with your
previous code with
just a rename—unless, at some point, we move both shmem and anon controls into
mm/huge_memory.c.

> ---
>  include/linux/huge_mm.h |  29 +++++++++++
>  mm/huge_memory.c        | 109 ++++++++++++++++++----------------------
>  2 files changed, 78 insertions(+), 60 deletions(-)
>
> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index b94c2e8ee918..b82e9379f2bf 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -471,6 +471,35 @@ void split_huge_pmd_locked(struct vm_area_struct *vma, unsigned long address,
>  bool unmap_huge_pmd_locked(struct vm_area_struct *vma, unsigned long addr,
>                            pmd_t *pmdp, struct folio *folio);
>
> +static inline int get_order_from_str(const char *size_str,
> +                                    const unsigned long thp_orders)
> +{
> +       unsigned long size;
> +       char *endptr;
> +       int order;
> +
> +       size = memparse(size_str, &endptr);
> +
> +       if (!is_power_of_2(size))
> +               goto err;
> +
> +       order = get_order(size);
> +       if (BIT(order) & ~thp_orders)
> +               goto err;
> +
> +       return order;
> +err:
> +       pr_err("invalid size %s in boot parameter\n", size_str);
> +       return -EINVAL;
> +}
> +
> +struct thp_policy_bitmap {
> +       const char *policy;
> +       unsigned long bitmap;
> +};
> +
> +int parse_huge_orders(char *p, struct thp_policy_bitmap *policies,
> +                     const int num_policies, const unsigned long thp_orders);
>  #else /* CONFIG_TRANSPARENT_HUGEPAGE */
>
>  static inline bool folio_test_pmd_mappable(struct folio *folio)
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 832ca761b4c3..c61f4481cb6a 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -958,91 +958,80 @@ static int __init setup_transparent_hugepage(char *str)
>  }
>  __setup("transparent_hugepage=", setup_transparent_hugepage);
>
> -static inline int get_order_from_str(const char *size_str)
> -{
> -       unsigned long size;
> -       char *endptr;
> -       int order;
> -
> -       size = memparse(size_str, &endptr);
> -
> -       if (!is_power_of_2(size))
> -               goto err;
> -       order = get_order(size);
> -       if (BIT(order) & ~THP_ORDERS_ALL_ANON)
> -               goto err;
> -
> -       return order;
> -err:
> -       pr_err("invalid size %s in thp_anon boot parameter\n", size_str);
> -       return -EINVAL;
> -}
> -
> -static char str_dup[PAGE_SIZE] __initdata;
> -static int __init setup_thp_anon(char *str)
> +int parse_huge_orders(char *p, struct thp_policy_bitmap *policies,
> +                     const int num_policies, const unsigned long thp_orders)
>  {
>         char *token, *range, *policy, *subtoken;
> -       unsigned long always, inherit, madvise;
>         char *start_size, *end_size;
> -       int start, end, nr;
> -       char *p;
> +       int start, end, nr, i;
> +       bool policy_set;
>
> -       if (!str || strlen(str) + 1 > PAGE_SIZE)
> -               goto err;
> -       strcpy(str_dup, str);
> -
> -       always = huge_anon_orders_always;
> -       madvise = huge_anon_orders_madvise;
> -       inherit = huge_anon_orders_inherit;
> -       p = str_dup;
>         while ((token = strsep(&p, ";")) != NULL) {
>                 range = strsep(&token, ":");
>                 policy = token;
>
>                 if (!policy)
> -                       goto err;
> +                       return 0;
>
>                 while ((subtoken = strsep(&range, ",")) != NULL) {
> +                       policy_set = false;
> +
>                         if (strchr(subtoken, '-')) {
>                                 start_size = strsep(&subtoken, "-");
>                                 end_size = subtoken;
>
> -                               start = get_order_from_str(start_size);
> -                               end = get_order_from_str(end_size);
> +                               start = get_order_from_str(start_size, thp_orders);
> +                               end = get_order_from_str(end_size, thp_orders);
>                         } else {
> -                               start = end = get_order_from_str(subtoken);
> +                               start = end = get_order_from_str(subtoken, thp_orders);
>                         }
>
>                         if (start < 0 || end < 0 || start > end)
> -                               goto err;
> +                               return 0;
>
>                         nr = end - start + 1;
> -                       if (!strcmp(policy, "always")) {
> -                               bitmap_set(&always, start, nr);
> -                               bitmap_clear(&inherit, start, nr);
> -                               bitmap_clear(&madvise, start, nr);
> -                       } else if (!strcmp(policy, "madvise")) {
> -                               bitmap_set(&madvise, start, nr);
> -                               bitmap_clear(&inherit, start, nr);
> -                               bitmap_clear(&always, start, nr);
> -                       } else if (!strcmp(policy, "inherit")) {
> -                               bitmap_set(&inherit, start, nr);
> -                               bitmap_clear(&madvise, start, nr);
> -                               bitmap_clear(&always, start, nr);
> -                       } else if (!strcmp(policy, "never")) {
> -                               bitmap_clear(&inherit, start, nr);
> -                               bitmap_clear(&madvise, start, nr);
> -                               bitmap_clear(&always, start, nr);
> -                       } else {
> -                               pr_err("invalid policy %s in thp_anon boot parameter\n", policy);
> -                               goto err;
> +
> +                       for (i = 0; i < num_policies; i++) {
> +                               if (!strcmp(policy, policies[i].policy)) {
> +                                       bitmap_set(&policies[i].bitmap, start, nr);
> +                                       policy_set = true;
> +                               } else
> +                                       bitmap_clear(&policies[i].bitmap, start, nr);
> +                       }
> +
> +                       if (!policy_set && strcmp(policy, "never")) {
> +                               pr_err("invalid policy %s in boot parameter\n", policy);
> +                               return 0;
>                         }
>                 }
>         }
>
> -       huge_anon_orders_always = always;
> -       huge_anon_orders_madvise = madvise;
> -       huge_anon_orders_inherit = inherit;
> +       return 1;
> +}
> +
> +static char str_dup[PAGE_SIZE] __initdata;
> +static int __init setup_thp_anon(char *str)
> +{
> +       struct thp_policy_bitmap policies[] = {
> +               { "always",  huge_anon_orders_always },
> +               { "madvise",  huge_anon_orders_madvise },
> +               { "inherit",  huge_anon_orders_inherit },
> +       };
> +       char *p;
> +
> +       if (!str || strlen(str) + 1 > PAGE_SIZE)
> +               goto err;
> +
> +       strscpy(str_dup, str);
> +       p = str_dup;
> +
> +       if (!parse_huge_orders(p, policies, ARRAY_SIZE(policies),
> +                             THP_ORDERS_ALL_ANON))
> +               goto err;
> +
> +       huge_anon_orders_always = policies[0].bitmap;
> +       huge_anon_orders_madvise = policies[1].bitmap;
> +       huge_anon_orders_inherit = policies[2].bitmap;
>         anon_orders_configured = true;
>         return 1;
>
> --
> 2.46.2
>

Thanks
Barry