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

[David Hildenbrand]

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?)

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 :)

--
Cheers,

David / dhildenb