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

From: David Hildenbrand
Date: Fri Aug 16 2024 - 05:34:57 EST


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