Re: [PATCH v5 3/5] mm: move ``get_order_from_str()`` to internal.h

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2024/11/2 00:54, Maíra Canal wrote:
> In order to implement a kernel parameter similar to ``thp_anon=`` for
> shmem, we'll need the function ``get_order_from_str()``.
> 
> Instead of duplicating the function, move the function to a shared
> header, in which both mm/shmem.c and mm/huge_memory.c will be able to
> use it.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   mm/huge_memory.c | 38 +++++++++++++++-----------------------
>   mm/internal.h    | 22 ++++++++++++++++++++++
>   2 files changed, 37 insertions(+), 23 deletions(-)
> 
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index f92068864469..a6edbd8c4f49 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -958,26 +958,6 @@ 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)
>   {
> @@ -1007,10 +987,22 @@ static int __init setup_thp_anon(char *str)
>   				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_ALL_ANON);
> +				end = get_order_from_str(end_size, THP_ORDERS_ALL_ANON);
>   			} else {
> -				start = end = get_order_from_str(subtoken);
> +				start_size = end_size = subtoken;
> +				start = end = get_order_from_str(subtoken,
> +								 THP_ORDERS_ALL_ANON);
> +			}
> +
> +			if (start == -EINVAL) {
> +				pr_err("invalid size %s in thp_anon boot parameter\n", start_size);
> +				goto err;
> +			}
> +
> +			if (end == -EINVAL) {
> +				pr_err("invalid size %s in thp_anon boot parameter\n", end_size);
> +				goto err;
>   			}

There are already checks for ‘start’ and ‘end’ below, and will print 
error messages if error occurs. So I suspect whether these repeated 
checks and error infor are helpful.

Anyway, I don't have a strong preference.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>