Re: [PATCH 7.2 v2 05/12] mm/khugepaged: remove READ_ONLY_THP_FOR_FS check in hugepage_pmd_enabled()

["David Hildenbrand (Arm)" <david@kernel.org>]

On 4/13/26 22:42, Zi Yan wrote:
> On 13 Apr 2026, at 16:33, Matthew Wilcox wrote:
> 
>> On Mon, Apr 13, 2026 at 03:20:23PM -0400, Zi Yan wrote:
>>> After READ_ONLY_THP_FOR_FS Kconfig is removed, this check becomes dead
>>> code.
>>>
>>> This changes hugepage_pmd_enabled() semantics. Previously, with
>>> READ_ONLY_THP_FOR_FS enabled, hugepage_pmd_enabled() returned true whenever
>>> /sys/kernel/mm/transparent_hugepage/enabled was set to "always" or
>>> "madvise".
>>>
>>> After this change, hugepage_pmd_enabled() is governed only by the anon and
>>> shmem PMD THP controls. As a result, khugepaged collapse for file-backed
>>> folios no longer runs unconditionally under the top-level THP setting, and
>>> now depends on the anon/shmem PMD configuration.
>>
>> This seems like it'll turn off khugepaged too easily.  I would have
>> thought we'd want:
>>
>> -	if (IS_ENABLED(CONFIG_READ_ONLY_THP_FOR_FS) &&
>> -	    hugepage_global_enabled())
>> +	if (hugepage_global_enabled())
>>  		return true;
>

I assume such a change should come before patch #4, as it seems to affect
the functionality that depended on CONFIG_READ_ONLY_THP_FOR_FS.
 
> I thought about this, but it means khugepaged is turned on regardless of
> anon and shmem configs. I tend to think the original code was a bug,
> since enabling CONFIG_READ_ONLY_THP_FOR_FS would enable khugepaged all
> the time.

There might be some FS mapping to collapse? So that makes sense to
some degree.

I really don't like the side-effects of "/sys/kernel/mm/transparent_hugepage/enabled".
Like, enabling khugepaged+PMD for files.

> 
>>
>> ... or maybe this whole thing could be simplified?
> 
> Alternatives could be:
> 1. to add a file-backed khhugepaged config, but another sysfs?

Maybe that would be the time to decouple file THP logic from
hugepage_global_enabled()/hugepage_global_always().

In particular, as pagecache folio allocation doesn't really care about __thp_vma_allowable_orders() IIRC.

I'm thinking about something like the following:

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index b2a6060b3c20..fb3a4fd84fe0 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -184,15 +184,6 @@ unsigned long __thp_vma_allowable_orders(struct vm_area_struct *vma,
                                                   forced_collapse);
 
        if (!vma_is_anonymous(vma)) {
-               /*
-                * Enforce THP collapse requirements as necessary. Anonymous vmas
-                * were already handled in thp_vma_allowable_orders().
-                */
-               if (!forced_collapse &&
-                   (!hugepage_global_enabled() || (!(vm_flags & VM_HUGEPAGE) &&
-                                                   !hugepage_global_always())))
-                       return 0;
-
                /*
                 * Trust that ->huge_fault() handlers know what they are doing
                 * in fault path.

Then, we might indeed just want a khugepaged toggle whether to enable it at
all in files. (or just a toggle to disable khugeapged entirely?)

-- 
Cheers,

David