Message-ID: <CAGsJ_4z2D2yWWZhUM_yDSdn9=zpkYkHhzAKO8CQ1Xu3gDaECRA@mail.gmail.com>
Date: Fri, 9 Aug 2024 17:24:56 +0800
From: Barry Song <baohua@...nel.org>
To: David Hildenbrand <david@...hat.com>
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 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.

Just my two cents :-)

>
> --
> Cheers,
>
> David / dhildenb

Thanks
Barry

Powered by blists - more mailing lists