linux-kernel - Re: [PATCH v2] 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: <75eb6e75-8a6d-460a-8e96-7496ed1396b7@redhat.com>
Date: Fri, 9 Aug 2024 11:32:01 +0200
From: David Hildenbrand <david@...hat.com>
To: Barry Song <baohua@...nel.org>
Cc: Ryan Roberts <ryan.roberts@....com>,
 Andrew Morton <akpm@...ux-foundation.org>, Jonathan Corbet <corbet@....net>,
 Lance Yang <ioworker0@...il.com>, Baolin Wang
 <baolin.wang@...ux.alibaba.com>, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org
Subject: Re: [PATCH v2] mm: Override mTHP "enabled" defaults at kernel cmdline

On 09.08.24 11:24, Barry Song wrote:
> On Fri, Aug 9, 2024 at 5:19 PM David Hildenbrand <david@...hat.com> wrote:
>>
>> On 08.08.24 12:16, Ryan Roberts wrote:
>>> Add thp_anon= cmdline parameter to allow specifying the default
>>> enablement of each supported anon THP size. The parameter accepts the
>>> following format and can be provided multiple times to configure each
>>> size:
>>>
>>> thp_anon=<size>[KMG]:<value>
>>>
>>> See Documentation/admin-guide/mm/transhuge.rst for more details.
>>>
>>> Configuring the defaults at boot time is useful to allow early user
>>> space to take advantage of mTHP before its been configured through
>>> sysfs.
>>
>> I suspect a khugeapged enhancement and/or kernel-config-dependant
>> defaults and/or early system settings will also be able to mitigate that
>> without getting kernel cmdlines involved in the future.
>>
>>>
>>> Signed-off-by: Ryan Roberts <ryan.roberts@....com>
>>> ---
>>>
>>> Hi All,
>>>
>>> I've split this off from my RFC at [1] because Barry highlighted that he would
>>> benefit from it immediately [2]. There are no changes vs the version in that
>>> series.
>>>
>>> It applies against today's mm-unstable (275d686abcb59). (although I had to fix a
>>> minor build bug in stackdepot.c due to MIN() not being defined in this tree).
>>>
>>> Thanks,
>>> Ryan
>>>
>>>
>>>    .../admin-guide/kernel-parameters.txt         |  8 +++
>>>    Documentation/admin-guide/mm/transhuge.rst    | 26 +++++++--
>>>    mm/huge_memory.c                              | 55 ++++++++++++++++++-
>>>    3 files changed, 82 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>>> index bcdee8984e1f0..5c79b58c108ec 100644
>>> --- a/Documentation/admin-guide/kernel-parameters.txt
>>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>>> @@ -6631,6 +6631,14 @@
>>>                        <deci-seconds>: poll all this frequency
>>>                        0: no polling (default)
>>>
>>> +     thp_anon=       [KNL]
>>> +                     Format: <size>[KMG]:always|madvise|never|inherit
>>> +                     Can be used to control the default behavior of the
>>> +                     system with respect to anonymous transparent hugepages.
>>> +                     Can be used multiple times for multiple anon THP sizes.
>>> +                     See Documentation/admin-guide/mm/transhuge.rst for more
>>> +                     details.
>>> +
>>>        threadirqs      [KNL,EARLY]
>>>                        Force threading of all interrupt handlers except those
>>>                        marked explicitly IRQF_NO_THREAD.
>>> diff --git a/Documentation/admin-guide/mm/transhuge.rst b/Documentation/admin-guide/mm/transhuge.rst
>>> index 24eec1c03ad88..f63b0717366c6 100644
>>> --- a/Documentation/admin-guide/mm/transhuge.rst
>>> +++ b/Documentation/admin-guide/mm/transhuge.rst
>>> @@ -284,13 +284,27 @@ that THP is shared. Exceeding the number would block the collapse::
>>>
>>>    A higher value may increase memory footprint for some workloads.
>>>
>>> -Boot parameter
>>> -==============
>>> +Boot parameters
>>> +===============
>>>
>>> -You can change the sysfs boot time defaults of Transparent Hugepage
>>> -Support by passing the parameter ``transparent_hugepage=always`` or
>>> -``transparent_hugepage=madvise`` or ``transparent_hugepage=never``
>>> -to the kernel command line.
>>> +You can change the sysfs boot time default for the top-level "enabled"
>>> +control by passing the parameter ``transparent_hugepage=always`` or
>>> +``transparent_hugepage=madvise`` or ``transparent_hugepage=never`` to the
>>> +kernel command line.
>>> +
>>> +Alternatively, each supported anonymous THP size can be controlled by
>>> +passing ``thp_anon=<size>[KMG]:<state>``, where ``<size>`` is the THP size
>>> +and ``<state>`` is one of ``always``, ``madvise``, ``never`` or
>>> +``inherit``.
>>> +
>>> +For example, the following will set 64K THP to ``always``::
>>> +
>>> +     thp_anon=64K:always
>>> +
>>> +``thp_anon=`` may be specified multiple times to configure all THP sizes as
>>> +required. If ``thp_anon=`` is specified at least once, any anon THP sizes
>>> +not explicitly configured on the command line are implicitly set to
>>> +``never``.
>>
>> I suggest documenting that "thp_anon=" will not effect the value of
>> "transparent_hugepage=", or any configured default.
>>
>> Wondering if a syntax like
>>
>> thp_anon=16K,32K,64K:always;1048K,2048K:madvise
>>
>> (one could also support ranges, like "16K-64K")
>>
>> Would be even better. Then, maybe only allow a single instance.
>>
>> Maybe consider it if it's not too crazy to parse ;)
> 
> I prefer the current approach because it effectively filters cases like this.
> 
> [    0.000000] huge_memory: thp_anon=8K:inherit: cannot parse, ignored
> [    0.000000] Unknown kernel command line parameters
> "thp_anon=8K:inherit", will be passed to user space.
> 
> if we put multiple sizes together, 8K,32K,64K:always
> 
> We can't determine whether this command line is legal or illegal as it
> is partially legal and partially illegal.

Besides: I wouldn't bother about this "user does something stupid" 
scenario that much.

But yes, once we support more sizes a cmdline might turn invalid on an 
older kernel.

However, I don't see the problem here. User passed a non-existant size. 
Ignore that one but handle the others, like you would with multiple 
commands?

It can be well defined and documented. The command line is legal, just 
one size does not exist.

The world will continue turning :)

-- 
Cheers,

David / dhildenb