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

[Baolin Wang]

On 11/4/26 5:13 PM, David Hildenbrand (Arm) wrote:
> 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.
>
> As I pulled this patch out from the stable fixes, I fixed it up.

Thanks.

> I realized that this helper should likely move to some header later and be
> reused elsewehere (zap_pte_table_if_empty), but that's something for a later
> cleanup.

Indeed.