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

[Baolin Wang]

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 <mca...@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.w...@linux.alibaba.com>