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

[David Hildenbrand <david@redhat.com>]

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

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