On 2025/8/21 22:18, Lorenzo Stoakes wrote:

On Tue, Aug 19, 2025 at 07:42:02AM -0600, Nico Pache wrote:

quoted

From: Baolin Wang <baolin.wang@linux.alibaba.com>

When only non-PMD-sized mTHP is enabled (such as only 64K mTHP enabled),

I don't think this example is very useful, probably just remove it.

Also 'non-PMD-sized mTHP' implies there is such a thing as PMD-sized mTP :)

quoted

we should also allow kicking khugepaged to attempt scanning and collapsing

What is kicking? I think this should be rephrased to something like 'we should
also allow khugepaged to attempt scanning...'

quoted

64K mTHP. Modify hugepage_pmd_enabled() to support mTHP collapse, and

64K mTHP -> "of mTHP ranges". Put the 'Modify...' bit in a new paragraph to
be clear.

quoted

while we are at it, rename it to make the function name more clear.

To make this clearer let me suggest:

	In order for khugepaged to operate when only mTHP sizes are
	specified in sysfs, we must modify the predicate function that
	determines whether it ought to run to do so.

	This function is currently called hugepage_pmd_enabled(), this
	patch renames it to hugepage_enabled() and updates the logic to
	check to determine whether any valid orders may exist which would
	justify khugepaged running.

Thanks. This looks good to me.

quoted

Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
Signed-off-by: Nico Pache <npache@redhat.com>

quoted

---
  mm/khugepaged.c | 20 ++++++++++----------
  1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 2cadd07341de..81d2ffd56ab9 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c

@@ -430,7 +430,7 @@ static inline int collapse_test_exit_or_disable(struct mm_struct *mm)
  		mm_flags_test(MMF_DISABLE_THP_COMPLETELY, mm);
  }

-static bool hugepage_pmd_enabled(void)
+static bool hugepage_enabled(void)
  {
  	/*
  	 * We cover the anon, shmem and the file-backed case here; file-backed

@@ -442,11 +442,11 @@ static bool hugepage_pmd_enabled(void)

The comment above this still references PMD-sized, please make sure to update
comments when you change the described behaviour, as it is now incorrect:

	/*
	 * We cover the anon, shmem and the file-backed case here; file-backed
	 * hugepages, when configured in, are determined by the global control.
	 * Anon pmd-sized hugepages are determined by the pmd-size control.
	 * Shmem pmd-sized hugepages are also determined by its pmd-size control,
	 * except when the global shmem_huge is set to SHMEM_HUGE_DENY.
	 */

Please correct this.

Sure. How about:

/*
  * We cover the anon, shmem and the file-backed case here; file-backed
  * hugepages, when configured in, are determined by the global control.
  * Anon hugepages are determined by its per-size mTHP control.
  * Shmem pmd-sized hugepages are also determined by its pmd-size control,
  * except when the global shmem_huge is set to SHMEM_HUGE_DENY.
  */

quoted

  	if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
  	    hugepage_global_enabled())
  		return true;
-	if (test_bit(PMD_ORDER, &huge_anon_orders_always))
+	if (READ_ONCE(huge_anon_orders_always))
  		return true;
-	if (test_bit(PMD_ORDER, &huge_anon_orders_madvise))
+	if (READ_ONCE(huge_anon_orders_madvise))
  		return true;
-	if (test_bit(PMD_ORDER, &huge_anon_orders_inherit) &&
+	if (READ_ONCE(huge_anon_orders_inherit) &&
  	    hugepage_global_enabled())

I guess READ_ONCE() is probably sufficient here as memory ordering isn't
important here, right?

Yes, I think so.