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

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

On Fri, Aug 16, 2024 at 9:33 PM David Hildenbrand <david@redhat.com> wrote:
>
> On 16.08.24 01:50, Barry Song wrote:
> > On Thu, Aug 15, 2024 at 10:26 PM David Hildenbrand <david@redhat.com> 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
>