linux-kernel - Re: [PATCH v5 3/5] mm: move ``get_order_from

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9d5ce0af-6fca-422f-b1f8-650879f8ff5a@linux.alibaba.com>
Date: Mon, 4 Nov 2024 10:25:31 +0800
From: Baolin Wang <baolin.wang@...ux.alibaba.com>
To: Maíra Canal <mcanal@...lia.com>,
 Jonathan Corbet <corbet@....net>, Andrew Morton <akpm@...ux-foundation.org>,
 Hugh Dickins <hughd@...gle.com>, Barry Song <baohua@...nel.org>,
 David Hildenbrand <david@...hat.com>, Ryan Roberts <ryan.roberts@....com>,
 Lance Yang <ioworker0@...il.com>
Cc: linux-mm@...ck.org, dri-devel@...ts.freedesktop.org,
 linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-dev@...lia.com
Subject: Re: [PATCH v5 3/5] mm: move ``get_order_from_str()`` to internal.h



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@...lia.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@...ux.alibaba.com>