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

[David Hildenbrand (Arm)]

On 10/29/26 02:48, Gregory Price wrote:
> On 2026-10-27 00:17 +0100, 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()")
>> Signed-off-by: David Hildenbrand (Arm) <david@xxxxxxxxxx>
> 
> This patch is painful. It's taking me a bit to wrap my head around.

The key here is that we unfuck (sorry) what we over-engineered before.

We just to the same locking independent of VMA flags. But if the trylock fails
(which we have to do because of possible deadlocks) we'll drop both locks and
just do another lookup.

If we raced with someone messing with the page tables in the meantime (no longer
empty), we give up.

> 
> I get that you want to avoid taking both locks while zapping, but
> wouldn't it be simpler?  I suppose we don't know what kind of
> regressions might pop up with that though, bah.

Exactly, that's the spirit of the original code. We would just block a 1GiB
range of va space for the entire operation.

If you think there is a more elegant way of doing it (while minimizing holding
the pmd lock), please shoot :)

> 
> I can say that I've reproduced and tested the before/after state,
> and it looks stable.
Thanks!

-- 
Cheers,

David