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: <20240815235001.96624-1-21cnbao@gmail.com>
Date: Fri, 16 Aug 2024 11:50:00 +1200
From: Barry Song <21cnbao@...il.com>
To: david@...hat.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 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;
 	order = get_order(size);
 	if ((1 << order) & ~THP_ORDERS_ALL_ANON)
>
> --
> Cheers,
>
> David / dhildenb
>

Thanks
Barry