synchronously scan and reclaim empty user PTE pages

In retract_page_tables(), we may modify the pmd entry after acquiring the pml and ptl, so we should also check whether the pmd entry is stable.

Using pte_offset_map_rw_nolock() + pmd_same() to do it, and then we can also remove the calling of the pte_lockptr().

Signed-off-by Qi Zheng <[email protected]>

---
 mm/khugepaged.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

mm/khugepaged.c +1 -0

index 6f8d46d107b4b..6d76dde64f5fb 100644
@@ -1721,6 +1721,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                spinlock_t *pml;
                spinlock_t *ptl;
                bool skipped_uffd = false;
+               pte_t *pte;
 
                /*
                 * Check vma->anon_vma to exclude MAP_PRIVATE mappings that

mm/khugepaged.c +9 -2

-               ptl = pte_lockptr(mm, pmd);
                if (ptl != pml)
                        spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
 
+               if (unlikely(!pmd_same(pgt_pmd, pmdp_get_lockless(pmd)))) {
+                       pte_unmap_unlock(pte, ptl);
+                       if (ptl != pml)
+                               spin_unlock(pml);
+                       mmu_notifier_invalidate_range_end(&range);
+                       continue;
+               }
+               pte_unmap(pte);
+
                /*
                 * Huge page lock is still held, so normally the page table
                 * must remain empty; and we have already skipped anon_vma
--

2.20.1

Quoted 2 lines · Qi Zheng

In retract_page_tables(), we may modify the pmd entry after acquiring the pml and ptl, so we should also check whether the pmd entry is stable.

Why does taking the PMD lock not guarantee that the PMD entry is stable?

Quoted 25 lines

Using pte_offset_map_rw_nolock() + pmd_same() to do it, and then we can also remove the calling of the pte_lockptr().

Signed-off-by Qi Zheng <[email protected]>

---
  mm/khugepaged.c | 17 ++++++++++++++++-
  1 file changed, 16 insertions(+), 1 deletion(-)

mm/khugepaged.c +1 -0

index 6f8d46d107b4b..6d76dde64f5fb 100644
@@ -1721,6 +1721,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                 spinlock_t *pml;
                 spinlock_t *ptl;
                 bool skipped_uffd = false;
+               pte_t *pte;
 
                 /*
                  * Check vma->anon_vma to exclude MAP_PRIVATE mappings that

@@ -1756,11 +1757,25 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                                         addr, addr + HPAGE_PMD_SIZE);
                 mmu_notifier_invalidate_range_start(&range);
 

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 1766

Quoted 7 lines

mm/khugepaged.c +6 -0

+               pte = pte_offset_map_rw_nolock(mm, pmd, addr, &pgt_pmd, &ptl);
+               if (!pte) {
+                       mmu_notifier_invalidate_range_end(&range);
+                       continue;
+               }
+
                 pml = pmd_lock(mm, pmd);

I don't understand why you're mapping the page table before locking the PMD. Doesn't that just mean we need more error checking afterwards?

Quoted 21 lines

mm/khugepaged.c +9 -2

-               ptl = pte_lockptr(mm, pmd);
                 if (ptl != pml)
                         spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
 
+               if (unlikely(!pmd_same(pgt_pmd, pmdp_get_lockless(pmd)))) {
+                       pte_unmap_unlock(pte, ptl);
+                       if (ptl != pml)
+                               spin_unlock(pml);
+                       mmu_notifier_invalidate_range_end(&range);
+                       continue;
+               }
+               pte_unmap(pte);
+
                 /*
                  * Huge page lock is still held, so normally the page table
                  * must remain empty; and we have already skipped anon_vma
--

2.20.1

Quoted 5 lines · Jann Horn

Quoted 2 lines · Qi Zheng

In retract_page_tables(), we may modify the pmd entry after acquiring the pml and ptl, so we should also check whether the pmd entry is stable.

Why does taking the PMD lock not guarantee that the PMD entry is stable?

Because the pmd entry may have changed before taking the pmd lock, so we need to recheck it after taking the pmd or pte lock.

Quoted 24 lines

Quoted 24 lines

Using pte_offset_map_rw_nolock() + pmd_same() to do it, and then we can also remove the calling of the pte_lockptr().

Signed-off-by Qi Zheng <[email protected]>

---
  mm/khugepaged.c | 17 ++++++++++++++++-
  1 file changed, 16 insertions(+), 1 deletion(-)

mm/khugepaged.c +1 -0

index 6f8d46d107b4b..6d76dde64f5fb 100644
@@ -1721,6 +1721,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                 spinlock_t *pml;
                 spinlock_t *ptl;
                 bool skipped_uffd = false;
+               pte_t *pte;
 
                 /*
                  * Check vma->anon_vma to exclude MAP_PRIVATE mappings that

@@ -1756,11 +1757,25 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                                         addr, addr + HPAGE_PMD_SIZE);
                 mmu_notifier_invalidate_range_start(&range);
 

Quoted 11 lines

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 1766

Quoted 7 lines

mm/khugepaged.c +6 -0

+               pte = pte_offset_map_rw_nolock(mm, pmd, addr, &pgt_pmd, &ptl);
+               if (!pte) {
+                       mmu_notifier_invalidate_range_end(&range);
+                       continue;
+               }
+
                 pml = pmd_lock(mm, pmd);

I don't understand why you're mapping the page table before locking the PMD. Doesn't that just mean we need more error checking afterwards?

The main purpose is to obtain the pmdval. If we don't use pte_offset_map_rw_nolock, we should pay attention to recheck pmd entry before pte_lockptr(), like this:

pmdval = pmdp_get_lockless(pmd);
pmd_lock
recheck pmdval
pte_lockptr(mm, pmd)

Otherwise, it may cause the system to crash. Consider the following situation:

CPU 0 CPU 1

zap_pte_range
--> clear pmd entry

     free pte page (by RCU)

                       retract_page_tables
                       --> pmd_lock
                           pte_lockptr(mm, pmd)  <-- BOOM!!

So maybe calling pte_offset_map_rw_nolock() is more convenient.

Quoted 9 lines · Qi Zheng

Quoted 5 lines · Jann Horn

Quoted 2 lines · Qi Zheng

In retract_page_tables(), we may modify the pmd entry after acquiring the pml and ptl, so we should also check whether the pmd entry is stable.

Why does taking the PMD lock not guarantee that the PMD entry is stable?

Because the pmd entry may have changed before taking the pmd lock, so we need to recheck it after taking the pmd or pte lock.

You mean it could have changed from the value we obtained from find_pmd_or_thp_or_none(mm, addr, &pmd)? I don't think that matters though.

Quoted 52 lines

Quoted 24 lines

Quoted 24 lines

Using pte_offset_map_rw_nolock() + pmd_same() to do it, and then we can also remove the calling of the pte_lockptr().

Signed-off-by Qi Zheng <[email protected]>

---
   mm/khugepaged.c | 17 ++++++++++++++++-
   1 file changed, 16 insertions(+), 1 deletion(-)

mm/khugepaged.c +1 -0

index 6f8d46d107b4b..6d76dde64f5fb 100644
@@ -1721,6 +1721,7 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                  spinlock_t *pml;
                  spinlock_t *ptl;
                  bool skipped_uffd = false;
+               pte_t *pte;
 
                  /*
                   * Check vma->anon_vma to exclude MAP_PRIVATE mappings that

@@ -1756,11 +1757,25 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
                                          addr, addr + HPAGE_PMD_SIZE);
                  mmu_notifier_invalidate_range_start(&range);
 

Quoted 11 lines

mm/khugepaged.c static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) · line 1766

Quoted 7 lines

mm/khugepaged.c +6 -0

+               pte = pte_offset_map_rw_nolock(mm, pmd, addr, &pgt_pmd, &ptl);
+               if (!pte) {
+                       mmu_notifier_invalidate_range_end(&range);
+                       continue;
+               }
+
                  pml = pmd_lock(mm, pmd);

I don't understand why you're mapping the page table before locking the PMD. Doesn't that just mean we need more error checking afterwards?

The main purpose is to obtain the pmdval. If we don't use pte_offset_map_rw_nolock, we should pay attention to recheck pmd entry before pte_lockptr(), like this:

pmdval = pmdp_get_lockless(pmd);
pmd_lock
recheck pmdval
pte_lockptr(mm, pmd)

Otherwise, it may cause the system to crash. Consider the following situation:

CPU 0 CPU 1

zap_pte_range
--> clear pmd entry

Quoted 7 lines

      free pte page (by RCU)

                        retract_page_tables
                        --> pmd_lock
                            pte_lockptr(mm, pmd)  <-- BOOM!!

So maybe calling pte_offset_map_rw_nolock() is more convenient.

How about refactoring find_pmd_or_thp_or_none() like this, by moving the checks of the PMD entry value into a separate helper:

(quoted diff) +13 -9

-static int find_pmd_or_thp_or_none(struct mm_struct *mm,
-                                  unsigned long address,
-                                  pmd_t **pmd)
+static int check_pmd_state(pmd_t *pmd)
  {
-       pmd_t pmde;
+       pmd_t pmde = pmdp_get_lockless(*pmd);
 
-       *pmd = mm_find_pmd(mm, address);
-       if (!*pmd)
-               return SCAN_PMD_NULL;
-
-       pmde = pmdp_get_lockless(*pmd);
         if (pmd_none(pmde))
                 return SCAN_PMD_NONE;
         if (!pmd_present(pmde))
                 return SCAN_PMD_NULL;
         if (pmd_trans_huge(pmde))
                 return SCAN_PMD_MAPPED;
         if (pmd_devmap(pmde))
                 return SCAN_PMD_NULL;
         if (pmd_bad(pmde))
                 return SCAN_PMD_NULL;
         return SCAN_SUCCEED;
  }
 
+static int find_pmd_or_thp_or_none(struct mm_struct *mm,
+                                  unsigned long address,
+                                  pmd_t **pmd)
+{
+
+       *pmd = mm_find_pmd(mm, address);
+       if (!*pmd)
+               return SCAN_PMD_NULL;
+       return check_pmd_state(*pmd);
+}
+

And simplifying retract_page_tables() a little bit like this:

         i_mmap_lock_read(mapping);
         vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) {
                 struct mmu_notifier_range range;
                 struct mm_struct *mm;
                 unsigned long addr;
                 pmd_t *pmd, pgt_pmd;
                 spinlock_t *pml;
                 spinlock_t *ptl;

(quoted diff) +5 -5

-               bool skipped_uffd = false;
+               bool success = false;
 
                 /*
                  * Check vma->anon_vma to exclude MAP_PRIVATE mappings that
                  * got written to. These VMAs are likely not worth removing
                  * page tables from, as PMD-mapping is likely to be split later.
                  */
                 if (READ_ONCE(vma->anon_vma))
                         continue;
 
                 addr = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT);

@@ -1763,34 +1767,34 @@ static void retract_page_tables(struct
 address_space *mapping, pgoff_t pgoff)
 
                 /*
                  * Huge page lock is still held, so normally the page table
                  * must remain empty; and we have already skipped anon_vma
                  * and userfaultfd_wp() vmas.  But since the mmap_lock is not
                  * held, it is still possible for a racing userfaultfd_ioctl()
                  * to have inserted ptes or markers.  Now that we hold ptlock,
                  * repeating the anon_vma check protects from one category,
                  * and repeating the userfaultfd_wp() check from another.
                  */
-               if (unlikely(vma->anon_vma || userfaultfd_wp(vma))) {
-                       skipped_uffd = true;
-               } else {
+               if (likely(!vma->anon_vma && !userfaultfd_wp(vma))) {
                         pgt_pmd = pmdp_collapse_flush(vma, addr, pmd);
                         pmdp_get_lockless_sync();
+                       success = true;
                 }
 
                 if (ptl != pml)
                         spin_unlock(ptl);
+drop_pml:
                 spin_unlock(pml);
 
                 mmu_notifier_invalidate_range_end(&range);
 
-               if (!skipped_uffd) {
+               if (success) {
                         mm_dec_nr_ptes(mm);
                         page_table_check_pte_clear_range(mm, addr, pgt_pmd);
                         pte_free_defer(mm, pmd_pgtable(pgt_pmd));
                 }
         }
         i_mmap_unlock_read(mapping);
 
 

And then instead of your patch, I think you can just do this?

(quoted diff) +2 -0

@@ -1754,20 +1754,22 @@ static void retract_page_tables(struct
 address_space *mapping, pgoff_t pgoff)
                  */
                 if (userfaultfd_wp(vma))
                         continue;
 
                 /* PTEs were notified when unmapped; but now for the PMD? */
                 mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, mm,
                                         addr, addr + HPAGE_PMD_SIZE);
                 mmu_notifier_invalidate_range_start(&range);
 
                 pml = pmd_lock(mm, pmd);
+               if (check_pmd_state(mm, addr, pmd) != SCAN_SUCCEED)
+                       goto drop_pml;
                 ptl = pte_lockptr(mm, pmd);
                 if (ptl != pml)
                         spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
 
                 /*
                  * Huge page lock is still held, so normally the page table
                  * must remain empty; and we have already skipped anon_vma
                  * and userfaultfd_wp() vmas.  But since the mmap_lock is not
                  * held, it is still possible for a racing userfaultfd_ioctl()

* to have inserted ptes or markers. Now that we hold ptlock,

Similar to zap_present_ptes(), let's introduce zap_nonpresent_ptes() to handle non-present ptes, which can improve code readability.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

Similar to zap_present_ptes(), let's introduce zap_nonpresent_ptes() to handle non-present ptes, which can improve code readability.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
  mm/memory.c | 45 ++++++++++++++++++++++++++-------------------
  1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +1 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct mmu_gather *tlb,
  	return nr;
  }
 
+static inline int do_zap_pte_range(struct mmu_gather *tlb,

Quoted 24 lines · Qi Zheng

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
  mm/memory.c | 45 ++++++++++++++++++++++++++-------------------

  1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +5 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct 
 mmu_gather *tlb,
       return nr;
   }
+static inline int do_zap_pte_range(struct mmu_gather *tlb,
+                   struct vm_area_struct *vma, pte_t *pte,
+                   unsigned long addr, unsigned long end,
+                   struct zap_details *details, int *rss,
+                   bool *force_flush, bool *force_break)

mm/memory.c static inline int zap_nonpresent_ptes(struct · line 1670

Quoted 6 lines

mm/memory.c +6 -0

+{
+    pte_t ptent = ptep_get(pte);
+    int max_nr = (end - addr) / PAGE_SIZE;
+
+    if (pte_none(ptent))
+        return 1;

Maybe we should just skip all applicable pte_none() here directly.

Acked-by David Hildenbrand <[email protected]>

Quoted 25 lines · David Hildenbrand

Quoted 24 lines · Qi Zheng

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
   mm/memory.c | 45 ++++++++++++++++++++++++++-------------------
   1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +5 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct
 mmu_gather *tlb,
       return nr;
   }
+static inline int do_zap_pte_range(struct mmu_gather *tlb,
+                   struct vm_area_struct *vma, pte_t *pte,
+                   unsigned long addr, unsigned long end,
+                   struct zap_details *details, int *rss,
+                   bool *force_flush, bool *force_break)

Quoted 8 lines

mm/memory.c static inline int zap_nonpresent_ptes(struct · line 1670

Quoted 6 lines

mm/memory.c +6 -0

+{
+    pte_t ptent = ptep_get(pte);
+    int max_nr = (end - addr) / PAGE_SIZE;
+
+    if (pte_none(ptent))
+        return 1;

Maybe we should just skip all applicable pte_none() here directly.

Do you mean we should keep pte_none() case in zap_pte_range()? Like below:

Quoted 37 lines · Qi Zheng

Quoted 25 lines · David Hildenbrand

Quoted 24 lines · Qi Zheng

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
   mm/memory.c | 45 ++++++++++++++++++++++++++-------------------

   1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +5 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct
 mmu_gather *tlb,
        return nr;
    }
+static inline int do_zap_pte_range(struct mmu_gather *tlb,
+                   struct vm_area_struct *vma, pte_t *pte,
+                   unsigned long addr, unsigned long end,
+                   struct zap_details *details, int *rss,
+                   bool *force_flush, bool *force_break)

Quoted 8 lines

mm/memory.c static inline int zap_nonpresent_ptes(struct · line 1670

Quoted 6 lines

mm/memory.c +6 -0

+{
+    pte_t ptent = ptep_get(pte);
+    int max_nr = (end - addr) / PAGE_SIZE;
+
+    if (pte_none(ptent))
+        return 1;

Maybe we should just skip all applicable pte_none() here directly.

Quoted 3 lines

Do you mean we should keep pte_none() case in zap_pte_range()? Like below:

No rather an addon patch that will simply skip over all consecutive pte_none, like:

if (pte_none(ptent)) {
    int nr;

    for (nr = 1; nr < max_nr; nr++) {
        ptent = ptep_get(pte + nr);
        if (pte_none(ptent))
            continue;
    }

    max_nr -= nr;
    if (!max_nr)
        return nr;
    addr += nr * PAGE_SIZE;
    pte += nr;
}

Assuming that it's likely more common to have larger pte_none() holes that single ones, optimizing out the need_resched()+force_break+incremental pte/addr increments etc.

Quoted 37 lines · Qi Zheng

Quoted 25 lines · David Hildenbrand

Quoted 24 lines · Qi Zheng

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
   mm/memory.c | 45 ++++++++++++++++++++++++++-------------------

   1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +5 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct
 mmu_gather *tlb,
        return nr;
    }
+static inline int do_zap_pte_range(struct mmu_gather *tlb,
+                   struct vm_area_struct *vma, pte_t *pte,
+                   unsigned long addr, unsigned long end,
+                   struct zap_details *details, int *rss,
+                   bool *force_flush, bool *force_break)

Quoted 8 lines

mm/memory.c static inline int zap_nonpresent_ptes(struct · line 1670

Quoted 6 lines

mm/memory.c +6 -0

+{
+    pte_t ptent = ptep_get(pte);
+    int max_nr = (end - addr) / PAGE_SIZE;
+
+    if (pte_none(ptent))
+        return 1;

Maybe we should just skip all applicable pte_none() here directly.

Quoted 3 lines

Do you mean we should keep pte_none() case in zap_pte_range()? Like below:

No rather an addon patch that will simply skip over all consecutive pte_none, like:

if (pte_none(ptent)) {
    int nr;

    for (nr = 1; nr < max_nr; nr++) {
        ptent = ptep_get(pte + nr);
        if (pte_none(ptent))
            continue;
    }

    max_nr -= nr;
    if (!max_nr)
        return nr;
    addr += nr * PAGE_SIZE;
    pte += nr;
}

Assuming that it's likely more common to have larger pte_none() holes that single ones, optimizing out the need_resched()+force_break+incremental pte/addr increments etc.

Quoted 54 lines · David Hildenbrand

Quoted 37 lines · Qi Zheng

Quoted 25 lines · David Hildenbrand

Quoted 24 lines · Qi Zheng

This commit introduces do_zap_pte_range() to actually zap the PTEs, which will help improve code readability and facilitate secondary checking of the processed PTEs in the future.

No functional change.

Signed-off-by Qi Zheng <[email protected]>

---
   mm/memory.c | 45 ++++++++++++++++++++++++++-------------------

   1 file changed, 26 insertions(+), 19 deletions(-)

mm/memory.c +5 -0

index bd9ebe0f4471f..c1150e62dd073 100644
@@ -1657,6 +1657,27 @@ static inline int zap_nonpresent_ptes(struct
 mmu_gather *tlb,
        return nr;
    }
+static inline int do_zap_pte_range(struct mmu_gather *tlb,
+                   struct vm_area_struct *vma, pte_t *pte,
+                   unsigned long addr, unsigned long end,
+                   struct zap_details *details, int *rss,
+                   bool *force_flush, bool *force_break)

Quoted 8 lines

mm/memory.c static inline int zap_nonpresent_ptes(struct · line 1670

Quoted 6 lines

mm/memory.c +6 -0

+{
+    pte_t ptent = ptep_get(pte);
+    int max_nr = (end - addr) / PAGE_SIZE;
+
+    if (pte_none(ptent))
+        return 1;

Maybe we should just skip all applicable pte_none() here directly.

Quoted 3 lines

Do you mean we should keep pte_none() case in zap_pte_range()? Like below:

No rather an addon patch that will simply skip over all consecutive pte_none, like:

if (pte_none(ptent)) {
     int nr;

     for (nr = 1; nr < max_nr; nr++) {
         ptent = ptep_get(pte + nr);
         if (pte_none(ptent))
             continue;
     }

Quoted 6 lines

     max_nr -= nr;
     if (!max_nr)
         return nr;
     addr += nr * PAGE_SIZE;
     pte += nr;
}

I tend to hand over the pte/addr increments here to the loop outside do_zap_pte_range(), like this:

In preparation for reclaiming empty PTE pages, this commit first makes zap_pte_range() to handle the full within-PMD range, so that we can more easily detect and free PTE pages in this function in subsequent commits.

Signed-off-by Qi Zheng <[email protected]>

As a first step, this commit aims to synchronously free the empty PTE pages in madvise(MADV_DONTNEED) case. We will detect and free empty PTE pages in zap_pte_range(), and will add zap_details.reclaim_pt to exclude cases other than madvise(MADV_DONTNEED).

Once an empty PTE is detected, we first try to hold the pmd lock within the pte lock. If successful, we clear the pmd entry directly (fast path). Otherwise, we wait until the pte lock is released, then re-hold the pmd and pte locks and loop PTRS_PER_PTE times to check pte_none() to re-detect whether the PTE page is empty and free it (slow path).

Signed-off-by Qi Zheng <[email protected]>

---
 include/linux/mm.h |  1 +
 mm/Kconfig         | 15 ++++++++++
 mm/Makefile        |  1 +
 mm/internal.h      | 23 ++++++++++++++++
 mm/madvise.c       |  4 ++-
 mm/memory.c        | 45 +++++++++++++++++++++++++++++-
 mm/pt_reclaim.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 155 insertions(+), 2 deletions(-)
 create mode 100644 mm/pt_reclaim.c

include/linux/mm.h +1 -0

index 3e4bb43035953..ce3936590fe72 100644
@@ -2319,6 +2319,7 @@ extern void pagefault_out_of_memory(void);
 struct zap_details {
        struct folio *single_folio;     /* Locked folio to be unmapped */
        bool even_cows;                 /* Zap COWed private pages too? */
+       bool reclaim_pt;                /* Need reclaim page tables? */
        zap_flags_t zap_flags;          /* Extra flags for zapping */
 };
 

mm/Kconfig +0 -0

index 84000b0168086..681909e0a9fa3 100644
@@ -1301,6 +1301,21 @@ config ARCH_HAS_USER_SHADOW_STACK
          The architecture has hardware support for userspace shadow call
           stacks (eg, x86 CET, arm64 GCS or RISC-V Zicfiss).
 

mm/Kconfig +3 -0

+         and exit_mmap path.
+
+         Note: now only empty user PTE page table pages will be reclaimed.

mm/madvise.c +3 -1

index 0ceae57da7dad..ee88652761d45 100644
@@ -851,7 +851,9 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
 static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
                                        unsigned long start, unsigned long end)
 {
-       zap_page_range_single(vma, start, end - start, NULL);
+       struct zap_details details = {.reclaim_pt = true,};
+
+       zap_page_range_single(vma, start, end - start, &details);
        return 0;
 }
 

mm/memory.c +0 -0

index 002aa4f454fa0..c4a8c18fbcfd7 100644

mm/memory.c +34 -0

+#ifdef CONFIG_PTE_MARKER_UFFD_WP
+       for (;;) {
+               if (pte_none(ptep_get(pte)))
+                       none_nr++;
+               if (--nr == 0)
+                       break;
+               pte++;
+       }
+#else
+       if (pte_none(ptep_get(pte)))
+               none_nr = nr;
+#endif
+       return none_nr;
+}
+
 static unsigned long zap_pte_range(struct mmu_gather *tlb,
                                struct vm_area_struct *vma, pmd_t *pmd,
                                unsigned long addr, unsigned long end,

@@ -1689,8 +1713,16 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
        spinlock_t *ptl;
        pte_t *start_pte;
        pte_t *pte;
+       pmd_t pmdval;
+       bool can_reclaim_pt = false;
+       bool direct_reclaim = false;
+       unsigned long start = addr;
+       int none_nr = 0;
        int nr;
 
+       if (details && details->reclaim_pt && (end - start >= PMD_SIZE))
+               can_reclaim_pt = true;
+
 retry:
        tlb_change_page_size(tlb, PAGE_SIZE);
        init_rss_vec(rss);

@@ -1706,12 +1738,16 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 
                nr = do_zap_pte_range(tlb, vma, pte, addr, end, details, rss,
                                      &force_flush, &force_break);
+               none_nr += count_pte_none(pte, nr);
                if (unlikely(force_break)) {
                        addr += nr * PAGE_SIZE;
                        break;
                }
        } while (pte += nr, addr += PAGE_SIZE * nr, addr != end);
 
+       if (addr == end && can_reclaim_pt && (none_nr == PTRS_PER_PTE))
+               direct_reclaim = try_get_and_clear_pmd(mm, pmd, &pmdval);
+
        add_mm_rss_vec(mm, rss);
        arch_leave_lazy_mmu_mode();
 

@@ -1738,6 +1774,13 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
                goto retry;
        }
 
+       if (can_reclaim_pt) {
+               if (direct_reclaim)
+                       free_pte(mm, start, tlb, pmdval);
+               else
+                       try_to_free_pte(mm, pmd, start, tlb);
+       }
+
        return addr;
 }
 

mm/pt_reclaim.c new file +26 -0

new file mode 100644
index 0000000000000..fc055da40b615
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/hugetlb.h>
+#include <asm-generic/tlb.h>
+#include <asm/pgalloc.h>
+
+#include "internal.h"
+
+bool try_get_and_clear_pmd(struct mm_struct *mm, pmd_t *pmd, pmd_t *pmdval)
+{
+       spinlock_t *pml = pmd_lockptr(mm, pmd);
+
+       if (!spin_trylock(pml))
+               return false;
+
+       *pmdval = pmdp_get_lockless(pmd);
+       pmd_clear(pmd);
+       spin_unlock(pml);
+
+       return true;
+}
+
+void free_pte(struct mm_struct *mm, unsigned long addr, struct mmu_gather *tlb,
+             pmd_t pmdval)
+{
+       pte_free_tlb(tlb, pmd_pgtable(pmdval), addr);
+       mm_dec_nr_ptes(mm);

mm/pt_reclaim.c new file +24 -1

+       if (unlikely(!pmd_same(pmdval, pmdp_get_lockless(pmd))))
+               goto out_ptl;
+
+       /* Check if it is empty PTE page */
+       for (i = 0, pte = start_pte; i < PTRS_PER_PTE; i++, pte++) {
+               if (!pte_none(ptep_get(pte)))
+                       goto out_ptl;
+       }
+       pte_unmap(start_pte);
+
+       pmd_clear(pmd);
+
+       if (ptl != pml)
+               spin_unlock(ptl);
+       spin_unlock(pml);
+
+       free_pte(mm, addr, tlb, pmdval);
+
+       return;
+out_ptl:
+       pte_unmap_unlock(start_pte, ptl);
+       if (pml != ptl)
+               spin_unlock(pml);
+}
--

2.20.1

Quoted 10 lines · Qi Zheng

As a first step, this commit aims to synchronously free the empty PTE pages in madvise(MADV_DONTNEED) case. We will detect and free empty PTE pages in zap_pte_range(), and will add zap_details.reclaim_pt to exclude cases other than madvise(MADV_DONTNEED).

Once an empty PTE is detected, we first try to hold the pmd lock within the pte lock. If successful, we clear the pmd entry directly (fast path). Otherwise, we wait until the pte lock is released, then re-hold the pmd and pte locks and loop PTRS_PER_PTE times to check pte_none() to re-detect whether the PTE page is empty and free it (slow path).

How does this interact with move_pages_pte()? I am looking at your series applied on top of next-20241106, and it looks to me like move_pages_pte() uses pte_offset_map_rw_nolock() and assumes that the PMD entry can't change. You can clearly see this assumption at the WARN_ON_ONCE(pmd_none(*dst_pmd)). And if we race the wrong way, I

think for example move_present_pte() can end up moving a present PTE into a page table that has already been scheduled for RCU freeing.

Quoted 33 lines

Signed-off-by Qi Zheng <[email protected]>

---
  include/linux/mm.h |  1 +
  mm/Kconfig         | 15 ++++++++++
  mm/Makefile        |  1 +
  mm/internal.h      | 23 ++++++++++++++++
  mm/madvise.c       |  4 ++-
  mm/memory.c        | 45 +++++++++++++++++++++++++++++-
  mm/pt_reclaim.c    | 68 ++++++++++++++++++++++++++++++++++++++++++++++
  7 files changed, 155 insertions(+), 2 deletions(-)

create mode 100644 mm/pt_reclaim.c

include/linux/mm.h +1 -0

index 3e4bb43035953..ce3936590fe72 100644
@@ -2319,6 +2319,7 @@ extern void pagefault_out_of_memory(void);
  struct zap_details {
         struct folio *single_folio;     /* Locked folio to be unmapped */
         bool even_cows;                 /* Zap COWed private pages too? */
+       bool reclaim_pt;                /* Need reclaim page tables? */
         zap_flags_t zap_flags;          /* Extra flags for zapping */
  };
 

mm/Kconfig +0 -0

index 84000b0168086..681909e0a9fa3 100644
@@ -1301,6 +1301,21 @@ config ARCH_HAS_USER_SHADOW_STACK
           The architecture has hardware support for userspace shadow call
            stacks (eg, x86 CET, arm64 GCS or RISC-V Zicfiss).
 

mm/Kconfig config ARCH_HAS_USER_SHADOW_STACK · line 1313

Quoted 10 lines

mm/Kconfig +10 -0

+config ARCH_SUPPORTS_PT_RECLAIM
+       def_bool n
+
+config PT_RECLAIM
+       bool "reclaim empty user page table pages"
+       default y
+       depends on ARCH_SUPPORTS_PT_RECLAIM && MMU && SMP
+       select MMU_GATHER_RCU_TABLE_FREE
+       help
+         Try to reclaim empty user page table pages in paths other that munmap

nit: s/other that/other than/

Quoted 4 lines

mm/Kconfig +3 -0

+         and exit_mmap path.
+
+         Note: now only empty user PTE page table pages will be reclaimed.

[...]

Quoted 27 lines

mm/madvise.c +3 -1

index 0ceae57da7dad..ee88652761d45 100644
@@ -851,7 +851,9 @@ static int madvise_free_single_vma(struct vm_area_struct *vma,
  static long madvise_dontneed_single_vma(struct vm_area_struct *vma,
                                         unsigned long start, unsigned long end)
  {
-       zap_page_range_single(vma, start, end - start, NULL);
+       struct zap_details details = {.reclaim_pt = true,};
+
+       zap_page_range_single(vma, start, end - start, &details);
         return 0;
  }
 

mm/memory.c +1 -1

index 002aa4f454fa0..c4a8c18fbcfd7 100644
@@ -1436,7 +1436,7 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
  static inline bool should_zap_cows(struct zap_details *details)
  {
         /* By default, zap all pages */
-       if (!details)
+       if (!details || details->reclaim_pt)
                 return true;
 

mm/memory.c copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma) · line 1442

Quoted 1 line

mm/memory.c +0 -0

         /* Or, we zap COWed pages only if the caller wants to */

This looks hacky - when we have a "details" object, its ->even_cows member is supposed to indicate whether COW pages should be zapped. So please instead set .even_cows=true in madvise_dontneed_single_vma().

Quoted 5 lines

mm/memory.c +0 -0

@@ -1678,6 +1678,30 @@ static inline int do_zap_pte_range(struct mmu_gather *tlb,
                                          details, rss);
  }
 

mm/memory.c static inline int do_zap_pte_range(struct mmu_gather *tlb, · line 1689

Quoted 9 lines

mm/memory.c +9 -0

+static inline int count_pte_none(pte_t *pte, int nr)
+{
+       int none_nr = 0;
+
+       /*
+        * If PTE_MARKER_UFFD_WP is enabled, the uffd-wp PTEs may be
+        * re-installed, so we need to check pte_none() one by one.
+        * Otherwise, checking a single PTE in a batch is sufficient.
+        */

Please also add a comment noting that count_pte_none() relies on this invariant on top of do_zap_pte_range() or somewhere like that.

Quoted 104 lines

mm/memory.c +34 -0

+#ifdef CONFIG_PTE_MARKER_UFFD_WP
+       for (;;) {
+               if (pte_none(ptep_get(pte)))
+                       none_nr++;
+               if (--nr == 0)
+                       break;
+               pte++;
+       }
+#else
+       if (pte_none(ptep_get(pte)))
+               none_nr = nr;
+#endif
+       return none_nr;
+}
+
  static unsigned long zap_pte_range(struct mmu_gather *tlb,
                                 struct vm_area_struct *vma, pmd_t *pmd,
                                 unsigned long addr, unsigned long end,

@@ -1689,8 +1713,16 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
         spinlock_t *ptl;
         pte_t *start_pte;
         pte_t *pte;
+       pmd_t pmdval;
+       bool can_reclaim_pt = false;
+       bool direct_reclaim = false;
+       unsigned long start = addr;
+       int none_nr = 0;
         int nr;
 
+       if (details && details->reclaim_pt && (end - start >= PMD_SIZE))
+               can_reclaim_pt = true;
+
  retry:
         tlb_change_page_size(tlb, PAGE_SIZE);
         init_rss_vec(rss);

@@ -1706,12 +1738,16 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 
                 nr = do_zap_pte_range(tlb, vma, pte, addr, end, details, rss,
                                       &force_flush, &force_break);
+               none_nr += count_pte_none(pte, nr);
                 if (unlikely(force_break)) {
                         addr += nr * PAGE_SIZE;
                         break;
                 }
         } while (pte += nr, addr += PAGE_SIZE * nr, addr != end);
 
+       if (addr == end && can_reclaim_pt && (none_nr == PTRS_PER_PTE))
+               direct_reclaim = try_get_and_clear_pmd(mm, pmd, &pmdval);
+
         add_mm_rss_vec(mm, rss);
         arch_leave_lazy_mmu_mode();
 

@@ -1738,6 +1774,13 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
                 goto retry;
         }
 
+       if (can_reclaim_pt) {
+               if (direct_reclaim)
+                       free_pte(mm, start, tlb, pmdval);
+               else
+                       try_to_free_pte(mm, pmd, start, tlb);
+       }
+
         return addr;
  }
 

mm/pt_reclaim.c new file +31 -0

new file mode 100644
index 0000000000000..fc055da40b615
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/hugetlb.h>
+#include <asm-generic/tlb.h>
+#include <asm/pgalloc.h>
+
+#include "internal.h"
+
+bool try_get_and_clear_pmd(struct mm_struct *mm, pmd_t *pmd, pmd_t *pmdval)
+{
+       spinlock_t *pml = pmd_lockptr(mm, pmd);
+
+       if (!spin_trylock(pml))
+               return false;
+
+       *pmdval = pmdp_get_lockless(pmd);
+       pmd_clear(pmd);
+       spin_unlock(pml);
+
+       return true;
+}
+
+void free_pte(struct mm_struct *mm, unsigned long addr, struct mmu_gather *tlb,
+             pmd_t pmdval)
+{
+       pte_free_tlb(tlb, pmd_pgtable(pmdval), addr);
+       mm_dec_nr_ptes(mm);
+}
+
+void try_to_free_pte(struct mm_struct *mm, pmd_t *pmd, unsigned long addr,
+                    struct mmu_gather *tlb)
+{

mm/pt_reclaim.c line 36

Quoted 5 lines

mm/pt_reclaim.c new file +5 -0

+       pmd_t pmdval;
+       spinlock_t *pml, *ptl;
+       pte_t *start_pte, *pte;
+       int i;
+

If you swap the following two operations around (first pmd_lock(), then pte_offset_map_rw_nolock(), then take the PTE lock):

mm/pt_reclaim.c line 44

Quoted 9 lines

mm/pt_reclaim.c new file +8 -0

+       start_pte = pte_offset_map_rw_nolock(mm, pmd, addr, &pmdval, &ptl);
+       if (!start_pte)
+               return;
+
+       pml = pmd_lock(mm, pmd);
+       if (ptl != pml)
+               spin_lock_nested(ptl, SINGLE_DEPTH_NESTING);
+

Then I think this check can go away:

Quoted 27 lines

mm/pt_reclaim.c new file +24 -1

+       if (unlikely(!pmd_same(pmdval, pmdp_get_lockless(pmd))))
+               goto out_ptl;
+
+       /* Check if it is empty PTE page */
+       for (i = 0, pte = start_pte; i < PTRS_PER_PTE; i++, pte++) {
+               if (!pte_none(ptep_get(pte)))
+                       goto out_ptl;
+       }
+       pte_unmap(start_pte);
+
+       pmd_clear(pmd);
+
+       if (ptl != pml)
+               spin_unlock(ptl);
+       spin_unlock(pml);
+
+       free_pte(mm, addr, tlb, pmdval);
+
+       return;
+out_ptl:
+       pte_unmap_unlock(start_pte, ptl);
+       if (pml != ptl)
+               spin_unlock(pml);
+}
--

2.20.1

Quoted 12 lines · Jann Horn

Quoted 10 lines · Qi Zheng

As a first step, this commit aims to synchronously free the empty PTE pages in madvise(MADV_DONTNEED) case. We will detect and free empty PTE pages in zap_pte_range(), and will add zap_details.reclaim_pt to exclude cases other than madvise(MADV_DONTNEED).

Once an empty PTE is detected, we first try to hold the pmd lock within the pte lock. If successful, we clear the pmd entry directly (fast path). Otherwise, we wait until the pte lock is released, then re-hold the pmd and pte locks and loop PTRS_PER_PTE times to check pte_none() to re-detect whether the PTE page is empty and free it (slow path).

Quoted 5 lines

How does this interact with move_pages_pte()? I am looking at your series applied on top of next-20241106, and it looks to me like move_pages_pte() uses pte_offset_map_rw_nolock() and assumes that the PMD entry can't change. You can clearly see this assumption at the WARN_ON_ONCE(pmd_none(*dst_pmd)). And if we race the wrong way, I

In move_pages_pte(), the following conditions may indeed be triggered:

        /* Sanity checks before the operation */
        if (WARN_ON_ONCE(pmd_none(*dst_pmd)) || WARN_ON_ONCE(pmd_none(*src_pmd)) ||
            WARN_ON_ONCE(pmd_trans_huge(*dst_pmd)) ||
WARN_ON_ONCE(pmd_trans_huge(*src_pmd))) {
                err = -EINVAL;
                goto out;
        }

But maybe we can just remove the WARN_ON_ONCE(), because...

Quoted 2 lines

think for example move_present_pte() can end up moving a present PTE into a page table that has already been scheduled for RCU freeing.

...this situation is impossible to happen. Before performing move operation, the pte_same() check will be performed after holding the pte lock, which can ensure that the PTE page is stable:

CPU 0 CPU 1

zap_pte_range

                        orig_src_pte = ptep_get(src_pte);

pmd_lock
pte_lock
check if all PTEs are pte_none()
--> clear pmd entry
unlock pte
unlock pmd

                        src_pte_lock
                        pte_same(orig_src_pte, ptep_get(src_pte))
                        --> return false and will skip the move op

Quoted 12 lines

Quoted 12 lines

mm/memory.c +1 -1

index 002aa4f454fa0..c4a8c18fbcfd7 100644
@@ -1436,7 +1436,7 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
  static inline bool should_zap_cows(struct zap_details *details)
  {
         /* By default, zap all pages */
-       if (!details)
+       if (!details || details->reclaim_pt)
                 return true;
 

Quoted 5 lines

mm/memory.c copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma) · line 1442

Quoted 1 line

mm/memory.c +0 -0

         /* Or, we zap COWed pages only if the caller wants to */

This looks hacky - when we have a "details" object, its ->even_cows member is supposed to indicate whether COW pages should be zapped. So please instead set .even_cows=true in madvise_dontneed_single_vma().

But the details->reclaim_pt should continue to be set, right? Because we need to use .reclaim_pt to indicate whether the empty PTE page should be reclaimed.

Quoted 51 lines · Qi Zheng

Quoted 12 lines · Jann Horn

Quoted 10 lines · Qi Zheng

As a first step, this commit aims to synchronously free the empty PTE pages in madvise(MADV_DONTNEED) case. We will detect and free empty PTE pages in zap_pte_range(), and will add zap_details.reclaim_pt to exclude cases other than madvise(MADV_DONTNEED).

Once an empty PTE is detected, we first try to hold the pmd lock within the pte lock. If successful, we clear the pmd entry directly (fast path). Otherwise, we wait until the pte lock is released, then re-hold the pmd and pte locks and loop PTRS_PER_PTE times to check pte_none() to re-detect whether the PTE page is empty and free it (slow path).

Quoted 5 lines

How does this interact with move_pages_pte()? I am looking at your series applied on top of next-20241106, and it looks to me like move_pages_pte() uses pte_offset_map_rw_nolock() and assumes that the PMD entry can't change. You can clearly see this assumption at the WARN_ON_ONCE(pmd_none(*dst_pmd)). And if we race the wrong way, I

In move_pages_pte(), the following conditions may indeed be triggered:

         /* Sanity checks before the operation */
         if (WARN_ON_ONCE(pmd_none(*dst_pmd)) || WARN_ON_ONCE(pmd_none(*src_pmd)) ||
             WARN_ON_ONCE(pmd_trans_huge(*dst_pmd)) ||
WARN_ON_ONCE(pmd_trans_huge(*src_pmd))) {
                 err = -EINVAL;
                 goto out;
         }

But maybe we can just remove the WARN_ON_ONCE(), because...

Quoted 2 lines

think for example move_present_pte() can end up moving a present PTE into a page table that has already been scheduled for RCU freeing.

...this situation is impossible to happen. Before performing move operation, the pte_same() check will be performed after holding the pte lock, which can ensure that the PTE page is stable:

CPU 0 CPU 1

zap_pte_range

                         orig_src_pte = ptep_get(src_pte);

pmd_lock
pte_lock
check if all PTEs are pte_none()
--> clear pmd entry
unlock pte
unlock pmd

Quoted 3 lines

                         src_pte_lock
                         pte_same(orig_src_pte, ptep_get(src_pte))
                         --> return false and will skip the move op

Yes, that works for the source PTE. But what about the destination?

Operations on the destination PTE in move_pages_pte() are, when moving a normal present source PTE pointing to an order-0 page, and assuming that the optimistic folio_trylock(src_folio) and anon_vma_trylock_write(src_anon_vma) succeed:

dst_pte = pte_offset_map_rw_nolock(mm, dst_pmd, dst_addr, &dummy_pmdval, &dst_ptl)
[check that dst_pte is non-NULL]
some racy WARN_ON_ONCE() checks spin_lock(dst_ptl);
orig_dst_pte = ptep_get(dst_pte); spin_unlock(dst_ptl);
[bail if orig_dst_pte isn't none] double_pt_lock(dst_ptl, src_ptl)
[check pte_same(ptep_get(dst_pte), orig_dst_pte)]

and then we're past the point of no return. Note that there is a pte_same() check against orig_dst_pte, but pte_none(orig_dst_pte) is intentionally pte_none(), so the pte_same() check does not guarantee that the destination page table is still linked in.

Quoted 19 lines

Quoted 18 lines

Quoted 6 lines

mm/memory.c +0 -0

index 002aa4f454fa0..c4a8c18fbcfd7 100644

mm/memory.c copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma) · line 1442

Quoted 8 lines

mm/memory.c +1 -1

@@ -1436,7 +1436,7 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
   static inline bool should_zap_cows(struct zap_details *details)
   {
          /* By default, zap all pages */
-       if (!details)
+       if (!details || details->reclaim_pt)
                  return true;
 
          /* Or, we zap COWed pages only if the caller wants to */

This looks hacky - when we have a "details" object, its ->even_cows member is supposed to indicate whether COW pages should be zapped. So please instead set .even_cows=true in madvise_dontneed_single_vma().

Quoted 3 lines

But the details->reclaim_pt should continue to be set, right? Because we need to use .reclaim_pt to indicate whether the empty PTE page should be reclaimed.

Yeah, you should set both.

Now, if CONFIG_MMU_GATHER_RCU_TABLE_FREE is selected, the page table pages will be freed by semi RCU, that is:

- batch table freeing: asynchronous free by RCU
- single table freeing: IPI + synchronous free

In this way, the page table can be lockless traversed by disabling IRQ in paths such as fast GUP. But this is not enough to free the empty PTE page table pages in paths other that munmap and exit_mmap path, because IPI cannot be synchronized with rcu_read_lock() in pte_offset_map{_lock}().

In preparation for supporting empty PTE page table pages reclaimation, let single table also be freed by RCU like batch table freeing. Then we can also use pte_offset_map() etc to prevent PTE page from being freed.

Like pte_free_defer(), we can also safely use ptdesc->pt_rcu_head to free the page table pages:

- The pt_rcu_head is unioned with pt_list and pmd_huge_pte.

- For pt_list, it is used to manage the PGD page in x86. Fortunately tlb_remove_table() will not be used for free PGD pages, so it is safe to use pt_rcu_head.

- For pmd_huge_pte, we will do zap_deposited_table() before freeing the PMD page, so it is also safe.

Signed-off-by Qi Zheng <[email protected]>

---
 arch/x86/include/asm/tlb.h | 19 +++++++++++++++++++
 arch/x86/kernel/paravirt.c |  7 +++++++
 arch/x86/mm/pgtable.c      | 10 +++++++++-
 mm/mmu_gather.c            |  9 ++++++++-
 4 files changed, 43 insertions(+), 2 deletions(-)

arch/x86/include/asm/tlb.h +0 -0

index 580636cdc257b..e223b53a8b190 100644
@@ -34,4 +34,23 @@ static inline void __tlb_remove_table(void *table)
        free_page_and_swap_cache(table);

arch/x86/include/asm/tlb.h +10 -0

+static inline void __tlb_remove_table_one(void *table)
+{
+       struct page *page;
+
+       page = table;
+       call_rcu(&page->rcu_head, __tlb_remove_table_one_rcu);
+}
+#define __tlb_remove_table_one __tlb_remove_table_one
+#endif /* CONFIG_PT_RECLAIM */
+
 #endif /* _ASM_X86_TLB_H */

arch/x86/kernel/paravirt.c +7 -0

index fec3815335558..89688921ea62e 100644
@@ -59,10 +59,17 @@ void __init native_pv_lock_init(void)
                static_branch_enable(&virt_spin_lock_key);
 }
 
+#ifndef CONFIG_PT_RECLAIM
 static void native_tlb_remove_table(struct mmu_gather *tlb, void *table)
 {
        tlb_remove_page(tlb, table);
 }
+#else
+static void native_tlb_remove_table(struct mmu_gather *tlb, void *table)
+{
+       tlb_remove_table(tlb, table);
+}
+#endif
 
 struct static_key paravirt_steal_enabled;
 struct static_key paravirt_steal_rq_enabled;

arch/x86/mm/pgtable.c +9 -1

index 5745a354a241c..69a357b15974a 100644
@@ -19,12 +19,20 @@ EXPORT_SYMBOL(physical_mask);
 #endif
 
 #ifndef CONFIG_PARAVIRT
+#ifndef CONFIG_PT_RECLAIM
 static inline
 void paravirt_tlb_remove_table(struct mmu_gather *tlb, void *table)
 {
        tlb_remove_page(tlb, table);
 }
-#endif
+#else
+static inline
+void paravirt_tlb_remove_table(struct mmu_gather *tlb, void *table)
+{
+       tlb_remove_table(tlb, table);
+}
+#endif /* !CONFIG_PT_RECLAIM */
+#endif /* !CONFIG_PARAVIRT */
 
 gfp_t __userpte_alloc_gfp = GFP_PGTABLE_USER | PGTABLE_HIGHMEM;
 

mm/mmu_gather.c +8 -2

index 99b3e9408aa0f..d948479ca09e6 100644
@@ -311,10 +311,17 @@ static inline void tlb_table_invalidate(struct mmu_gather *tlb)
        }
 }
 
+#ifndef __tlb_remove_table_one
+static inline void __tlb_remove_table_one(void *table)
+{
+       __tlb_remove_table(table);
+}
+#endif
+
 static void tlb_remove_table_one(void *table)
 {
        tlb_remove_table_sync_one();
-       __tlb_remove_table(table);
+       __tlb_remove_table_one(table);
 }
 
 static void tlb_table_flush(struct mmu_gather *tlb)
--

2.20.1

+x86 MM maintainers - [email protected] was already cc'ed, but I don't know if that is enough for them to see it, and I haven't seen them comment on this series yet; I think you need an ack from them for this change.

Quoted 11 lines

Now, if CONFIG_MMU_GATHER_RCU_TABLE_FREE is selected, the page table pages will be freed by semi RCU, that is:

- batch table freeing: asynchronous free by RCU
- single table freeing: IPI + synchronous free

In this way, the page table can be lockless traversed by disabling IRQ in paths such as fast GUP. But this is not enough to free the empty PTE page table pages in paths other that munmap and exit_mmap path, because IPI cannot be synchronized with rcu_read_lock() in pte_offset_map{_lock}().

Quoted 3 lines

In preparation for supporting empty PTE page table pages reclaimation, let single table also be freed by RCU like batch table freeing. Then we can also use pte_offset_map() etc to prevent PTE page from being freed.

I applied your series locally and followed the page table freeing path that the reclaim feature would use on x86-64. Looks like it goes like this with the series applied:

free_pte
pte_free_tlb
__pte_free_tlb
___pte_free_tlb
paravirt_tlb_remove_table
tlb_remove_table [!CONFIG_PARAVIRT, Xen PV, Hyper-V, KVM]
[no-free-memory slowpath:]
tlb_table_invalidate
tlb_remove_table_one
tlb_remove_table_sync_one [does IPI for GUP-fast]

__tlb_remove_table_one [frees via RCU]
[fastpath:]
tlb_table_flush
tlb_remove_table_free [frees via RCU]
native_tlb_remove_table [CONFIG_PARAVIRT on native]
tlb_remove_table [see above]

Basically, the only remaining case in which paravirt_tlb_remove_table() does not use tlb_remove_table() with RCU delay is !CONFIG_PARAVIRT && !CONFIG_PT_RECLAIM. Given that CONFIG_PT_RECLAIM is defined as "default y" when supported, I guess that means X86's direct page table freeing path will almost never be used? If it stays that way and the X86 folks don't see a performance impact from using RCU to free tables on munmap() / process exit, I guess we might want to get rid of the direct page table freeing path on x86 at some point to simplify things...

(That simplification might also help prepare for Intel Remote Action Request, if that is a thing people want...)

Quoted 6 lines

Like pte_free_defer(), we can also safely use ptdesc->pt_rcu_head to free the page table pages:

- The pt_rcu_head is unioned with pt_list and pmd_huge_pte.

Quoted 6 lines

- For pt_list, it is used to manage the PGD page in x86. Fortunately tlb_remove_table() will not be used for free PGD pages, so it is safe to use pt_rcu_head.

- For pmd_huge_pte, we will do zap_deposited_table() before freeing the PMD page, so it is also safe.

Please also update the comments on "struct ptdesc" accordingly.

Quoted 17 lines

Signed-off-by Qi Zheng <[email protected]>

---
  arch/x86/include/asm/tlb.h | 19 +++++++++++++++++++
  arch/x86/kernel/paravirt.c |  7 +++++++
  arch/x86/mm/pgtable.c      | 10 +++++++++-
  mm/mmu_gather.c            |  9 ++++++++-
  4 files changed, 43 insertions(+), 2 deletions(-)

arch/x86/include/asm/tlb.h +0 -0

index 580636cdc257b..e223b53a8b190 100644
@@ -34,4 +34,23 @@ static inline void __tlb_remove_table(void *table)
         free_page_and_swap_cache(table);
  }
 

arch/x86/include/asm/tlb.h static inline void __tlb_remove_table(void *table) · line 44

Quoted 8 lines

arch/x86/include/asm/tlb.h +8 -0

+#ifdef CONFIG_PT_RECLAIM
+static inline void __tlb_remove_table_one_rcu(struct rcu_head *head)
+{
+       struct page *page;
+
+       page = container_of(head, struct page, rcu_head);
+       free_page_and_swap_cache(page);
+}

Why free_page_and_swap_cache()? Page tables shouldn't have swap cache, so I think something like put_page() would do the job.

Quoted 86 lines

arch/x86/include/asm/tlb.h +10 -0

+static inline void __tlb_remove_table_one(void *table)
+{
+       struct page *page;
+
+       page = table;
+       call_rcu(&page->rcu_head, __tlb_remove_table_one_rcu);
+}
+#define __tlb_remove_table_one __tlb_remove_table_one
+#endif /* CONFIG_PT_RECLAIM */
+
  #endif /* _ASM_X86_TLB_H */

arch/x86/kernel/paravirt.c +7 -0

index fec3815335558..89688921ea62e 100644
@@ -59,10 +59,17 @@ void __init native_pv_lock_init(void)
                 static_branch_enable(&virt_spin_lock_key);
  }
 
+#ifndef CONFIG_PT_RECLAIM
  static void native_tlb_remove_table(struct mmu_gather *tlb, void *table)
  {
         tlb_remove_page(tlb, table);
  }
+#else
+static void native_tlb_remove_table(struct mmu_gather *tlb, void *table)
+{
+       tlb_remove_table(tlb, table);
+}
+#endif
 
  struct static_key paravirt_steal_enabled;
  struct static_key paravirt_steal_rq_enabled;

arch/x86/mm/pgtable.c +9 -1

index 5745a354a241c..69a357b15974a 100644
@@ -19,12 +19,20 @@ EXPORT_SYMBOL(physical_mask);
  #endif
 
  #ifndef CONFIG_PARAVIRT
+#ifndef CONFIG_PT_RECLAIM
  static inline
  void paravirt_tlb_remove_table(struct mmu_gather *tlb, void *table)
  {
         tlb_remove_page(tlb, table);
  }
-#endif
+#else
+static inline
+void paravirt_tlb_remove_table(struct mmu_gather *tlb, void *table)
+{
+       tlb_remove_table(tlb, table);
+}
+#endif /* !CONFIG_PT_RECLAIM */
+#endif /* !CONFIG_PARAVIRT */
 
  gfp_t __userpte_alloc_gfp = GFP_PGTABLE_USER | PGTABLE_HIGHMEM;
 

mm/mmu_gather.c +8 -2

index 99b3e9408aa0f..d948479ca09e6 100644
@@ -311,10 +311,17 @@ static inline void tlb_table_invalidate(struct mmu_gather *tlb)
         }
  }
 
+#ifndef __tlb_remove_table_one
+static inline void __tlb_remove_table_one(void *table)
+{
+       __tlb_remove_table(table);
+}
+#endif
+
  static void tlb_remove_table_one(void *table)
  {
         tlb_remove_table_sync_one();
-       __tlb_remove_table(table);
+       __tlb_remove_table_one(table);
  }
 
  static void tlb_table_flush(struct mmu_gather *tlb)
--

2.20.1

Quoted 11 lines · Jann Horn

On Thu, Oct 31, 2024 at 9:14 AM Qi Zheng <[email protected]> wrote:
free_pte
pte_free_tlb
__pte_free_tlb
___pte_free_tlb
paravirt_tlb_remove_table
tlb_remove_table [!CONFIG_PARAVIRT, Xen PV, Hyper-V, KVM]
[no-free-memory slowpath:]
tlb_table_invalidate
tlb_remove_table_one
tlb_remove_table_sync_one [does IPI for GUP-fast]

^
It seems that this step can be ommitted when CONFIG_PT_RECLAIM is enabled, because GUP-fast will disable IRQ, which can also serve as the RCU critical section.

Quoted 11 lines

arch/x86/include/asm/tlb.h static inline void __tlb_remove_table(void *table) · line 44

Quoted 8 lines

arch/x86/include/asm/tlb.h +8 -0

+#ifdef CONFIG_PT_RECLAIM
+static inline void __tlb_remove_table_one_rcu(struct rcu_head *head)
+{
+       struct page *page;
+
+       page = container_of(head, struct page, rcu_head);
+       free_page_and_swap_cache(page);
+}

Why free_page_and_swap_cache()? Page tables shouldn't have swap cache, so I think something like put_page() would do the job.

Ah, I just did the same thing as __tlb_remove_table(). But I also have the same doubt as you, why does __tlb_remove_table() need to call free_page_and_swap_cache() instead of put_page().

Quoted 13 lines · Qi Zheng

Quoted 11 lines · Jann Horn

On Thu, Oct 31, 2024 at 9:14 AM Qi Zheng <[email protected]> wrote:
free_pte
pte_free_tlb
__pte_free_tlb
___pte_free_tlb
paravirt_tlb_remove_table
tlb_remove_table [!CONFIG_PARAVIRT, Xen PV, Hyper-V, KVM]
[no-free-memory slowpath:]
tlb_table_invalidate
tlb_remove_table_one
tlb_remove_table_sync_one [does IPI for GUP-fast]

Quoted 5 lines

^
It seems that this step can be ommitted when CONFIG_PT_RECLAIM is enabled, because GUP-fast will disable IRQ, which can also serve as the RCU critical section.

Yeah, I think so too.

Quoted 13 lines

Quoted 12 lines

arch/x86/include/asm/tlb.h static inline void __tlb_remove_table(void *table) · line 44

Quoted 9 lines

arch/x86/include/asm/tlb.h +8 -0

+#ifdef CONFIG_PT_RECLAIM
+static inline void __tlb_remove_table_one_rcu(struct rcu_head *head)
+{
+       struct page *page;
+
+       page = container_of(head, struct page, rcu_head);
+       free_page_and_swap_cache(page);
+}

Why free_page_and_swap_cache()? Page tables shouldn't have swap cache, so I think something like put_page() would do the job.

Quoted 3 lines

Ah, I just did the same thing as __tlb_remove_table(). But I also have the same doubt as you, why does __tlb_remove_table() need to call free_page_and_swap_cache() instead of put_page().

I think commit 9e52fc2b50de3a1c08b44f94c610fbe998c0031a probably just copy-pasted it from a more generic page freeing path...

Hi Jann,

Quoted 4 lines · Jann Horn

+x86 MM maintainers - [email protected] was already cc'ed, but I don't know if that is enough for them to see it, and I haven't seen them comment on this series yet; I think you need an ack from them for this change.

Yes, thanks to Jann for cc-ing x86 MM maintainers, and look forward to their feedback!

Quoted 13 lines · Qi Zheng

Quoted 11 lines

Now, if CONFIG_MMU_GATHER_RCU_TABLE_FREE is selected, the page table pages will be freed by semi RCU, that is:

  - batch table freeing: asynchronous free by RCU
  - single table freeing: IPI + synchronous free

In this way, the page table can be lockless traversed by disabling IRQ in paths such as fast GUP. But this is not enough to free the empty PTE page table pages in paths other that munmap and exit_mmap path, because IPI cannot be synchronized with rcu_read_lock() in pte_offset_map{_lock}().

Quoted 7 lines

Quoted 3 lines

In preparation for supporting empty PTE page table pages reclaimation, let single table also be freed by RCU like batch table freeing. Then we can also use pte_offset_map() etc to prevent PTE page from being freed.

I applied your series locally and followed the page table freeing path that the reclaim feature would use on x86-64. Looks like it goes like this with the series applied:

Yes.

Quoted 11 lines

free_pte
   pte_free_tlb
     __pte_free_tlb
       ___pte_free_tlb
         paravirt_tlb_remove_table
           tlb_remove_table [!CONFIG_PARAVIRT, Xen PV, Hyper-V, KVM] [no-free-memory slowpath:] tlb_table_invalidate
               tlb_remove_table_one tlb_remove_table_sync_one [does IPI for GUP-fast]

           ^
           It seems that this step can be ommitted when CONFIG_PT_RECLAIM is enabled, because GUP-fast will disable IRQ, which can also serve as the RCU critical section.

Quoted 7 lines

                 __tlb_remove_table_one [frees via RCU] [fastpath:]
               tlb_table_flush
                 tlb_remove_table_free [frees via RCU] native_tlb_remove_table [CONFIG_PARAVIRT on native] tlb_remove_table [see above]

Quoted 9 lines

Basically, the only remaining case in which paravirt_tlb_remove_table() does not use tlb_remove_table() with RCU delay is !CONFIG_PARAVIRT && !CONFIG_PT_RECLAIM. Given that CONFIG_PT_RECLAIM is defined as "default y" when supported, I guess that means X86's direct page table freeing path will almost never be used? If it stays that way and the X86 folks don't see a performance impact from using RCU to free tables on munmap() / process exit, I guess we might want to get rid of the direct page table freeing path on x86 at some point to simplify things...

In theory, using RCU to asynchronously free PTE pages should make munmap() / process exit path faster. I can try to grab some data.