[RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

Prior discussion about this can be found at [1].

rmap_walk() requires all folios, except non-KSM anon, to be locked. This
implies that when threads update folio->mapping to an anon_vma with
different root (currently only done by UFFDIO MOVE), they have to
serialize against rmap_walk() with write-lock on the anon_vma, hurting
scalability. Furthermore, this necessitates rechecking anon_vma when
pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).

This can be simplified quite a bit by ensuring that rmap_walk() is
always called on locked folios. Among the few callers of rmap_walk() on
unlocked anon folios, shrink_active_list()->folio_referenced() is the
only performance critical one.

shrink_active_list() doesn't act differently depending on what
folio_referenced() returns for an anon folio. So returning 1 when it
is contended, like in case of other folio types, wouldn't have any
negative impact.

Furthermore, as David pointed out in the previous discussion [2], this
could potentially only affect R/O pages after fork as PG_anon_exclusive
is not set. But, such folios are already isolated (prior to calling
folio_referenced()) by grabbing a reference and clearing LRU, so
do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
anyways.

[1] https://lore.kernel.org/all/CA+EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5+4aDx5jh2DvY6UHg@mail.gmail.com/
[2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com/

CC: David Hildenbrand <david@redhat.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
CC: Harry Yoo <harry.yoo@oracle.com>
CC: Peter Xu <peterx@redhat.com>
CC: Suren Baghdasaryan <surenb@google.com>
CC: Barry Song <baohua@kernel.org>
CC: SeongJae Park <sj@kernel.org>
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
 mm/damon/ops-common.c | 16 ++++------------
 mm/page_idle.c        |  8 ++------
 mm/rmap.c             | 40 ++++++++++------------------------------
 3 files changed, 16 insertions(+), 48 deletions(-)

diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
index 998c5180a603..f61d6dde13dc 100644
--- a/mm/damon/ops-common.c
+++ b/mm/damon/ops-common.c
@@ -162,21 +162,17 @@ void damon_folio_mkold(struct folio *folio)
 		.rmap_one = damon_folio_mkold_one,
 		.anon_lock = folio_lock_anon_vma_read,
 	};
-	bool need_lock;
 
 	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
 		folio_set_idle(folio);
 		return;
 	}
 
-	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
-	if (need_lock && !folio_trylock(folio))
+	if (!folio_trylock(folio))
 		return;
 
 	rmap_walk(folio, &rwc);
-
-	if (need_lock)
-		folio_unlock(folio);
+	folio_unlock(folio);
 
 }
 
@@ -228,7 +224,6 @@ bool damon_folio_young(struct folio *folio)
 		.rmap_one = damon_folio_young_one,
 		.anon_lock = folio_lock_anon_vma_read,
 	};
-	bool need_lock;
 
 	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
 		if (folio_test_idle(folio))
@@ -237,14 +232,11 @@ bool damon_folio_young(struct folio *folio)
 			return true;
 	}
 
-	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
-	if (need_lock && !folio_trylock(folio))
+	if (!folio_trylock(folio))
 		return false;
 
 	rmap_walk(folio, &rwc);
-
-	if (need_lock)
-		folio_unlock(folio);
+	folio_unlock(folio);
 
 	return accessed;
 }
diff --git a/mm/page_idle.c b/mm/page_idle.c
index a82b340dc204..9bf573d22e87 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -101,19 +101,15 @@ static void page_idle_clear_pte_refs(struct folio *folio)
 		.rmap_one = page_idle_clear_pte_refs_one,
 		.anon_lock = folio_lock_anon_vma_read,
 	};
-	bool need_lock;
 
 	if (!folio_mapped(folio) || !folio_raw_mapping(folio))
 		return;
 
-	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
-	if (need_lock && !folio_trylock(folio))
+	if (!folio_trylock(folio))
 		return;
 
 	rmap_walk(folio, &rwc);
-
-	if (need_lock)
-		folio_unlock(folio);
+	folio_unlock(folio);
 }
 
 static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
diff --git a/mm/rmap.c b/mm/rmap.c
index 34333ae3bd80..fc53f31434f4 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -489,17 +489,15 @@ void __init anon_vma_init(void)
  * if there is a mapcount, we can dereference the anon_vma after observing
  * those.
  *
- * NOTE: the caller should normally hold folio lock when calling this.  If
- * not, the caller needs to double check the anon_vma didn't change after
- * taking the anon_vma lock for either read or write (UFFDIO_MOVE can modify it
- * concurrently without folio lock protection). See folio_lock_anon_vma_read()
- * which has already covered that, and comment above remap_pages().
+ * NOTE: the caller should hold folio lock when calling this.
  */
 struct anon_vma *folio_get_anon_vma(const struct folio *folio)
 {
 	struct anon_vma *anon_vma = NULL;
 	unsigned long anon_mapping;
 
+	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
+
 	rcu_read_lock();
 	anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
 	if ((anon_mapping & FOLIO_MAPPING_FLAGS) != FOLIO_MAPPING_ANON)
@@ -546,7 +544,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
 	struct anon_vma *root_anon_vma;
 	unsigned long anon_mapping;
 
-retry:
 	rcu_read_lock();
 	anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
 	if ((anon_mapping & FOLIO_MAPPING_FLAGS) != FOLIO_MAPPING_ANON)
@@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
 	anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
 	root_anon_vma = READ_ONCE(anon_vma->root);
 	if (down_read_trylock(&root_anon_vma->rwsem)) {
-		/*
-		 * folio_move_anon_rmap() might have changed the anon_vma as we
-		 * might not hold the folio lock here.
-		 */
-		if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
-			     anon_mapping)) {
-			up_read(&root_anon_vma->rwsem);
-			rcu_read_unlock();
-			goto retry;
-		}
-
 		/*
 		 * If the folio is still mapped, then this anon_vma is still
 		 * its anon_vma, and holding the mutex ensures that it will
@@ -602,18 +588,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
 	rcu_read_unlock();
 	anon_vma_lock_read(anon_vma);
 
-	/*
-	 * folio_move_anon_rmap() might have changed the anon_vma as we might
-	 * not hold the folio lock here.
-	 */
-	if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
-		     anon_mapping)) {
-		anon_vma_unlock_read(anon_vma);
-		put_anon_vma(anon_vma);
-		anon_vma = NULL;
-		goto retry;
-	}
-
 	if (atomic_dec_and_test(&anon_vma->refcount)) {
 		/*
 		 * Oops, we held the last refcount, release the lock
@@ -1005,7 +979,7 @@ int folio_referenced(struct folio *folio, int is_locked,
 	if (!folio_raw_mapping(folio))
 		return 0;
 
-	if (!is_locked && (!folio_test_anon(folio) || folio_test_ksm(folio))) {
+	if (!is_locked) {
 		we_locked = folio_trylock(folio);
 		if (!we_locked)
 			return 1;
@@ -2815,6 +2789,12 @@ static void rmap_walk_anon(struct folio *folio,
 	pgoff_t pgoff_start, pgoff_end;
 	struct anon_vma_chain *avc;
 
+	/*
+	 * The folio lock ensures that folio->mapping can be changed under us to
+	 * an anon_vma with different root, like UFFDIO MOVE.
+	 */
+	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
+
 	if (locked) {
 		anon_vma = folio_anon_vma(folio);
 		/* anon_vma disappear under us? */

base-commit: b024763926d2726978dff6588b81877d000159c1
-- 
2.51.0.355.g5224444f11-goog

[RFC PATCH 2/2] userfaultfd: remove anon-vma lock for moving folios in MOVE ioctl

[Lokesh Gidra]

Since rmap_walk() is now always called on locked anon folios, we don't
have to serialize on anon_vma lock when updating folio->mapping.

This helps avoid contention on src anon_vma when multiple threads are
simultaneously moving distinct pages from the same src vma.

CC: David Hildenbrand <david@redhat.com>
CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
CC: Peter Xu <peterx@redhat.com>
CC: Suren Baghdasaryan <surenb@google.com>
CC: Barry Song <baohua@kernel.org>
Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
---
 mm/huge_memory.c | 22 +----------------
 mm/userfaultfd.c | 62 +++++++++---------------------------------------
 2 files changed, 12 insertions(+), 72 deletions(-)

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 26cedfcd7418..5cd3957f92d4 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -2533,7 +2533,6 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 	pmd_t _dst_pmd, src_pmdval;
 	struct page *src_page;
 	struct folio *src_folio;
-	struct anon_vma *src_anon_vma;
 	spinlock_t *src_ptl, *dst_ptl;
 	pgtable_t src_pgtable;
 	struct mmu_notifier_range range;
@@ -2582,23 +2581,9 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 				src_addr + HPAGE_PMD_SIZE);
 	mmu_notifier_invalidate_range_start(&range);
 
-	if (src_folio) {
+	if (src_folio)
 		folio_lock(src_folio);
 
-		/*
-		 * split_huge_page walks the anon_vma chain without the page
-		 * lock. Serialize against it with the anon_vma lock, the page
-		 * lock is not enough.
-		 */
-		src_anon_vma = folio_get_anon_vma(src_folio);
-		if (!src_anon_vma) {
-			err = -EAGAIN;
-			goto unlock_folio;
-		}
-		anon_vma_lock_write(src_anon_vma);
-	} else
-		src_anon_vma = NULL;
-
 	dst_ptl = pmd_lockptr(mm, dst_pmd);
 	double_pt_lock(src_ptl, dst_ptl);
 	if (unlikely(!pmd_same(*src_pmd, src_pmdval) ||
@@ -2643,11 +2628,6 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
 	pgtable_trans_huge_deposit(mm, dst_pmd, src_pgtable);
 unlock_ptls:
 	double_pt_unlock(src_ptl, dst_ptl);
-	if (src_anon_vma) {
-		anon_vma_unlock_write(src_anon_vma);
-		put_anon_vma(src_anon_vma);
-	}
-unlock_folio:
 	/* unblock rmap walks */
 	if (src_folio)
 		folio_unlock(src_folio);
diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
index 50aaa8dcd24c..1a36760a36c7 100644
--- a/mm/userfaultfd.c
+++ b/mm/userfaultfd.c
@@ -1035,8 +1035,7 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
  */
 static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
 						 unsigned long src_addr,
-						 pte_t *src_pte, pte_t *dst_pte,
-						 struct anon_vma *src_anon_vma)
+						 pte_t *src_pte, pte_t *dst_pte)
 {
 	pte_t orig_dst_pte, orig_src_pte;
 	struct folio *folio;
@@ -1052,8 +1051,7 @@ static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
 	folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
 	if (!folio || !folio_trylock(folio))
 		return NULL;
-	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
-	    folio_anon_vma(folio) != src_anon_vma) {
+	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio)) {
 		folio_unlock(folio);
 		return NULL;
 	}
@@ -1061,9 +1059,8 @@ static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
 }
 
 /*
- * Moves src folios to dst in a batch as long as they share the same
- * anon_vma as the first folio, are not large, and can successfully
- * take the lock via folio_trylock().
+ * Moves src folios to dst in a batch as long as they are not large, and can
+ * successfully take the lock via folio_trylock().
  */
 static long move_present_ptes(struct mm_struct *mm,
 			      struct vm_area_struct *dst_vma,
@@ -1073,8 +1070,7 @@ static long move_present_ptes(struct mm_struct *mm,
 			      pte_t orig_dst_pte, pte_t orig_src_pte,
 			      pmd_t *dst_pmd, pmd_t dst_pmdval,
 			      spinlock_t *dst_ptl, spinlock_t *src_ptl,
-			      struct folio **first_src_folio, unsigned long len,
-			      struct anon_vma *src_anon_vma)
+			      struct folio **first_src_folio, unsigned long len)
 {
 	int err = 0;
 	struct folio *src_folio = *first_src_folio;
@@ -1132,8 +1128,8 @@ static long move_present_ptes(struct mm_struct *mm,
 		src_pte++;
 
 		folio_unlock(src_folio);
-		src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
-							dst_pte, src_anon_vma);
+		src_folio = check_ptes_for_batched_move(src_vma, src_addr,
+							src_pte, dst_pte);
 		if (!src_folio)
 			break;
 	}
@@ -1263,7 +1259,6 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
 	pmd_t dummy_pmdval;
 	pmd_t dst_pmdval;
 	struct folio *src_folio = NULL;
-	struct anon_vma *src_anon_vma = NULL;
 	struct mmu_notifier_range range;
 	long ret = 0;
 
@@ -1347,9 +1342,9 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
 		}
 
 		/*
-		 * Pin and lock both source folio and anon_vma. Since we are in
-		 * RCU read section, we can't block, so on contention have to
-		 * unmap the ptes, obtain the lock and retry.
+		 * Pin and lock source folio. Since we are in RCU read section,
+		 * we can't block, so on contention have to unmap the ptes,
+		 * obtain the lock and retry.
 		 */
 		if (!src_folio) {
 			struct folio *folio;
@@ -1423,33 +1418,11 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
 			goto retry;
 		}
 
-		if (!src_anon_vma) {
-			/*
-			 * folio_referenced walks the anon_vma chain
-			 * without the folio lock. Serialize against it with
-			 * the anon_vma lock, the folio lock is not enough.
-			 */
-			src_anon_vma = folio_get_anon_vma(src_folio);
-			if (!src_anon_vma) {
-				/* page was unmapped from under us */
-				ret = -EAGAIN;
-				goto out;
-			}
-			if (!anon_vma_trylock_write(src_anon_vma)) {
-				pte_unmap(src_pte);
-				pte_unmap(dst_pte);
-				src_pte = dst_pte = NULL;
-				/* now we can block and wait */
-				anon_vma_lock_write(src_anon_vma);
-				goto retry;
-			}
-		}
-
 		ret = move_present_ptes(mm, dst_vma, src_vma,
 					dst_addr, src_addr, dst_pte, src_pte,
 					orig_dst_pte, orig_src_pte, dst_pmd,
 					dst_pmdval, dst_ptl, src_ptl, &src_folio,
-					len, src_anon_vma);
+					len);
 	} else {
 		struct folio *folio = NULL;
 
@@ -1516,10 +1489,6 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
 	}
 
 out:
-	if (src_anon_vma) {
-		anon_vma_unlock_write(src_anon_vma);
-		put_anon_vma(src_anon_vma);
-	}
 	if (src_folio) {
 		folio_unlock(src_folio);
 		folio_put(src_folio);
@@ -1793,15 +1762,6 @@ static void uffd_move_unlock(struct vm_area_struct *dst_vma,
  * virtual regions without knowing if there are transparent hugepage
  * in the regions or not, but preventing the risk of having to split
  * the hugepmd during the remap.
- *
- * If there's any rmap walk that is taking the anon_vma locks without
- * first obtaining the folio lock (the only current instance is
- * folio_referenced), they will have to verify if the folio->mapping
- * has changed after taking the anon_vma lock. If it changed they
- * should release the lock and retry obtaining a new anon_vma, because
- * it means the anon_vma was changed by move_pages() before the lock
- * could be obtained. This is the only additional complexity added to
- * the rmap code to provide this anonymous page remapping functionality.
  */
 ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
 		   unsigned long src_start, unsigned long len, __u64 mode)
-- 
2.51.0.355.g5224444f11-goog

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Barry Song]

On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> Prior discussion about this can be found at [1].
>
> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> implies that when threads update folio->mapping to an anon_vma with
> different root (currently only done by UFFDIO MOVE), they have to
> serialize against rmap_walk() with write-lock on the anon_vma, hurting
> scalability. Furthermore, this necessitates rechecking anon_vma when
> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
>
> This can be simplified quite a bit by ensuring that rmap_walk() is
> always called on locked folios. Among the few callers of rmap_walk() on
> unlocked anon folios, shrink_active_list()->folio_referenced() is the
> only performance critical one.

As I understand it, shrink_inactive_list() also invokes folio_referenced().
Shouldn’t it be called just as often as shrink_active_list()?

>
> shrink_active_list() doesn't act differently depending on what
> folio_referenced() returns for an anon folio. So returning 1 when it
> is contended, like in case of other folio types, wouldn't have any
> negative impact.

A complaint was raised that the LRU could become slightly disordered:
https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/

We can re-test to confirm if this is the case.

Thanks
Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > Prior discussion about this can be found at [1].
> >
> > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > implies that when threads update folio->mapping to an anon_vma with
> > different root (currently only done by UFFDIO MOVE), they have to
> > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > scalability. Furthermore, this necessitates rechecking anon_vma when
> > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> >
> > This can be simplified quite a bit by ensuring that rmap_walk() is
> > always called on locked folios. Among the few callers of rmap_walk() on
> > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > only performance critical one.
>
> As I understand it, shrink_inactive_list() also invokes folio_referenced().
> Shouldn’t it be called just as often as shrink_active_list()?

I'm only talking about those callers which call rmap_walk() without
locking anon folio. The
shrink_inactive_list()->folio_check_references()->folio_referenced()
path that you are talking about always locks the folio. So the
behavior in that case wouldn't change.
>
> >
> > shrink_active_list() doesn't act differently depending on what
> > folio_referenced() returns for an anon folio. So returning 1 when it
> > is contended, like in case of other folio types, wouldn't have any
> > negative impact.
>
> A complaint was raised that the LRU could become slightly disordered:
> https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
>
> We can re-test to confirm if this is the case.
The patch in the link you provided is suggesting to control try-lock
for anon_vma lock. But here we are dealing with folio lock. Not sure
if the ordering issue will be there in this case.
>
> Thanks
> Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Barry Song]

On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > >
> > > Prior discussion about this can be found at [1].
> > >
> > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > implies that when threads update folio->mapping to an anon_vma with
> > > different root (currently only done by UFFDIO MOVE), they have to
> > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > >
> > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > always called on locked folios. Among the few callers of rmap_walk() on
> > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > only performance critical one.
> >
> > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > Shouldn’t it be called just as often as shrink_active_list()?
>
> I'm only talking about those callers which call rmap_walk() without
> locking anon folio. The
> shrink_inactive_list()->folio_check_references()->folio_referenced()
> path that you are talking about always locks the folio. So the
> behavior in that case wouldn't change.

Thanks for the clarification. Could you add a note about this if there
is a v2?

> >
> > >
> > > shrink_active_list() doesn't act differently depending on what
> > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > is contended, like in case of other folio types, wouldn't have any
> > > negative impact.
> >
> > A complaint was raised that the LRU could become slightly disordered:
> > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> >
> > We can re-test to confirm if this is the case.
> The patch in the link you provided is suggesting to control try-lock
> for anon_vma lock. But here we are dealing with folio lock. Not sure
> if the ordering issue will be there in this case.

Right. Not sure what percentage of folios will be contended; I assume
it is minor. Maybe you could share some data on this in a v2?

Thanks
Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Mon, Sep 8, 2025 at 5:40 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > >
> > > > Prior discussion about this can be found at [1].
> > > >
> > > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > > implies that when threads update folio->mapping to an anon_vma with
> > > > different root (currently only done by UFFDIO MOVE), they have to
> > > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > > >
> > > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > > always called on locked folios. Among the few callers of rmap_walk() on
> > > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > > only performance critical one.
> > >
> > > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > > Shouldn’t it be called just as often as shrink_active_list()?
> >
> > I'm only talking about those callers which call rmap_walk() without
> > locking anon folio. The
> > shrink_inactive_list()->folio_check_references()->folio_referenced()
> > path that you are talking about always locks the folio. So the
> > behavior in that case wouldn't change.
>
> Thanks for the clarification. Could you add a note about this if there
> is a v2?
>
Certainly, will do.
> > >
> > > >
> > > > shrink_active_list() doesn't act differently depending on what
> > > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > > is contended, like in case of other folio types, wouldn't have any
> > > > negative impact.
> > >
> > > A complaint was raised that the LRU could become slightly disordered:
> > > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> > >
> > > We can re-test to confirm if this is the case.
> > The patch in the link you provided is suggesting to control try-lock
> > for anon_vma lock. But here we are dealing with folio lock. Not sure
> > if the ordering issue will be there in this case.
>
> Right. Not sure what percentage of folios will be contended; I assume
> it is minor. Maybe you could share some data on this in a v2?

Any suggestion on how (or which test/benchmark) would be good to
gather data on this?

IIUC, shink_active_list() doesn't behave any differently whether there
is contention or not, except when it's an executable file folio. So I
doubt such data would be any useful. Please correct me if I'm wrong.
>
> Thanks
> Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Barry Song]

On Tue, Sep 9, 2025 at 1:37 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Mon, Sep 8, 2025 at 5:40 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > >
> > > On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> > > >
> > > > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > >
> > > > > Prior discussion about this can be found at [1].
> > > > >
> > > > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > > > implies that when threads update folio->mapping to an anon_vma with
> > > > > different root (currently only done by UFFDIO MOVE), they have to
> > > > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > > > >
> > > > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > > > always called on locked folios. Among the few callers of rmap_walk() on
> > > > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > > > only performance critical one.
> > > >
> > > > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > > > Shouldn’t it be called just as often as shrink_active_list()?
> > >
> > > I'm only talking about those callers which call rmap_walk() without
> > > locking anon folio. The
> > > shrink_inactive_list()->folio_check_references()->folio_referenced()
> > > path that you are talking about always locks the folio. So the
> > > behavior in that case wouldn't change.
> >
> > Thanks for the clarification. Could you add a note about this if there
> > is a v2?
> >
> Certainly, will do.
> > > >
> > > > >
> > > > > shrink_active_list() doesn't act differently depending on what
> > > > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > > > is contended, like in case of other folio types, wouldn't have any
> > > > > negative impact.
> > > >
> > > > A complaint was raised that the LRU could become slightly disordered:
> > > > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> > > >
> > > > We can re-test to confirm if this is the case.
> > > The patch in the link you provided is suggesting to control try-lock
> > > for anon_vma lock. But here we are dealing with folio lock. Not sure
> > > if the ordering issue will be there in this case.
> >
> > Right. Not sure what percentage of folios will be contended; I assume
> > it is minor. Maybe you could share some data on this in a v2?
>
> Any suggestion on how (or which test/benchmark) would be good to
> gather data on this?
>
> IIUC, shink_active_list() doesn't behave any differently whether there
> is contention or not, except when it's an executable file folio. So I
> doubt such data would be any useful. Please correct me if I'm wrong.

Since we skipped clearing the PTE young bit in folio_referenced_one, a
cold page might be misidentified as hot during shrink_inactive_list().
My understanding is that as long as the percentage is small, this
shouldn't be a concern.

Thanks
Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Mon, Sep 8, 2025 at 10:52 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 1:37 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > On Mon, Sep 8, 2025 at 5:40 PM Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > >
> > > > On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > >
> > > > > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > >
> > > > > > Prior discussion about this can be found at [1].
> > > > > >
> > > > > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > > > > implies that when threads update folio->mapping to an anon_vma with
> > > > > > different root (currently only done by UFFDIO MOVE), they have to
> > > > > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > > > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > > > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > > > > >
> > > > > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > > > > always called on locked folios. Among the few callers of rmap_walk() on
> > > > > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > > > > only performance critical one.
> > > > >
> > > > > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > > > > Shouldn’t it be called just as often as shrink_active_list()?
> > > >
> > > > I'm only talking about those callers which call rmap_walk() without
> > > > locking anon folio. The
> > > > shrink_inactive_list()->folio_check_references()->folio_referenced()
> > > > path that you are talking about always locks the folio. So the
> > > > behavior in that case wouldn't change.
> > >
> > > Thanks for the clarification. Could you add a note about this if there
> > > is a v2?
> > >
> > Certainly, will do.
> > > > >
> > > > > >
> > > > > > shrink_active_list() doesn't act differently depending on what
> > > > > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > > > > is contended, like in case of other folio types, wouldn't have any
> > > > > > negative impact.
> > > > >
> > > > > A complaint was raised that the LRU could become slightly disordered:
> > > > > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> > > > >
> > > > > We can re-test to confirm if this is the case.
> > > > The patch in the link you provided is suggesting to control try-lock
> > > > for anon_vma lock. But here we are dealing with folio lock. Not sure
> > > > if the ordering issue will be there in this case.
> > >
> > > Right. Not sure what percentage of folios will be contended; I assume
> > > it is minor. Maybe you could share some data on this in a v2?
> >
> > Any suggestion on how (or which test/benchmark) would be good to
> > gather data on this?
> >
> > IIUC, shink_active_list() doesn't behave any differently whether there
> > is contention or not, except when it's an executable file folio. So I
> > doubt such data would be any useful. Please correct me if I'm wrong.
>
> Since we skipped clearing the PTE young bit in folio_referenced_one, a
> cold page might be misidentified as hot during shrink_inactive_list().
> My understanding is that as long as the percentage is small, this
> shouldn't be a concern.
I see. That makes a lot more sense why folio_referenced() is called on
all folios in shrink_active_list(). I missed that young bit clearing
before.

Any suggestions on a good testcase to gather this data?
>
> Thanks
> Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Barry Song]

On Tue, Sep 9, 2025 at 1:57 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
>
> On Mon, Sep 8, 2025 at 10:52 PM Barry Song <21cnbao@gmail.com> wrote:
> >
> > On Tue, Sep 9, 2025 at 1:37 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > >
> > > On Mon, Sep 8, 2025 at 5:40 PM Barry Song <21cnbao@gmail.com> wrote:
> > > >
> > > > On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > >
> > > > > On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > > >
> > > > > > > Prior discussion about this can be found at [1].
> > > > > > >
> > > > > > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > > > > > implies that when threads update folio->mapping to an anon_vma with
> > > > > > > different root (currently only done by UFFDIO MOVE), they have to
> > > > > > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > > > > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > > > > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > > > > > >
> > > > > > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > > > > > always called on locked folios. Among the few callers of rmap_walk() on
> > > > > > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > > > > > only performance critical one.
> > > > > >
> > > > > > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > > > > > Shouldn’t it be called just as often as shrink_active_list()?
> > > > >
> > > > > I'm only talking about those callers which call rmap_walk() without
> > > > > locking anon folio. The
> > > > > shrink_inactive_list()->folio_check_references()->folio_referenced()
> > > > > path that you are talking about always locks the folio. So the
> > > > > behavior in that case wouldn't change.
> > > >
> > > > Thanks for the clarification. Could you add a note about this if there
> > > > is a v2?
> > > >
> > > Certainly, will do.
> > > > > >
> > > > > > >
> > > > > > > shrink_active_list() doesn't act differently depending on what
> > > > > > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > > > > > is contended, like in case of other folio types, wouldn't have any
> > > > > > > negative impact.
> > > > > >
> > > > > > A complaint was raised that the LRU could become slightly disordered:
> > > > > > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> > > > > >
> > > > > > We can re-test to confirm if this is the case.
> > > > > The patch in the link you provided is suggesting to control try-lock
> > > > > for anon_vma lock. But here we are dealing with folio lock. Not sure
> > > > > if the ordering issue will be there in this case.
> > > >
> > > > Right. Not sure what percentage of folios will be contended; I assume
> > > > it is minor. Maybe you could share some data on this in a v2?
> > >
> > > Any suggestion on how (or which test/benchmark) would be good to
> > > gather data on this?
> > >
> > > IIUC, shink_active_list() doesn't behave any differently whether there
> > > is contention or not, except when it's an executable file folio. So I
> > > doubt such data would be any useful. Please correct me if I'm wrong.
> >
> > Since we skipped clearing the PTE young bit in folio_referenced_one, a
> > cold page might be misidentified as hot during shrink_inactive_list().
> > My understanding is that as long as the percentage is small, this
> > shouldn't be a concern.
> I see. That makes a lot more sense why folio_referenced() is called on
> all folios in shrink_active_list(). I missed that young bit clearing
> before.
>
> Any suggestions on a good testcase to gather this data?

I would run monkey for a few hours with some debug counters, e.g. how
many folios pass through shrink_active_list(), how many get contended
and moved to the inactive list without clearing the young bit. If the
percentage is small, we can just ignore this disordered LRU behavior.

Thanks
Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Harry Yoo]

On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> Prior discussion about this can be found at [1].
> 
> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> implies that when threads update folio->mapping to an anon_vma with
> different root (currently only done by UFFDIO MOVE), they have to
> serialize against rmap_walk() with write-lock on the anon_vma, hurting
> scalability. Furthermore, this necessitates rechecking anon_vma when
> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> 
> This can be simplified quite a bit by ensuring that rmap_walk() is
> always called on locked folios. Among the few callers of rmap_walk() on
> unlocked anon folios, shrink_active_list()->folio_referenced() is the
> only performance critical one.
> 
> shrink_active_list() doesn't act differently depending on what
> folio_referenced() returns for an anon folio. So returning 1 when it
> is contended, like in case of other folio types, wouldn't have any
> negative impact.
> 
> Furthermore, as David pointed out in the previous discussion [2], this
> could potentially only affect R/O pages after fork as PG_anon_exclusive
> is not set. But, such folios are already isolated (prior to calling
> folio_referenced()) by grabbing a reference and clearing LRU, so
> do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
> anyways.
> 
> [1] https://lore.kernel.org/all/CA*EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5*4aDx5jh2DvY6UHg@mail.gmail.com
> [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com
> 
> CC: David Hildenbrand <david@redhat.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Harry Yoo <harry.yoo@oracle.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Suren Baghdasaryan <surenb@google.com>
> CC: Barry Song <baohua@kernel.org>
> CC: SeongJae Park <sj@kernel.org>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
>  mm/damon/ops-common.c | 16 ++++------------
>  mm/page_idle.c        |  8 ++------
>  mm/rmap.c             | 40 ++++++++++------------------------------
>  3 files changed, 16 insertions(+), 48 deletions(-)
> 
> @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
>  	root_anon_vma = READ_ONCE(anon_vma->root);
>  	if (down_read_trylock(&root_anon_vma->rwsem)) {
> -		/*
> -		 * folio_move_anon_rmap() might have changed the anon_vma as we
> -		 * might not hold the folio lock here.
> -		 */
> -		if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> -			     anon_mapping)) {
> -			up_read(&root_anon_vma->rwsem);
> -			rcu_read_unlock();
> -			goto retry;
> -		}
> -

folio_lock_anon_vma_read() can be called without folio lock in a path:
memory_failure() -> kill_procs_now() -> collect_procs() ->
collect_procs_anon().

Not sure why collect_procs_{anon,ksm,file,fsdax} do not use rmap_walk()
functionality :/

Should we take folio lock before calling kill_procs_now() in
memory_failure()?

-- 
Cheers,
Harry / Hyeonggon

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Wed, Sep 10, 2025 at 3:10 AM Harry Yoo <harry.yoo@oracle.com> wrote:
>
> On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> > Prior discussion about this can be found at [1].
> >
> > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > implies that when threads update folio->mapping to an anon_vma with
> > different root (currently only done by UFFDIO MOVE), they have to
> > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > scalability. Furthermore, this necessitates rechecking anon_vma when
> > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> >
> > This can be simplified quite a bit by ensuring that rmap_walk() is
> > always called on locked folios. Among the few callers of rmap_walk() on
> > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > only performance critical one.
> >
> > shrink_active_list() doesn't act differently depending on what
> > folio_referenced() returns for an anon folio. So returning 1 when it
> > is contended, like in case of other folio types, wouldn't have any
> > negative impact.
> >
> > Furthermore, as David pointed out in the previous discussion [2], this
> > could potentially only affect R/O pages after fork as PG_anon_exclusive
> > is not set. But, such folios are already isolated (prior to calling
> > folio_referenced()) by grabbing a reference and clearing LRU, so
> > do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
> > anyways.
> >
> > [1] https://lore.kernel.org/all/CA*EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5*4aDx5jh2DvY6UHg@mail.gmail.com
> > [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com
> >
> > CC: David Hildenbrand <david@redhat.com>
> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > CC: Harry Yoo <harry.yoo@oracle.com>
> > CC: Peter Xu <peterx@redhat.com>
> > CC: Suren Baghdasaryan <surenb@google.com>
> > CC: Barry Song <baohua@kernel.org>
> > CC: SeongJae Park <sj@kernel.org>
> > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > ---
> >  mm/damon/ops-common.c | 16 ++++------------
> >  mm/page_idle.c        |  8 ++------
> >  mm/rmap.c             | 40 ++++++++++------------------------------
> >  3 files changed, 16 insertions(+), 48 deletions(-)
> >
> > @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
> >       anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
> >       root_anon_vma = READ_ONCE(anon_vma->root);
> >       if (down_read_trylock(&root_anon_vma->rwsem)) {
> > -             /*
> > -              * folio_move_anon_rmap() might have changed the anon_vma as we
> > -              * might not hold the folio lock here.
> > -              */
> > -             if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> > -                          anon_mapping)) {
> > -                     up_read(&root_anon_vma->rwsem);
> > -                     rcu_read_unlock();
> > -                     goto retry;
> > -             }
> > -
>
> folio_lock_anon_vma_read() can be called without folio lock in a path:
> memory_failure() -> kill_procs_now() -> collect_procs() ->
> collect_procs_anon().
>
Thanks for catching this. Fell off the cracks for me.

> Not sure why collect_procs_{anon,ksm,file,fsdax} do not use rmap_walk()
> functionality :/
>
> Should we take folio lock before calling kill_procs_now() in
> memory_failure()?

To me it seems minimal (and sufficient) to put the collect_procs()
call in kill_procs_now() inside folio lock's critical section.
>
> --
> Cheers,
> Harry / Hyeonggon

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Harry Yoo]

On Wed, Sep 10, 2025 at 08:33:54AM -0700, Lokesh Gidra wrote:
> On Wed, Sep 10, 2025 at 3:10 AM Harry Yoo <harry.yoo@oracle.com> wrote:
> >
> > On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> > > Prior discussion about this can be found at [1].
> > >
> > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > implies that when threads update folio->mapping to an anon_vma with
> > > different root (currently only done by UFFDIO MOVE), they have to
> > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > >
> > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > always called on locked folios. Among the few callers of rmap_walk() on
> > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > only performance critical one.
> > >
> > > shrink_active_list() doesn't act differently depending on what
> > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > is contended, like in case of other folio types, wouldn't have any
> > > negative impact.
> > >
> > > Furthermore, as David pointed out in the previous discussion [2], this
> > > could potentially only affect R/O pages after fork as PG_anon_exclusive
> > > is not set. But, such folios are already isolated (prior to calling
> > > folio_referenced()) by grabbing a reference and clearing LRU, so
> > > do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
> > > anyways.
> > >
> > > [1] https://lore.kernel.org/all/CA*EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5*4aDx5jh2DvY6UHg@mail.gmail.com
> > > [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com
> > >
> > > CC: David Hildenbrand <david@redhat.com>
> > > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > CC: Harry Yoo <harry.yoo@oracle.com>
> > > CC: Peter Xu <peterx@redhat.com>
> > > CC: Suren Baghdasaryan <surenb@google.com>
> > > CC: Barry Song <baohua@kernel.org>
> > > CC: SeongJae Park <sj@kernel.org>
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > >  mm/damon/ops-common.c | 16 ++++------------
> > >  mm/page_idle.c        |  8 ++------
> > >  mm/rmap.c             | 40 ++++++++++------------------------------
> > >  3 files changed, 16 insertions(+), 48 deletions(-)
> > >
> > > @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
> > >       anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
> > >       root_anon_vma = READ_ONCE(anon_vma->root);
> > >       if (down_read_trylock(&root_anon_vma->rwsem)) {
> > > -             /*
> > > -              * folio_move_anon_rmap() might have changed the anon_vma as we
> > > -              * might not hold the folio lock here.
> > > -              */
> > > -             if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> > > -                          anon_mapping)) {
> > > -                     up_read(&root_anon_vma->rwsem);
> > > -                     rcu_read_unlock();
> > > -                     goto retry;
> > > -             }
> > > -
> >
> > folio_lock_anon_vma_read() can be called without folio lock in a path:
> > memory_failure() -> kill_procs_now() -> collect_procs() ->
> > collect_procs_anon().
> >
>
> Thanks for catching this. Fell off the cracks for me.

No problem ;)

> > Not sure why collect_procs_{anon,ksm,file,fsdax} do not use rmap_walk()
> > functionality :/
> >
> > Should we take folio lock before calling kill_procs_now() in
> > memory_failure()?
> 
> To me it seems minimal (and sufficient) to put the collect_procs()
> call in kill_procs_now() inside folio lock's critical section.

Sounds sufficient to me.

-- 
Cheers,
Harry / Hyeonggon

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Mon, Sep 8, 2025 at 11:01 PM Barry Song <21cnbao@gmail.com> wrote:
>
> On Tue, Sep 9, 2025 at 1:57 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> >
> > On Mon, Sep 8, 2025 at 10:52 PM Barry Song <21cnbao@gmail.com> wrote:
> > >
> > > On Tue, Sep 9, 2025 at 1:37 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > >
> > > > On Mon, Sep 8, 2025 at 5:40 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > >
> > > > > On Tue, Sep 9, 2025 at 6:12 AM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > >
> > > > > > On Mon, Sep 8, 2025 at 2:47 PM Barry Song <21cnbao@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Sep 8, 2025 at 12:50 PM Lokesh Gidra <lokeshgidra@google.com> wrote:
> > > > > > > >
> > > > > > > > Prior discussion about this can be found at [1].
> > > > > > > >
> > > > > > > > rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> > > > > > > > implies that when threads update folio->mapping to an anon_vma with
> > > > > > > > different root (currently only done by UFFDIO MOVE), they have to
> > > > > > > > serialize against rmap_walk() with write-lock on the anon_vma, hurting
> > > > > > > > scalability. Furthermore, this necessitates rechecking anon_vma when
> > > > > > > > pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> > > > > > > >
> > > > > > > > This can be simplified quite a bit by ensuring that rmap_walk() is
> > > > > > > > always called on locked folios. Among the few callers of rmap_walk() on
> > > > > > > > unlocked anon folios, shrink_active_list()->folio_referenced() is the
> > > > > > > > only performance critical one.
> > > > > > >
> > > > > > > As I understand it, shrink_inactive_list() also invokes folio_referenced().
> > > > > > > Shouldn’t it be called just as often as shrink_active_list()?
> > > > > >
> > > > > > I'm only talking about those callers which call rmap_walk() without
> > > > > > locking anon folio. The
> > > > > > shrink_inactive_list()->folio_check_references()->folio_referenced()
> > > > > > path that you are talking about always locks the folio. So the
> > > > > > behavior in that case wouldn't change.
> > > > >
> > > > > Thanks for the clarification. Could you add a note about this if there
> > > > > is a v2?
> > > > >
> > > > Certainly, will do.
> > > > > > >
> > > > > > > >
> > > > > > > > shrink_active_list() doesn't act differently depending on what
> > > > > > > > folio_referenced() returns for an anon folio. So returning 1 when it
> > > > > > > > is contended, like in case of other folio types, wouldn't have any
> > > > > > > > negative impact.
> > > > > > >
> > > > > > > A complaint was raised that the LRU could become slightly disordered:
> > > > > > > https://lore.kernel.org/linux-mm/20240219141703.3851-1-lipeifeng@oppo.com/
> > > > > > >
> > > > > > > We can re-test to confirm if this is the case.
> > > > > > The patch in the link you provided is suggesting to control try-lock
> > > > > > for anon_vma lock. But here we are dealing with folio lock. Not sure
> > > > > > if the ordering issue will be there in this case.
> > > > >
> > > > > Right. Not sure what percentage of folios will be contended; I assume
> > > > > it is minor. Maybe you could share some data on this in a v2?
> > > >
> > > > Any suggestion on how (or which test/benchmark) would be good to
> > > > gather data on this?
> > > >
> > > > IIUC, shink_active_list() doesn't behave any differently whether there
> > > > is contention or not, except when it's an executable file folio. So I
> > > > doubt such data would be any useful. Please correct me if I'm wrong.
> > >
> > > Since we skipped clearing the PTE young bit in folio_referenced_one, a
> > > cold page might be misidentified as hot during shrink_inactive_list().
> > > My understanding is that as long as the percentage is small, this
> > > shouldn't be a concern.
> > I see. That makes a lot more sense why folio_referenced() is called on
> > all folios in shrink_active_list(). I missed that young bit clearing
> > before.
> >
> > Any suggestions on a good testcase to gather this data?
>
> I would run monkey for a few hours with some debug counters, e.g. how
> many folios pass through shrink_active_list(), how many get contended
> and moved to the inactive list without clearing the young bit. If the
> percentage is small, we can just ignore this disordered LRU behavior.
>
Thanks for the suggestion, Barry.

Monkey test wasn't successful in creating sufficient mem pressure. So,
I used an app cycle test. It took over 1 hour to complete it on an
arm64 Android device with memory limited to 6GB.

During the test shrink_active_list() got called over 140k times. Out
of that, over 29k invocations had at least one non-KSM anon folio.
None of the folio_referenced() calls on these folios ended up with
contention i.e. folio_trylock() failing.

So, as thought, this patch doesn't seem to have any negative effect on
shrink_active_list().

> Thanks
> Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lorenzo Stoakes]

Please please please use a cover letter if there's more than 1 patch :)

I really dislike the 2/2 replying to the 1/2.

On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> Prior discussion about this can be found at [1].
>
> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> implies that when threads update folio->mapping to an anon_vma with
> different root (currently only done by UFFDIO MOVE), they have to

I said this on the discussion thread, but can we please stop dancing around
and acting as if this isn't an entirely uffd-specific patch please :)

Let's very explicitly say that's why we're doing this.

> serialize against rmap_walk() with write-lock on the anon_vma, hurting
> scalability. Furthermore, this necessitates rechecking anon_vma when
> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).

THis is really quite confusing, you're compressing far too much information
into a single sentence.

Let's reword this to make it clearer like:

	Userfaultfd has a scaling issue with its UFFDIO_MOVE operation, an
	operation that is heavily used in android [insert reason why].

	The issue arises because UFFDIO_MOVE updates folio->mapping to an
	anon_vma with a different root. It acquires the folio lock to do
	so, but this is insufficient, because rmap_walk() has a mode in
	which a folio lock need not be acquired, exclusive to non-KSM
	anonymous folios.

	This means that UFFDIO_MOVE has to acquire the anon_vma write lock
	of the root anon_vma belonging to the folio it wishes to move.

	This has resulted in scalability issues due to contention between
	[insert contention information]. We have observed:

	[ insert some data to back this up ]

	This patch resolves the issue by removing this exception. This is
	less problematic than it might seem, as the only caller which
	utilises this mode is shrink_active_list().

Something like this is _a lot_ clearer I think.

>
> This can be simplified quite a bit by ensuring that rmap_walk() is
> always called on locked folios. Among the few callers of rmap_walk() on
> unlocked anon folios, shrink_active_list()->folio_referenced() is the
> only performance critical one.

Let's please not call this a simplification, I mean yes we simplify the
code per se, but we're fundamentally changing the locking logic.

Let's explicitly say that.

Also I find it odd that you say shrink_active_list()->folio_referenced() is
'performance critical', I mean if so, surely this series is broken then?

I'd delete that, the entire basis of this being ok is that it's _not_
performance critical to make this change.

>
> shrink_active_list() doesn't act differently depending on what
> folio_referenced() returns for an anon folio. So returning 1 when it
> is contended, like in case of other folio types, wouldn't have any
> negative impact.

I think this is a little unclear. I mean it very much _does_ behave
differently if it returns 0. So better to say it treats folio_referenced()
as a boolean, so if the folio_referenced() call returns an incorrect
referenced count while still indicating it is referenced that's fine.

>
> Furthermore, as David pointed out in the previous discussion [2], this
> could potentially only affect R/O pages after fork as PG_anon_exclusive
> is not set. But, such folios are already isolated (prior to calling
> folio_referenced()) by grabbing a reference and clearing LRU, so
> do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
> anyways.

I think this needs to be expanded too, this is still very unclear.

I think clearer to explicitly expand upon this, like e.g.:

	"Of course we now take a lock where we wouldn't previously have
	done so. In the past would have had a major impact in causing a CoW
	write fault to copy a page in do_wp_page(), since commit
	09854ba94c6a ("mm: do_wp_page() simplification") caused a failure
	to obtain a folio lock to result in a copy even if one wasn't
	necessary.

	However, since commit 6c287605fd56 ("mm: remember exclusively
	mapped anonymous pages with PG_anon_exclusive"), and the
	introduction of the folio anon exclusive flag, this issue is
	significantly mitigated.

	The only case remaining that we might worry about from this
	perspective is that of read-only folios immediately after fork
	where the anon exclusive bit will not have been set yet.

	We note however in the case of read-only just-forked folios that
	wp_can_reuse_anon_folio() will notice the raised reference count
	established by shrink_active_list() via isolate_lru_folios() and
	refuse to reuse in any case, so this will in fact have no impact -
	the folio lock is ultimately immaterial here.

	All-in-all it appears that there is little opportunity for
	meaningful negative impact from this change".

And now, having said all of that, you can talk about simplification,
something like:

	"As a result of changing our approach to locking, we can remove all
	the code that took steps to acuqire an anon_vma write lock instead
	of a folio lock. This results in a significant simplification of
	the code."

:)

>
> [1] https://lore.kernel.org/all/CA+EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5+4aDx5jh2DvY6UHg@mail.gmail.com/
> [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com/
>
> CC: David Hildenbrand <david@redhat.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Harry Yoo <harry.yoo@oracle.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Suren Baghdasaryan <surenb@google.com>
> CC: Barry Song <baohua@kernel.org>
> CC: SeongJae Park <sj@kernel.org>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---
>  mm/damon/ops-common.c | 16 ++++------------
>  mm/page_idle.c        |  8 ++------
>  mm/rmap.c             | 40 ++++++++++------------------------------
>  3 files changed, 16 insertions(+), 48 deletions(-)
>
> diff --git a/mm/damon/ops-common.c b/mm/damon/ops-common.c
> index 998c5180a603..f61d6dde13dc 100644
> --- a/mm/damon/ops-common.c
> +++ b/mm/damon/ops-common.c
> @@ -162,21 +162,17 @@ void damon_folio_mkold(struct folio *folio)
>  		.rmap_one = damon_folio_mkold_one,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};
> -	bool need_lock;
>
>  	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
>  		folio_set_idle(folio);
>  		return;
>  	}
>
> -	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> -	if (need_lock && !folio_trylock(folio))
> +	if (!folio_trylock(folio))
>  		return;
>
>  	rmap_walk(folio, &rwc);
> -
> -	if (need_lock)
> -		folio_unlock(folio);
> +	folio_unlock(folio);
>
>  }
>
> @@ -228,7 +224,6 @@ bool damon_folio_young(struct folio *folio)
>  		.rmap_one = damon_folio_young_one,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};
> -	bool need_lock;
>
>  	if (!folio_mapped(folio) || !folio_raw_mapping(folio)) {
>  		if (folio_test_idle(folio))
> @@ -237,14 +232,11 @@ bool damon_folio_young(struct folio *folio)
>  			return true;
>  	}
>
> -	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> -	if (need_lock && !folio_trylock(folio))
> +	if (!folio_trylock(folio))
>  		return false;
>
>  	rmap_walk(folio, &rwc);
> -
> -	if (need_lock)
> -		folio_unlock(folio);
> +	folio_unlock(folio);

Oh wow damon did this too... Are we sure we caught all such cases? Have you
checked carefully?

Hate that we've open-coded this all over the place.

>
>  	return accessed;
>  }
> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index a82b340dc204..9bf573d22e87 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -101,19 +101,15 @@ static void page_idle_clear_pte_refs(struct folio *folio)
>  		.rmap_one = page_idle_clear_pte_refs_one,
>  		.anon_lock = folio_lock_anon_vma_read,
>  	};
> -	bool need_lock;
>
>  	if (!folio_mapped(folio) || !folio_raw_mapping(folio))
>  		return;
>
> -	need_lock = !folio_test_anon(folio) || folio_test_ksm(folio);
> -	if (need_lock && !folio_trylock(folio))
> +	if (!folio_trylock(folio))
>  		return;

And more open-coding here, god. Horrid. It's quite nice to attack this actually.

>
>  	rmap_walk(folio, &rwc);
> -
> -	if (need_lock)
> -		folio_unlock(folio);
> +	folio_unlock(folio);
>  }
>
>  static ssize_t page_idle_bitmap_read(struct file *file, struct kobject *kobj,
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 34333ae3bd80..fc53f31434f4 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -489,17 +489,15 @@ void __init anon_vma_init(void)
>   * if there is a mapcount, we can dereference the anon_vma after observing
>   * those.
>   *
> - * NOTE: the caller should normally hold folio lock when calling this.  If
> - * not, the caller needs to double check the anon_vma didn't change after
> - * taking the anon_vma lock for either read or write (UFFDIO_MOVE can modify it
> - * concurrently without folio lock protection). See folio_lock_anon_vma_read()
> - * which has already covered that, and comment above remap_pages().
> + * NOTE: the caller should hold folio lock when calling this.
>   */
>  struct anon_vma *folio_get_anon_vma(const struct folio *folio)
>  {
>  	struct anon_vma *anon_vma = NULL;
>  	unsigned long anon_mapping;
>
> +	VM_WARN_ON_ONCE_FOLIO(!folio_test_locked(folio), folio);
> +
>  	rcu_read_lock();
>  	anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
>  	if ((anon_mapping & FOLIO_MAPPING_FLAGS) != FOLIO_MAPPING_ANON)
> @@ -546,7 +544,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	struct anon_vma *root_anon_vma;
>  	unsigned long anon_mapping;
>
> -retry:
>  	rcu_read_lock();
>  	anon_mapping = (unsigned long)READ_ONCE(folio->mapping);
>  	if ((anon_mapping & FOLIO_MAPPING_FLAGS) != FOLIO_MAPPING_ANON)
> @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
>  	root_anon_vma = READ_ONCE(anon_vma->root);
>  	if (down_read_trylock(&root_anon_vma->rwsem)) {
> -		/*
> -		 * folio_move_anon_rmap() might have changed the anon_vma as we
> -		 * might not hold the folio lock here.
> -		 */
> -		if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> -			     anon_mapping)) {
> -			up_read(&root_anon_vma->rwsem);
> -			rcu_read_unlock();
> -			goto retry;
> -		}
> -
>  		/*
>  		 * If the folio is still mapped, then this anon_vma is still
>  		 * its anon_vma, and holding the mutex ensures that it will
> @@ -602,18 +588,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>  	rcu_read_unlock();
>  	anon_vma_lock_read(anon_vma);
>
> -	/*
> -	 * folio_move_anon_rmap() might have changed the anon_vma as we might
> -	 * not hold the folio lock here.
> -	 */
> -	if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
> -		     anon_mapping)) {
> -		anon_vma_unlock_read(anon_vma);
> -		put_anon_vma(anon_vma);
> -		anon_vma = NULL;
> -		goto retry;
> -	}
> -
>  	if (atomic_dec_and_test(&anon_vma->refcount)) {
>  		/*
>  		 * Oops, we held the last refcount, release the lock
> @@ -1005,7 +979,7 @@ int folio_referenced(struct folio *folio, int is_locked,
>  	if (!folio_raw_mapping(folio))
>  		return 0;
>
> -	if (!is_locked && (!folio_test_anon(folio) || folio_test_ksm(folio))) {
> +	if (!is_locked) {
>  		we_locked = folio_trylock(folio);
>  		if (!we_locked)
>  			return 1;
> @@ -2815,6 +2789,12 @@ static void rmap_walk_anon(struct folio *folio,
>  	pgoff_t pgoff_start, pgoff_end;
>  	struct anon_vma_chain *avc;
>
> +	/*
> +	 * The folio lock ensures that folio->mapping can be changed under us to

Can't? :)

> +	 * an anon_vma with different root, like UFFDIO MOVE.

Not a fan of this 'like UFFDIO_MOVE'. I mean here I think we should just
drop that bit.

> +	 */
> +	VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);

PLEASE do not add BUG_ON(), checkpatch.pl will warn you on this and it's
not something we do these days pretty much ever. VM_WARN_ON_ONCE_FOLIO()
please.

> +
>  	if (locked) {
>  		anon_vma = folio_anon_vma(folio);
>  		/* anon_vma disappear under us? */
>
> base-commit: b024763926d2726978dff6588b81877d000159c1
> --
> 2.51.0.355.g5224444f11-goog
>
>

Cheers, Lorenzo

Re: [RFC PATCH 2/2] userfaultfd: remove anon-vma lock for moving folios in MOVE ioctl

[Lorenzo Stoakes]

Subject is weird, it's anon_vma not anon-vma and mm/userfaultfd seems more
appropriate as a prefix.

How about:

userfaultfd: don't lock anon_vma when performing UFFDIO_MOVE


On Sun, Sep 07, 2025 at 09:49:50PM -0700, Lokesh Gidra wrote:
> Since rmap_walk() is now always called on locked anon folios, we don't
> have to serialize on anon_vma lock when updating folio->mapping.

You're again being indirect about when this happens, please EXPLICITLY mention
uffd and UFFDIO_MOVE please.

I mean, this patch ilterailiy has userfautlfd in the prefix of the subject,
there's no need to be indirect here :)

>
> This helps avoid contention on src anon_vma when multiple threads are
> simultaneously moving distinct pages from the same src vma.

It feels like you need to add more here. I don't know what 'src' anon_vma is,
spell it out as the anon_vma belonging to the folio which you intend to move,
and be a little more specific - when multiple threads invoke UFFDIO_MOVE on the
same VMA.

Also here is a _prime_ chance to mention the simplification which is
something you do achieve here (very clearly) as a result of changing the
locking approach.

>
> CC: David Hildenbrand <david@redhat.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Suren Baghdasaryan <surenb@google.com>
> CC: Barry Song <baohua@kernel.org>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>

This generally LGTM, but let's revisit on respin w/commit msg update.

I do like the simplification here :)

Cheers, Lorenzo

> ---
>  mm/huge_memory.c | 22 +----------------
>  mm/userfaultfd.c | 62 +++++++++---------------------------------------
>  2 files changed, 12 insertions(+), 72 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 26cedfcd7418..5cd3957f92d4 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -2533,7 +2533,6 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
>  	pmd_t _dst_pmd, src_pmdval;
>  	struct page *src_page;
>  	struct folio *src_folio;
> -	struct anon_vma *src_anon_vma;
>  	spinlock_t *src_ptl, *dst_ptl;
>  	pgtable_t src_pgtable;
>  	struct mmu_notifier_range range;
> @@ -2582,23 +2581,9 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
>  				src_addr + HPAGE_PMD_SIZE);
>  	mmu_notifier_invalidate_range_start(&range);
>
> -	if (src_folio) {
> +	if (src_folio)
>  		folio_lock(src_folio);
>
> -		/*
> -		 * split_huge_page walks the anon_vma chain without the page
> -		 * lock. Serialize against it with the anon_vma lock, the page
> -		 * lock is not enough.
> -		 */
> -		src_anon_vma = folio_get_anon_vma(src_folio);
> -		if (!src_anon_vma) {
> -			err = -EAGAIN;
> -			goto unlock_folio;
> -		}
> -		anon_vma_lock_write(src_anon_vma);
> -	} else
> -		src_anon_vma = NULL;
> -
>  	dst_ptl = pmd_lockptr(mm, dst_pmd);
>  	double_pt_lock(src_ptl, dst_ptl);
>  	if (unlikely(!pmd_same(*src_pmd, src_pmdval) ||
> @@ -2643,11 +2628,6 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
>  	pgtable_trans_huge_deposit(mm, dst_pmd, src_pgtable);
>  unlock_ptls:
>  	double_pt_unlock(src_ptl, dst_ptl);
> -	if (src_anon_vma) {
> -		anon_vma_unlock_write(src_anon_vma);
> -		put_anon_vma(src_anon_vma);
> -	}
> -unlock_folio:
>  	/* unblock rmap walks */
>  	if (src_folio)
>  		folio_unlock(src_folio);
> diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> index 50aaa8dcd24c..1a36760a36c7 100644
> --- a/mm/userfaultfd.c
> +++ b/mm/userfaultfd.c
> @@ -1035,8 +1035,7 @@ static inline bool is_pte_pages_stable(pte_t *dst_pte, pte_t *src_pte,
>   */
>  static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
>  						 unsigned long src_addr,
> -						 pte_t *src_pte, pte_t *dst_pte,
> -						 struct anon_vma *src_anon_vma)
> +						 pte_t *src_pte, pte_t *dst_pte)
>  {
>  	pte_t orig_dst_pte, orig_src_pte;
>  	struct folio *folio;
> @@ -1052,8 +1051,7 @@ static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
>  	folio = vm_normal_folio(src_vma, src_addr, orig_src_pte);
>  	if (!folio || !folio_trylock(folio))
>  		return NULL;
> -	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio) ||
> -	    folio_anon_vma(folio) != src_anon_vma) {
> +	if (!PageAnonExclusive(&folio->page) || folio_test_large(folio)) {
>  		folio_unlock(folio);
>  		return NULL;
>  	}
> @@ -1061,9 +1059,8 @@ static struct folio *check_ptes_for_batched_move(struct vm_area_struct *src_vma,
>  }
>
>  /*
> - * Moves src folios to dst in a batch as long as they share the same
> - * anon_vma as the first folio, are not large, and can successfully
> - * take the lock via folio_trylock().
> + * Moves src folios to dst in a batch as long as they are not large, and can
> + * successfully take the lock via folio_trylock().
>   */
>  static long move_present_ptes(struct mm_struct *mm,
>  			      struct vm_area_struct *dst_vma,
> @@ -1073,8 +1070,7 @@ static long move_present_ptes(struct mm_struct *mm,
>  			      pte_t orig_dst_pte, pte_t orig_src_pte,
>  			      pmd_t *dst_pmd, pmd_t dst_pmdval,
>  			      spinlock_t *dst_ptl, spinlock_t *src_ptl,
> -			      struct folio **first_src_folio, unsigned long len,
> -			      struct anon_vma *src_anon_vma)
> +			      struct folio **first_src_folio, unsigned long len)
>  {
>  	int err = 0;
>  	struct folio *src_folio = *first_src_folio;
> @@ -1132,8 +1128,8 @@ static long move_present_ptes(struct mm_struct *mm,
>  		src_pte++;
>
>  		folio_unlock(src_folio);
> -		src_folio = check_ptes_for_batched_move(src_vma, src_addr, src_pte,
> -							dst_pte, src_anon_vma);
> +		src_folio = check_ptes_for_batched_move(src_vma, src_addr,
> +							src_pte, dst_pte);
>  		if (!src_folio)
>  			break;
>  	}
> @@ -1263,7 +1259,6 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
>  	pmd_t dummy_pmdval;
>  	pmd_t dst_pmdval;
>  	struct folio *src_folio = NULL;
> -	struct anon_vma *src_anon_vma = NULL;
>  	struct mmu_notifier_range range;
>  	long ret = 0;
>
> @@ -1347,9 +1342,9 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
>  		}
>
>  		/*
> -		 * Pin and lock both source folio and anon_vma. Since we are in
> -		 * RCU read section, we can't block, so on contention have to
> -		 * unmap the ptes, obtain the lock and retry.
> +		 * Pin and lock source folio. Since we are in RCU read section,
> +		 * we can't block, so on contention have to unmap the ptes,
> +		 * obtain the lock and retry.
>  		 */
>  		if (!src_folio) {
>  			struct folio *folio;
> @@ -1423,33 +1418,11 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
>  			goto retry;
>  		}
>
> -		if (!src_anon_vma) {
> -			/*
> -			 * folio_referenced walks the anon_vma chain
> -			 * without the folio lock. Serialize against it with
> -			 * the anon_vma lock, the folio lock is not enough.
> -			 */
> -			src_anon_vma = folio_get_anon_vma(src_folio);
> -			if (!src_anon_vma) {
> -				/* page was unmapped from under us */
> -				ret = -EAGAIN;
> -				goto out;
> -			}
> -			if (!anon_vma_trylock_write(src_anon_vma)) {
> -				pte_unmap(src_pte);
> -				pte_unmap(dst_pte);
> -				src_pte = dst_pte = NULL;
> -				/* now we can block and wait */
> -				anon_vma_lock_write(src_anon_vma);
> -				goto retry;
> -			}
> -		}
> -
>  		ret = move_present_ptes(mm, dst_vma, src_vma,
>  					dst_addr, src_addr, dst_pte, src_pte,
>  					orig_dst_pte, orig_src_pte, dst_pmd,
>  					dst_pmdval, dst_ptl, src_ptl, &src_folio,
> -					len, src_anon_vma);
> +					len);
>  	} else {
>  		struct folio *folio = NULL;
>
> @@ -1516,10 +1489,6 @@ static long move_pages_ptes(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd
>  	}
>
>  out:
> -	if (src_anon_vma) {
> -		anon_vma_unlock_write(src_anon_vma);
> -		put_anon_vma(src_anon_vma);
> -	}
>  	if (src_folio) {
>  		folio_unlock(src_folio);
>  		folio_put(src_folio);
> @@ -1793,15 +1762,6 @@ static void uffd_move_unlock(struct vm_area_struct *dst_vma,
>   * virtual regions without knowing if there are transparent hugepage
>   * in the regions or not, but preventing the risk of having to split
>   * the hugepmd during the remap.
> - *
> - * If there's any rmap walk that is taking the anon_vma locks without
> - * first obtaining the folio lock (the only current instance is
> - * folio_referenced), they will have to verify if the folio->mapping
> - * has changed after taking the anon_vma lock. If it changed they
> - * should release the lock and retry obtaining a new anon_vma, because
> - * it means the anon_vma was changed by move_pages() before the lock
> - * could be obtained. This is the only additional complexity added to
> - * the rmap code to provide this anonymous page remapping functionality.
>   */
>  ssize_t move_pages(struct userfaultfd_ctx *ctx, unsigned long dst_start,
>  		   unsigned long src_start, unsigned long len, __u64 mode)
> --
> 2.51.0.355.g5224444f11-goog
>
>

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Miaohe Lin]

On 2025/9/10 18:10, Harry Yoo wrote:
> On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
>> Prior discussion about this can be found at [1].
>>
>> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
>> implies that when threads update folio->mapping to an anon_vma with
>> different root (currently only done by UFFDIO MOVE), they have to
>> serialize against rmap_walk() with write-lock on the anon_vma, hurting
>> scalability. Furthermore, this necessitates rechecking anon_vma when
>> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
>>
>> This can be simplified quite a bit by ensuring that rmap_walk() is
>> always called on locked folios. Among the few callers of rmap_walk() on
>> unlocked anon folios, shrink_active_list()->folio_referenced() is the
>> only performance critical one.
>>
>> shrink_active_list() doesn't act differently depending on what
>> folio_referenced() returns for an anon folio. So returning 1 when it
>> is contended, like in case of other folio types, wouldn't have any
>> negative impact.
>>
>> Furthermore, as David pointed out in the previous discussion [2], this
>> could potentially only affect R/O pages after fork as PG_anon_exclusive
>> is not set. But, such folios are already isolated (prior to calling
>> folio_referenced()) by grabbing a reference and clearing LRU, so
>> do_wp_page()->wp_can_reuse_anon_folio() would not reuse such folios
>> anyways.
>>
>> [1] https://lore.kernel.org/all/CA*EESO4Z6wtX7ZMdDHQRe5jAAS_bQ-POq5*4aDx5jh2DvY6UHg@mail.gmail.com
>> [2] https://lore.kernel.org/all/dc92aef8-757f-4432-923e-70d92d13fb37@redhat.com
>>
>> CC: David Hildenbrand <david@redhat.com>
>> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>> CC: Harry Yoo <harry.yoo@oracle.com>
>> CC: Peter Xu <peterx@redhat.com>
>> CC: Suren Baghdasaryan <surenb@google.com>
>> CC: Barry Song <baohua@kernel.org>
>> CC: SeongJae Park <sj@kernel.org>
>> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
>> ---
>>  mm/damon/ops-common.c | 16 ++++------------
>>  mm/page_idle.c        |  8 ++------
>>  mm/rmap.c             | 40 ++++++++++------------------------------
>>  3 files changed, 16 insertions(+), 48 deletions(-)
>>
>> @@ -557,17 +554,6 @@ struct anon_vma *folio_lock_anon_vma_read(const struct folio *folio,
>>  	anon_vma = (struct anon_vma *) (anon_mapping - FOLIO_MAPPING_ANON);
>>  	root_anon_vma = READ_ONCE(anon_vma->root);
>>  	if (down_read_trylock(&root_anon_vma->rwsem)) {
>> -		/*
>> -		 * folio_move_anon_rmap() might have changed the anon_vma as we
>> -		 * might not hold the folio lock here.
>> -		 */
>> -		if (unlikely((unsigned long)READ_ONCE(folio->mapping) !=
>> -			     anon_mapping)) {
>> -			up_read(&root_anon_vma->rwsem);
>> -			rcu_read_unlock();
>> -			goto retry;
>> -		}
>> -
> 
> folio_lock_anon_vma_read() can be called without folio lock in a path:
> memory_failure() -> kill_procs_now() -> collect_procs() ->
> collect_procs_anon().
> 
> Not sure why collect_procs_{anon,ksm,file,fsdax} do not use rmap_walk()
> functionality :/

Because we need to find all processes having the page mapped and kill them.
But there's no convenient way to get back to mapped processes from the VMAs.
So a brute-force search over all running processes is used here.

> 
> Should we take folio lock before calling kill_procs_now() in
> memory_failure()?

I agree with Lokesh that we should put kill_procs_now() inside folio lock's critical section.

Thanks.
.

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Barry Song]

[...]
> > > > >
> > > > > IIUC, shink_active_list() doesn't behave any differently whether there
> > > > > is contention or not, except when it's an executable file folio. So I
> > > > > doubt such data would be any useful. Please correct me if I'm wrong.
> > > >
> > > > Since we skipped clearing the PTE young bit in folio_referenced_one, a
> > > > cold page might be misidentified as hot during shrink_inactive_list().
> > > > My understanding is that as long as the percentage is small, this
> > > > shouldn't be a concern.
> > > I see. That makes a lot more sense why folio_referenced() is called on
> > > all folios in shrink_active_list(). I missed that young bit clearing
> > > before.
> > >
> > > Any suggestions on a good testcase to gather this data?
> >
> > I would run monkey for a few hours with some debug counters, e.g. how
> > many folios pass through shrink_active_list(), how many get contended
> > and moved to the inactive list without clearing the young bit. If the
> > percentage is small, we can just ignore this disordered LRU behavior.
> >
> Thanks for the suggestion, Barry.
>
> Monkey test wasn't successful in creating sufficient mem pressure. So,
> I used an app cycle test. It took over 1 hour to complete it on an
> arm64 Android device with memory limited to 6GB.
>
> During the test shrink_active_list() got called over 140k times. Out
> of that, over 29k invocations had at least one non-KSM anon folio.
> None of the folio_referenced() calls on these folios ended up with
> contention i.e. folio_trylock() failing.
>
> So, as thought, this patch doesn't seem to have any negative effect on
> shrink_active_list().

Cool, thanks! It seems to meet expectations, given that a folio is
a very small granularity. I also raised another discussion [1],
which, if it works out, should provide one more reason why we
need your patches.


[1] https://lore.kernel.org/linux-mm/CAGsJ_4x=YsQR=nNcHA-q=0vg0b7ok=81C_qQqKmoJ+BZ+HVduQ@mail.gmail.com/

Thanks
Barry

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[David Hildenbrand]

On 11.09.25 21:39, Lorenzo Stoakes wrote:
> Please please please use a cover letter if there's more than 1 patch :)
> 
> I really dislike the 2/2 replying to the 1/2.

+1000

> 
> On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
>> Prior discussion about this can be found at [1].
>>
>> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
>> implies that when threads update folio->mapping to an anon_vma with
>> different root (currently only done by UFFDIO MOVE), they have to
> 
> I said this on the discussion thread, but can we please stop dancing around
> and acting as if this isn't an entirely uffd-specific patch please :)
> 
> Let's very explicitly say that's why we're doing this.
> 
>> serialize against rmap_walk() with write-lock on the anon_vma, hurting
>> scalability. Furthermore, this necessitates rechecking anon_vma when
>> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> 
> THis is really quite confusing, you're compressing far too much information
> into a single sentence.
> 
> Let's reword this to make it clearer like:
> 
> 	Userfaultfd has a scaling issue with its UFFDIO_MOVE operation, an
> 	operation that is heavily used in android [insert reason why].
> 
> 	The issue arises because UFFDIO_MOVE updates folio->mapping to an
> 	anon_vma with a different root. It acquires the folio lock to do
> 	so, but this is insufficient, because rmap_walk() has a mode in
> 	which a folio lock need not be acquired, exclusive to non-KSM
> 	anonymous folios.
> 
> 	This means that UFFDIO_MOVE has to acquire the anon_vma write lock
> 	of the root anon_vma belonging to the folio it wishes to move.
> 
> 	This has resulted in scalability issues due to contention between
> 	[insert contention information]. We have observed:
> 
> 	[ insert some data to back this up ]
> 
> 	This patch resolves the issue by removing this exception. This is
> 	less problematic than it might seem, as the only caller which
> 	utilises this mode is shrink_active_list().
> 
> Something like this is _a lot_ clearer I think.

Yes, fully agreed.

> 
>>
>> This can be simplified quite a bit by ensuring that rmap_walk() is
>> always called on locked folios. Among the few callers of rmap_walk() on
>> unlocked anon folios, shrink_active_list()->folio_referenced() is the
>> only performance critical one.
> 
> Let's please not call this a simplification, I mean yes we simplify the
> code per se, but we're fundamentally changing the locking logic.
> 
> Let's explicitly say that.
> 
> Also I find it odd that you say shrink_active_list()->folio_referenced() is
> 'performance critical', I mean if so, surely this series is broken then?
> 
> I'd delete that, the entire basis of this being ok is that it's _not_
> performance critical to make this change.

I think we can mention that as a side-effect of this performance 
optimization for uffd, folio_get_anon_vma() gets simpler and we no 
langer handle locking of anon folios different to locking of other 
(pagecache, ksm) folios.


-- 
Cheers

David / dhildenb

Re: [RFC PATCH 2/2] userfaultfd: remove anon-vma lock for moving folios in MOVE ioctl

[David Hildenbrand]

On 08.09.25 06:49, Lokesh Gidra wrote:
> Since rmap_walk() is now always called on locked anon folios, we don't
> have to serialize on anon_vma lock when updating folio->mapping.

I would write that as

"Now that rmap_walk() is guaranteed to be called with the folio lock 
held, we can stop serializing on the src VMA anon_vma lock when moving 
an exclusive folio from one a src VMA to a dst VMA.

When moving a folio, we modify folio->mapping through 
folio_move_anon_rmap() and adjust folio->index accordingly. Doing that
while we could have concurrent RMAP walks would be dangerous.

folio_move_anon_rmap() already enforces that the folio is locked. So
when we have the folio locked we can no longer race with concurrent
rmap_walk() as used by folio_referenced() and the anon_vma lock is no 
longer required.

Note that this handling is now the same as for other 
folio_move_anon_rmap() users that also do not hold the anon_vma lock -- 
namely COW reuse handling. These users never required the anon_vma lock
as they are only moving the anon VMA closer to the anon_vma leaf of the 
VMA, for example, from an anon_vma root to a leaf of that root. rmap 
walks were always able to tolerate that scenario."

Something like that.

> 
> This helps avoid contention on src anon_vma when multiple threads are
> simultaneously moving distinct pages from the same src vma.
> 
> CC: David Hildenbrand <david@redhat.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Peter Xu <peterx@redhat.com>
> CC: Suren Baghdasaryan <surenb@google.com>
> CC: Barry Song <baohua@kernel.org>
> Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> ---



-- 
Cheers

David / dhildenb

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lokesh Gidra]

On Fri, Sep 12, 2025 at 2:03 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 11.09.25 21:39, Lorenzo Stoakes wrote:
> > Please please please use a cover letter if there's more than 1 patch :)
> >
> > I really dislike the 2/2 replying to the 1/2.
>
> +1000
>
> >
> > On Sun, Sep 07, 2025 at 09:49:49PM -0700, Lokesh Gidra wrote:
> >> Prior discussion about this can be found at [1].
> >>
> >> rmap_walk() requires all folios, except non-KSM anon, to be locked. This
> >> implies that when threads update folio->mapping to an anon_vma with
> >> different root (currently only done by UFFDIO MOVE), they have to
> >
> > I said this on the discussion thread, but can we please stop dancing around
> > and acting as if this isn't an entirely uffd-specific patch please :)
> >
> > Let's very explicitly say that's why we're doing this.
> >
> >> serialize against rmap_walk() with write-lock on the anon_vma, hurting
> >> scalability. Furthermore, this necessitates rechecking anon_vma when
> >> pinning/locking an anon_vma (like in folio_lock_anon_vma_read()).
> >
> > THis is really quite confusing, you're compressing far too much information
> > into a single sentence.
> >
> > Let's reword this to make it clearer like:
> >
> >       Userfaultfd has a scaling issue with its UFFDIO_MOVE operation, an
> >       operation that is heavily used in android [insert reason why].
> >
> >       The issue arises because UFFDIO_MOVE updates folio->mapping to an
> >       anon_vma with a different root. It acquires the folio lock to do
> >       so, but this is insufficient, because rmap_walk() has a mode in
> >       which a folio lock need not be acquired, exclusive to non-KSM
> >       anonymous folios.
> >
> >       This means that UFFDIO_MOVE has to acquire the anon_vma write lock
> >       of the root anon_vma belonging to the folio it wishes to move.
> >
> >       This has resulted in scalability issues due to contention between
> >       [insert contention information]. We have observed:
> >
> >       [ insert some data to back this up ]
> >
> >       This patch resolves the issue by removing this exception. This is
> >       less problematic than it might seem, as the only caller which
> >       utilises this mode is shrink_active_list().
> >
> > Something like this is _a lot_ clearer I think.
>
> Yes, fully agreed.
>
> >
> >>
> >> This can be simplified quite a bit by ensuring that rmap_walk() is
> >> always called on locked folios. Among the few callers of rmap_walk() on
> >> unlocked anon folios, shrink_active_list()->folio_referenced() is the
> >> only performance critical one.
> >
> > Let's please not call this a simplification, I mean yes we simplify the
> > code per se, but we're fundamentally changing the locking logic.
> >
> > Let's explicitly say that.
> >
> > Also I find it odd that you say shrink_active_list()->folio_referenced() is
> > 'performance critical', I mean if so, surely this series is broken then?
> >
> > I'd delete that, the entire basis of this being ok is that it's _not_
> > performance critical to make this change.
>
> I think we can mention that as a side-effect of this performance
> optimization for uffd, folio_get_anon_vma() gets simpler and we no
> langer handle locking of anon folios different to locking of other
> (pagecache, ksm) folios.
>
Thank you both for the valuable feedback. I'll upload next version
within few days addressing all the comments.
>
> --
> Cheers
>
> David / dhildenb
>

Re: [RFC PATCH 1/2] mm: always call rmap_walk() on locked folios

[Lorenzo Stoakes]

On Fri, Sep 12, 2025 at 09:27:03PM -0700, Lokesh Gidra wrote:
> Thank you both for the valuable feedback. I'll upload next version
> within few days addressing all the comments.

Thanks! :)

Cheers, Lorenzo