| Message ID | 20241101165719.1074234-5-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: add more kernel parameters to control mTHP | expand |
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>
Hi Baolin, On 03/11/24 23:25, Baolin Wang wrote: > > > 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. The idea is to explicitly show to the user which part of the kernel parameter is broke. Instead of saying that something is broken, it is going to return that, for example, "33K" is invalid. Best Regards, - Maíra > > Anyway, I don't have a strong preference. > Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
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; } if (start < 0 || end < 0 || start > end) diff --git a/mm/internal.h b/mm/internal.h index d5b93c5b6364..5a7302baeed7 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -1291,6 +1291,28 @@ static inline bool alloc_zeroed(void) &init_on_alloc); } +/* + * Parses a string with mem suffixes into its order. Useful to parse kernel + * parameters. + */ +static inline int get_order_from_str(const char *size_str, + unsigned long valid_orders) +{ + unsigned long size; + char *endptr; + int order; + + size = memparse(size_str, &endptr); + + if (!is_power_of_2(size)) + return -EINVAL; + order = get_order(size); + if (BIT(order) & ~valid_orders) + return -EINVAL; + + return order; +} + enum { /* mark page accessed */ FOLL_TOUCH = 1 << 16,
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(-)