On Fri, Aug 16, 2024 at 9:33 PM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 16.08.24 01:50, Barry Song wrote: > > On Thu, Aug 15, 2024 at 10:26 PM David Hildenbrand <david@xxxxxxxxxx> wrote: > >> > >>>>> +static inline int get_order_from_str(const char *size_str) > >>>>> +{ > >>>>> + unsigned long size; > >>>>> + char *endptr; > >>>>> + int order; > >>>>> + > >>>>> + size = memparse(size_str, &endptr); > >>>> > >>>> Do we have to also test if is_power_of_2(), and refuse if not? For > >>>> example, what if someone would pass 3K, would the existing check catch it? > >>> > >>> no, the existing check can't catch it. > >>> > >>> I passed thp_anon=15K-64K:always, then I got 16K enabled: > >>> > >>> / # cat /sys/kernel/mm/transparent_hugepage/hugepages-16kB/enabled > >>> [always] inherit madvise never > >>> > >> > >> Okay, so we should document then that start/end of the range must be > >> valid THP sizes. > > > > Ack > > > >> > >>> I can actually check that by: > >>> > >>> 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 >> PAGE_SHIFT)) > >> > >> No need for the shift. > >> > >> if (!is_power_of_2(size)) > >> > >> Is likely even more correct if someone would manage to pass something > >> stupid like > >> > >> 16385 (16K + 1) > > > > Ack > > > >> > >>> 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; > >>> } > >>> > >>>> > >>>>> + order = fls(size >> PAGE_SHIFT) - 1; > >>>> > >>>> Is this a fancy way of writing > >>>> > >>>> order = log2(size >> PAGE_SHIFT); > >>>> > >>>> ? :) > >>> > >>> I think ilog2 is implemented by fls ? > >> > >> Yes, so we should have used that instead. But get_order() > >> is even better. > >> > >>> > >>>> > >>>> Anyhow, if get_order() wraps that, all good. > >>> > >>> I guess it doesn't check power of 2? > >>> > >>>> > >>>>> + 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; > >>>>> + } > >>>>> + > >>>>> + return order; > >>>>> +} > >>>> > >>>> Apart from that, nothing jumped at me. > >>> > >>> Please take a look at the new get_order_from_str() before I > >>> send v5 :-) > >> > >> Besides the shift for is_power_of_2(), LGTM, thanks! > > > > Thanks, David! > > > > Hi Andrew, > > > > Apologies for sending another squash request. If you'd > > prefer me to send a new v5 that includes all the changes, > > please let me know. > > > > > > Don't shift the size, as it can still detect invalid sizes > > like 16K+1. Also, document that the size must be a valid THP > > size. > > > > diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst > > index 15404f06eefd..4468851b6ecb 100644 > > --- a/Documentation/admin-guide/mm/transhuge.rst > > +++ b/Documentation/admin-guide/mm/transhuge.rst > > @@ -294,8 +294,9 @@ kernel command line. > > > > Alternatively, each supported anonymous THP size can be controlled by > > passing ``thp_anon=<size>,<size>[KMG]:<state>;<size>-<size>[KMG]:<state>``, > > -where ``<size>`` is the THP size and ``<state>`` is one of ``always``, > > -``madvise``, ``never`` or ``inherit``. > > +where ``<size>`` is the THP size (must be a power of 2 of PAGE_SIZE and > > +supported anonymous THP) and ``<state>`` is one of ``always``, ``madvise``, > > +``never`` or ``inherit``. > > > > For example, the following will set 16K, 32K, 64K THP to ``always``, > > set 128K, 512K to ``inherit``, set 256K to ``madvise`` and 1M, 2M > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > index d6dade8ac5f6..903b47f2b2db 100644 > > --- a/mm/huge_memory.c > > +++ b/mm/huge_memory.c > > @@ -953,7 +953,7 @@ static inline int get_order_from_str(const char *size_str) > > > > size = memparse(size_str, &endptr); > > > > - if (!is_power_of_2(size >> PAGE_SHIFT)) > > + if (!is_power_of_2(size)) > > goto err; > > > Reading your documentation above, do we also want to test "if (size < > PAGE_SIZE)", or is that implicitly covered? (likely not I assume?) as we also check the order is valid. so size <PAGE_SIZE will get invalid order. 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 >> 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; } > > I assume it's implicitly covered: if we pass "1k" , it would be mapped > to "4k" (order-0) and that is not a valid mTHP size, right? > > I would appreciate a quick v5, just so can see the final result more > easily :) sure. > > -- > Cheers, > > David / dhildenb >