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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d4f302e6-033a-4a6a-9c42-077331b32bd1@redhat.com>
Date: Fri, 16 Aug 2024 11:33:37 +0200
From: David Hildenbrand <david@...hat.com>
To: Barry Song <21cnbao@...il.com>
Cc: akpm@...ux-foundation.org, baohua@...nel.org,
 baolin.wang@...ux.alibaba.com, corbet@....net, ioworker0@...il.com,
 linux-kernel@...r.kernel.org, linux-mm@...ck.org, ryan.roberts@....com,
 v-songbaohua@...o.com
Subject: Re: [PATCH v4] mm: Override mTHP "enabled" defaults at kernel cmdline

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