Re: [PATCH v3 18/47] mm/khugepaged: check if PTE table is empty in try_collapse_pte_mapped_thp()

[David Hildenbrand (Arm)]

On 11/2/26 09:20, Baolin Wang wrote:
> 
> 
> On 10/31/26 5:39 PM, David Hildenbrand (Arm) wrote:
>> The existing code is rather horrible and already resulted in bugs in the
>> past: we special-case UFFD inserting anonymous folios by grabbing the PMD
>> lock early, but ignore any other markers (guard, poison through uffd) that
>> could have been inserted into the page table in the meantime, effectively
>> dropping them.
>>
>> Let's just unify and simplify the locking: try to grab the PMD table
>> lock and if that fails, flush the TLB, drop the PTE lock, take PT locks in
>> the right order and check if the page table is still empty.
>>
>> We still perform the heavy lifting (zapping the folio ptes) under PTE table
>> lock only.
>>
>> Include <linux/hugetlb.h> for now, as x86's flush_tlb_range() macro
>> depends on hugetlb helpers. This dependency needs a separate cleanup.
>>
>> Both issues this fixes are rather corner cases. Let's not CC stable for
>> now.
>>
>> Fixes: f807123d578d ("mm: allow guard regions in file-backed and read-only
>> mappings")
>> Fixes: 1043173eb5eb ("mm/khugepaged: collapse_pte_mapped_thp() with
>> mmap_read_lock()")
>> Tested-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
>> Reviewed-by: Gregory Price (Meta) <gourry@xxxxxxxxxx>
>> Tested-by: Muhammad Usama Anjum <usama.anjum@xxxxxxx>
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
>> ---
>>   mm/khugepaged.c | 64 ++++++++++++++++++++++++++++-----------------------------
>>   1 file changed, 31 insertions(+), 33 deletions(-)
>>
>> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
>> index ce794d9760a87..ad07c8caa3bd9 100644
>> --- a/mm/khugepaged.c
>> +++ b/mm/khugepaged.c
>> @@ -10,6 +10,7 @@
>>   #include <linux/mm_inline.h>
>>   #include <linux/kthread.h>
>>   #include <linux/khugepaged.h>
>> +#include <linux/hugetlb.h>
>>   #include <linux/freezer.h>
>>   #include <linux/mman.h>
>>   #include <linux/hashtable.h>
>> @@ -1832,6 +1833,17 @@ static enum scan_result set_huge_pmd(struct
>> vm_area_struct *vma, unsigned long a
>>       return SCAN_SUCCEED;
>>   }
>>   +static bool pte_table_empty(pte_t *pte)
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < HPAGE_PMD_NR; i++, pte++) {
> 
> Using PTRS_PER_PTE to iterate PTEs seems more straightforward? Otherwise LGTM.

The difference currently shouldn't matter.

I'll probably leave it as is for no such that I can move forward with
upstreaming them soonish (having the code as is in -next for a couple of days).

But this might be reasonable to clean up alter, thanks!

-- 
Cheers,

David