Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline

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

On Wed, Aug 14, 2024 at 2:03 PM Barry Song <21cnbao@gmail.com> wrote:
>
> From: Ryan Roberts <ryan.roberts@arm.com>
>
> Add thp_anon= cmdline parameter to allow specifying the default
> enablement of each supported anon THP size. The parameter accepts the
> following format and can be provided multiple times to configure each
> size:
>
> thp_anon=<size>,<size>[KMG]:<value>;<size>-<size>[KMG]:<value>
>
> An example:
>
> thp_anon=16K-64K:always;128K,512K:inherit;256K:madvise;1M-2M:never
>
> See Documentation/admin-guide/mm/transhuge.rst for more details.
>
> Configuring the defaults at boot time is useful to allow early user
> space to take advantage of mTHP before its been configured through
> sysfs.
>
> Signed-off-by: Ryan Roberts <ryan.roberts@arm.com>
> Co-developed-by: Barry Song <v-songbaohua@oppo.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---

Hi Andrew,

I saw you have pulled v4. Thanks!

Can you please squash the below changes suggested by Baolin and David?

From af42aa80f45d89798027e44a8711f7737e08b115 Mon Sep 17 00:00:00 2001
From: Barry Song <v-songbaohua@oppo.com>
Date: Thu, 15 Aug 2024 10:34:16 +1200
Subject: [PATCH] mm: use get_oder() and check size is is_power_of_2

Using get_order() is more robust according to Baolin.
It is also better to filter illegal size such as 3KB,
16KB according to David.

Suggested-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Suggested-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
 mm/huge_memory.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 01beda16aece..d6dade8ac5f6 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -952,14 +952,17 @@ static inline int get_order_from_str(const char *size_str)
 	int order;
 
 	size = memparse(size_str, &endptr);
-	order = fls(size >> PAGE_SHIFT) - 1;
-	if ((1 << order) & ~THP_ORDERS_ALL_ANON) {
-		pr_err("invalid size %s(order %d) in thp_anon boot parameter\n",
-			size_str, order);
-		return -EINVAL;
-	}
+
+	if (!is_power_of_2(size >> PAGE_SHIFT))
+		goto err;
+	order = get_order(size);
+	if ((1 << 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] __meminitdata;
-- 
2.34.1

Thanks
Barry