[PATCH v2 0/8] mm: clean up anon_vma implementation

[PATCH v2 0/8] mm: clean up anon_vma implementation

[Lorenzo Stoakes]

The anon_vma logic is hugely confusing and, much like a bundle of wires
entangled with one another, pulling on one thread seems only to lead to
more entanglement elsewhere.

There is a mish-mash of the core implementation, how that implementation is
invoked, how helper functions are invoked and concepts such as adjacent
anon_vma merge and anon_vma object reuse.

This series tries to improve the situation somewhat.

It starts by establishing some invariants in the core anon_vma_clone() and
unlink_anon_vmas() functions, largely expressed via VM_WARN_ON_ONCE()
asserts.

These act as some form of self-documentation as to the conditions we find
ourselves in when invoking these functions.

We also add kdoc comments for anon_vma_clone() and unlink_anon_vmas().

We then makes use of these known conditions to directly skip unfaulted VMAs
(rather than implicitly via an empty vma->anon_vma_chain list).

We remove the confusing anon_vma_merge() function (we already have a
concept of anon_vma merge in that we merge anon_vma's that would otherwise
be compatible except for attributes that mprotect() could change - which
anon_vma_merge() has nothing to do with).

We make the anon_vma functions internal to mm as they do not need to be
used by any other part of the kernel, which allows for future abstraction
without concern about this.

We then reduce the time over which we hold the anon rmap lock in
anon_vma_clone(), as it turns out we can allocate anon_vma_chain objects
without holding this lock, since the objects are not yet accessible from
the rmap.

This should reduce anon_vma lock contention.

This additionally allows us to remove a confusing GFP_NOWAIT, GFP_KERNEL
allocation fallback strategy.

Finally, we explicitly indicate which operation is being performed upon
anon_vma_clone(), and separate out fork-only logic to make it very clear
that anon_vma reuse only occurs on fork.


v2:
* Propagated tags (thanks all!)
* Corrected typo in 1/8 as per Suren.
* Updated commit message in 1/8 to clarify when we use a downgraded read
  lock in unlink_anon_vmas(), as per Suren.
* Updated !src->anon_vma no-op comment as per Suren.
* When anon_vma_clone() fails to allocate we have thus far been invoking
  unlink_anon_vmas() to clean up the partially set up VMA. However this
  means we have to account for this (likely impossible) scenario in the
  code and prevents further improvements. Resolve by adding a partial
  cleanup function specifically for this case.
* Fixed various other typos.
* Placed check_anon_vma_clone() before the !src->anon_vma check in
  anon_vma_clone() in 2/8 as per Suren.
* Retained !vma->anon_vma && !list_empty(&vma->anon_vma_chain) check on
  unlink_anon_vmas() as per Liam.
* Added comment about anon_vma's sharing same root in 3/8 as per Suren.
* Updated 7/8 to have cleanup_partial_anon_vmas() do even less - since we
  now allocate AVC's first before inserting into the interval tree we do
  not need to acquire the lock or do anything special here, just clean up
  the AVC's.
* Updated commit messages as necessary.
* Renamed find_reusable_anon_vma() to try_to_reuse_anon_vma() for clarity
  as per Suren.
* Added a new assert to check_anon_vma_clone() to make it clear that, when
  not forking, we expect dst->anon_vma to be set.
* Renamed vma to dst in try_to_reuse_anon_vma() to make it clear that we're
  checking/manipulating the destination VMA.

v1:
https://lore.kernel.org/all/cover.1765970117.git.lorenzo.stoakes@oracle.com/

Lorenzo Stoakes (8):
  mm/rmap: improve anon_vma_clone(), unlink_anon_vmas() comments, add
    asserts
  mm/rmap: skip unfaulted VMAs on anon_vma clone, unlink
  mm/rmap: remove unnecessary root lock dance in anon_vma clone, unmap
  mm/rmap: remove anon_vma_merge() function
  mm/rmap: make anon_vma functions internal
  mm/mmap_lock: add vma_is_attached() helper
  mm/rmap: allocate anon_vma_chain objects unlocked when possible
  mm/rmap: separate out fork-only logic on anon_vma_clone()

 include/linux/mmap_lock.h        |   9 +-
 include/linux/rmap.h             |  67 --------
 mm/internal.h                    |  67 ++++++++
 mm/rmap.c                        | 271 ++++++++++++++++++++-----------
 mm/vma.c                         |   8 +-
 tools/testing/vma/vma_internal.h |  16 +-
 6 files changed, 264 insertions(+), 174 deletions(-)

--
2.52.0

[PATCH v2 1/8] mm/rmap: improve anon_vma_clone(), unlink_anon_vmas() comments, add asserts

[Lorenzo Stoakes]

Add kdoc comments and describe exactly what these functions are used for in
detail, pointing out importantly that the anon_vma_clone() !dst->anon_vma
&& src->anon_vma dance is ONLY for fork.

Both are confusing functions that will be refactored in a subsequent patch
but the first stage is establishing documentation and some invariants.

Add some basic CONFIG_DEBUG_VM asserts that help document expected state,
specifically:

anon_vma_clone()
- mmap write lock held.
- We do nothing if src VMA is not faulted.
- The destination VMA has no anon_vma_chain yet.
- We are always operating on the same active VMA (i.e. vma->anon_vma).
- If not forking, must operate on the same mm_struct.

unlink_anon_vmas()
- mmap lock held (write lock except when freeing page tables).
- That unfaulted VMAs are no-ops.

We are presented with a special case when anon_vma_clone() fails to
allocate memory, where we have a VMA with partially set up anon_vma state.
Since we hold the exclusive mmap write lock, and since we are cloning from
a source VMA which consequently can't also have its anon_vma state
modified, we know no anon_vma referenced can be empty.

This allows us to significantly simplify this case and just remove
anon_vma_chain objects associated with the VMA, so we add a specific
partial cleanup path for this scenario.

This also allows us to drop the hack of setting vma->anon_vma to NULL
before unlinking anon_vma state in this scenario.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
---
 mm/rmap.c | 130 +++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 104 insertions(+), 26 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index c86f1135222b..54ccf884d90a 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -258,30 +258,62 @@ static inline void unlock_anon_vma_root(struct anon_vma *root)
 		up_write(&root->rwsem);
 }
 
-/*
- * Attach the anon_vmas from src to dst.
- * Returns 0 on success, -ENOMEM on failure.
- *
- * anon_vma_clone() is called by vma_expand(), vma_merge(), __split_vma(),
- * copy_vma() and anon_vma_fork(). The first four want an exact copy of src,
- * while the last one, anon_vma_fork(), may try to reuse an existing anon_vma to
- * prevent endless growth of anon_vma. Since dst->anon_vma is set to NULL before
- * call, we can identify this case by checking (!dst->anon_vma &&
- * src->anon_vma).
- *
- * If (!dst->anon_vma && src->anon_vma) is true, this function tries to find
- * and reuse existing anon_vma which has no vmas and only one child anon_vma.
- * This prevents degradation of anon_vma hierarchy to endless linear chain in
- * case of constantly forking task. On the other hand, an anon_vma with more
- * than one child isn't reused even if there was no alive vma, thus rmap
- * walker has a good chance of avoiding scanning the whole hierarchy when it
- * searches where page is mapped.
+static void check_anon_vma_clone(struct vm_area_struct *dst,
+				 struct vm_area_struct *src)
+{
+	/* The write lock must be held. */
+	mmap_assert_write_locked(src->vm_mm);
+	/* If not a fork (implied by dst->anon_vma) then must be on same mm. */
+	VM_WARN_ON_ONCE(dst->anon_vma && dst->vm_mm != src->vm_mm);
+
+	/* If we have anything to do src->anon_vma must be provided. */
+	VM_WARN_ON_ONCE(!src->anon_vma && !list_empty(&src->anon_vma_chain));
+	VM_WARN_ON_ONCE(!src->anon_vma && dst->anon_vma);
+	/* We are establishing a new anon_vma_chain. */
+	VM_WARN_ON_ONCE(!list_empty(&dst->anon_vma_chain));
+	/*
+	 * On fork, dst->anon_vma is set NULL (temporarily). Otherwise, anon_vma
+	 * must be the same across dst and src.
+	 */
+	VM_WARN_ON_ONCE(dst->anon_vma && dst->anon_vma != src->anon_vma);
+}
+
+static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
+
+/**
+ * anon_vma_clone - Establishes new anon_vma_chain objects in @dst linking to
+ * all of the anon_vma objects contained within @src anon_vma_chain's.
+ * @dst: The destination VMA with an empty anon_vma_chain.
+ * @src: The source VMA we wish to duplicate.
+ *
+ * This is the heart of the VMA side of the anon_vma implementation - we invoke
+ * this function whenever we need to set up a new VMA's anon_vma state.
+ *
+ * This is invoked for:
+ *
+ * - VMA Merge, but only when @dst is unfaulted and @src is faulted - meaning we
+ *   clone @src into @dst.
+ * - VMA split.
+ * - VMA (m)remap.
+ * - Fork of faulted VMA.
+ *
+ * In all cases other than fork this is simply a duplication. Fork additionally
+ * adds a new active anon_vma.
+ *
+ * ONLY in the case of fork do we try to 'reuse' existing anon_vma's in an
+ * anon_vma hierarchy, reusing anon_vma's which have no VMA associated with them
+ * but do have a single child. This is to avoid waste of memory when repeatedly
+ * forking.
+ *
+ * Returns: 0 on success, -ENOMEM on failure.
  */
 int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 {
 	struct anon_vma_chain *avc, *pavc;
 	struct anon_vma *root = NULL;
 
+	check_anon_vma_clone(dst, src);
+
 	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
 		struct anon_vma *anon_vma;
 
@@ -315,14 +347,7 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 	return 0;
 
  enomem_failure:
-	/*
-	 * dst->anon_vma is dropped here otherwise its num_active_vmas can
-	 * be incorrectly decremented in unlink_anon_vmas().
-	 * We can safely do this because callers of anon_vma_clone() don't care
-	 * about dst->anon_vma if anon_vma_clone() failed.
-	 */
-	dst->anon_vma = NULL;
-	unlink_anon_vmas(dst);
+	cleanup_partial_anon_vmas(dst);
 	return -ENOMEM;
 }
 
@@ -393,11 +418,64 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	return -ENOMEM;
 }
 
+/*
+ * In the unfortunate case of anon_vma_clone() failing to allocate memory we
+ * have to clean things up.
+ *
+ * On clone we hold the exclusive mmap write lock, so we can't race
+ * unlink_anon_vmas(). Since we're cloning, we know we can't have empty
+ * anon_vma's, since existing anon_vma's are what we're cloning from.
+ *
+ * So this function needs only traverse the anon_vma_chain and free each
+ * allocated anon_vma_chain.
+ */
+static void cleanup_partial_anon_vmas(struct vm_area_struct *vma)
+{
+	struct anon_vma_chain *avc, *next;
+	bool locked = false;
+
+	/*
+	 * We exclude everybody else from being able to modify anon_vma's
+	 * underneath us.
+	 */
+	mmap_assert_locked(vma->vm_mm);
+
+	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
+		struct anon_vma *anon_vma = avc->anon_vma;
+
+		/* All anon_vma's share the same root. */
+		if (!locked) {
+			anon_vma_lock_write(anon_vma);
+			locked = true;
+		}
+
+		anon_vma_interval_tree_remove(avc, &anon_vma->rb_root);
+		list_del(&avc->same_vma);
+		anon_vma_chain_free(avc);
+	}
+}
+
+/**
+ * unlink_anon_vmas() - remove all links between a VMA and anon_vma's, freeing
+ * anon_vma_chain objects.
+ * @vma: The VMA whose links to anon_vma objects is to be severed.
+ *
+ * As part of the process anon_vma_chain's are freed,
+ * anon_vma->num_children,num_active_vmas is updated as required and, if the
+ * relevant anon_vma references no further VMAs, its reference count is
+ * decremented.
+ */
 void unlink_anon_vmas(struct vm_area_struct *vma)
 {
 	struct anon_vma_chain *avc, *next;
 	struct anon_vma *root = NULL;
 
+	/* Always hold mmap lock, read-lock on unmap possibly. */
+	mmap_assert_locked(vma->vm_mm);
+
+	/* Unfaulted is a no-op. */
+	VM_WARN_ON_ONCE(!vma->anon_vma && !list_empty(&vma->anon_vma_chain));
+
 	/*
 	 * Unlink each anon_vma chained to the VMA.  This list is ordered
 	 * from newest to oldest, ensuring the root anon_vma gets freed last.
-- 
2.52.0

[PATCH v2 2/8] mm/rmap: skip unfaulted VMAs on anon_vma clone, unlink

[Lorenzo Stoakes]

For both anon_vma_clone() and unlink_anon_vmas(), if the source VMA or the
VMA to be linked are unfaulted (e.g. !vma->anon_vma), then the functions do
nothing. Simply exit early in these cases.

In the unlink_anon_vmas() case we can also remove a conditional that checks
whether vma->anon_vma is set.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/rmap.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 54ccf884d90a..de2cbe860566 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -314,6 +314,9 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 
 	check_anon_vma_clone(dst, src);
 
+	if (!src->anon_vma)
+		return 0;
+
 	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
 		struct anon_vma *anon_vma;
 
@@ -474,7 +477,10 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
 	mmap_assert_locked(vma->vm_mm);
 
 	/* Unfaulted is a no-op. */
-	VM_WARN_ON_ONCE(!vma->anon_vma && !list_empty(&vma->anon_vma_chain));
+	if (!vma->anon_vma) {
+		VM_WARN_ON_ONCE(!list_empty(&vma->anon_vma_chain));
+		return;
+	}
 
 	/*
 	 * Unlink each anon_vma chained to the VMA.  This list is ordered
@@ -498,15 +504,13 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
 		list_del(&avc->same_vma);
 		anon_vma_chain_free(avc);
 	}
-	if (vma->anon_vma) {
-		vma->anon_vma->num_active_vmas--;
 
-		/*
-		 * vma would still be needed after unlink, and anon_vma will be prepared
-		 * when handle fault.
-		 */
-		vma->anon_vma = NULL;
-	}
+	vma->anon_vma->num_active_vmas--;
+	/*
+	 * vma would still be needed after unlink, and anon_vma will be prepared
+	 * when handle fault.
+	 */
+	vma->anon_vma = NULL;
 	unlock_anon_vma_root(root);
 
 	/*
-- 
2.52.0

[PATCH v2 3/8] mm/rmap: remove unnecessary root lock dance in anon_vma clone, unmap

[Lorenzo Stoakes]

The root anon_vma of all anon_vma's linked to a VMA must by definition be
the same - a VMA and all of its descendants/ancestors must exist in the
same CoW chain.

Commit bb4aa39676f7 ("mm: avoid repeated anon_vma lock/unlock sequences in
anon_vma_clone()") introduced paranoid checking of the root anon_vma
remaining the same throughout all AVC's in 2011.

I think 15 years later we can safely assume that this is always the case.

Additionally, since unfaulted VMAs being cloned from or unlinked are
no-op's, we can simply lock the anon_vma's associated with this rather than
doing any specific dance around this.

This removes unnecessary checks and makes it clear that the root anon_vma
is shared between all anon_vma's in a given VMA's anon_vma_chain.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/rmap.c | 51 +++++++++++++++------------------------------------
 1 file changed, 15 insertions(+), 36 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index de2cbe860566..6ac42671bedd 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -232,32 +232,6 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
 	return -ENOMEM;
 }
 
-/*
- * This is a useful helper function for locking the anon_vma root as
- * we traverse the vma->anon_vma_chain, looping over anon_vma's that
- * have the same vma.
- *
- * Such anon_vma's should have the same root, so you'd expect to see
- * just a single mutex_lock for the whole traversal.
- */
-static inline struct anon_vma *lock_anon_vma_root(struct anon_vma *root, struct anon_vma *anon_vma)
-{
-	struct anon_vma *new_root = anon_vma->root;
-	if (new_root != root) {
-		if (WARN_ON_ONCE(root))
-			up_write(&root->rwsem);
-		root = new_root;
-		down_write(&root->rwsem);
-	}
-	return root;
-}
-
-static inline void unlock_anon_vma_root(struct anon_vma *root)
-{
-	if (root)
-		up_write(&root->rwsem);
-}
-
 static void check_anon_vma_clone(struct vm_area_struct *dst,
 				 struct vm_area_struct *src)
 {
@@ -310,26 +284,28 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
 int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 {
 	struct anon_vma_chain *avc, *pavc;
-	struct anon_vma *root = NULL;
 
 	check_anon_vma_clone(dst, src);
 
 	if (!src->anon_vma)
 		return 0;
 
+	check_anon_vma_clone(dst, src);
+
+	/* All anon_vma's share the same root. */
+	anon_vma_lock_write(src->anon_vma);
 	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
 		struct anon_vma *anon_vma;
 
 		avc = anon_vma_chain_alloc(GFP_NOWAIT);
 		if (unlikely(!avc)) {
-			unlock_anon_vma_root(root);
-			root = NULL;
+			anon_vma_unlock_write(src->anon_vma);
 			avc = anon_vma_chain_alloc(GFP_KERNEL);
 			if (!avc)
 				goto enomem_failure;
+			anon_vma_lock_write(src->anon_vma);
 		}
 		anon_vma = pavc->anon_vma;
-		root = lock_anon_vma_root(root, anon_vma);
 		anon_vma_chain_link(dst, avc, anon_vma);
 
 		/*
@@ -346,7 +322,8 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 	}
 	if (dst->anon_vma)
 		dst->anon_vma->num_active_vmas++;
-	unlock_anon_vma_root(root);
+
+	anon_vma_unlock_write(src->anon_vma);
 	return 0;
 
  enomem_failure:
@@ -471,17 +448,19 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma)
 void unlink_anon_vmas(struct vm_area_struct *vma)
 {
 	struct anon_vma_chain *avc, *next;
-	struct anon_vma *root = NULL;
+	struct anon_vma *active_anon_vma = vma->anon_vma;
 
 	/* Always hold mmap lock, read-lock on unmap possibly. */
 	mmap_assert_locked(vma->vm_mm);
 
 	/* Unfaulted is a no-op. */
-	if (!vma->anon_vma) {
+	if (!active_anon_vma) {
 		VM_WARN_ON_ONCE(!list_empty(&vma->anon_vma_chain));
 		return;
 	}
 
+	anon_vma_lock_write(active_anon_vma);
+
 	/*
 	 * Unlink each anon_vma chained to the VMA.  This list is ordered
 	 * from newest to oldest, ensuring the root anon_vma gets freed last.
@@ -489,7 +468,6 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
 	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
 		struct anon_vma *anon_vma = avc->anon_vma;
 
-		root = lock_anon_vma_root(root, anon_vma);
 		anon_vma_interval_tree_remove(avc, &anon_vma->rb_root);
 
 		/*
@@ -505,13 +483,14 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
 		anon_vma_chain_free(avc);
 	}
 
-	vma->anon_vma->num_active_vmas--;
+	active_anon_vma->num_active_vmas--;
 	/*
 	 * vma would still be needed after unlink, and anon_vma will be prepared
 	 * when handle fault.
 	 */
 	vma->anon_vma = NULL;
-	unlock_anon_vma_root(root);
+	anon_vma_unlock_write(active_anon_vma);
+
 
 	/*
 	 * Iterate the list once more, it now only contains empty and unlinked
-- 
2.52.0

[PATCH v2 4/8] mm/rmap: remove anon_vma_merge() function

[Lorenzo Stoakes]

This function is confusing, we already have the concept of anon_vma merge
to adjacent VMA's anon_vma's to increase probability of anon_vma
compatibility and therefore VMA merge (see is_mergeable_anon_vma() etc.),
as well as anon_vma reuse, along side the usual VMA merge logic.

We can remove the anon_vma check as it is redundant - a merge would not
have been permitted with removal if the anon_vma's were not the same (and
in the case of an unfaulted/faulted merge, we would have already set the
unfaulted VMA's anon_vma to vp->remove->anon_vma in dup_anon_vma()).

Avoid overloading this term when we're very simply unlinking anon_vma state
from a removed VMA upon merge.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/rmap.h             | 7 -------
 mm/vma.c                         | 2 +-
 tools/testing/vma/vma_internal.h | 5 -----
 3 files changed, 1 insertion(+), 13 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index daa92a58585d..832bfc0ccfc6 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -165,13 +165,6 @@ static inline int anon_vma_prepare(struct vm_area_struct *vma)
 	return __anon_vma_prepare(vma);
 }
 
-static inline void anon_vma_merge(struct vm_area_struct *vma,
-				  struct vm_area_struct *next)
-{
-	VM_BUG_ON_VMA(vma->anon_vma != next->anon_vma, vma);
-	unlink_anon_vmas(next);
-}
-
 struct anon_vma *folio_get_anon_vma(const struct folio *folio);
 
 #ifdef CONFIG_MM_ID
diff --git a/mm/vma.c b/mm/vma.c
index fb45a6be7417..4294ecdc23a5 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -379,7 +379,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
 			fput(vp->file);
 		}
 		if (vp->remove->anon_vma)
-			anon_vma_merge(vp->vma, vp->remove);
+			unlink_anon_vmas(vp->remove);
 		mm->map_count--;
 		mpol_put(vma_policy(vp->remove));
 		if (!vp->remove2)
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 9f0a9f5ed0fe..93e5792306d9 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -1265,11 +1265,6 @@ static inline void i_mmap_unlock_write(struct address_space *mapping)
 {
 }
 
-static inline void anon_vma_merge(struct vm_area_struct *vma,
-				  struct vm_area_struct *next)
-{
-}
-
 static inline int userfaultfd_unmap_prep(struct vm_area_struct *vma,
 					 unsigned long start,
 					 unsigned long end,
-- 
2.52.0

[PATCH v2 5/8] mm/rmap: make anon_vma functions internal

[Lorenzo Stoakes]

The bulk of the anon_vma operations are only used by mm, so formalise this
by putting the function prototypes and inlines in mm/internal.h. This
allows us to make changes without having to worry about the rest of the
kernel.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/rmap.h | 60 --------------------------------------------
 mm/internal.h        | 58 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 60 deletions(-)

diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 832bfc0ccfc6..dd764951b03d 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -104,68 +104,8 @@ enum ttu_flags {
 };
 
 #ifdef CONFIG_MMU
-static inline void get_anon_vma(struct anon_vma *anon_vma)
-{
-	atomic_inc(&anon_vma->refcount);
-}
-
-void __put_anon_vma(struct anon_vma *anon_vma);
-
-static inline void put_anon_vma(struct anon_vma *anon_vma)
-{
-	if (atomic_dec_and_test(&anon_vma->refcount))
-		__put_anon_vma(anon_vma);
-}
-
-static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
-{
-	down_write(&anon_vma->root->rwsem);
-}
 
-static inline int anon_vma_trylock_write(struct anon_vma *anon_vma)
-{
-	return down_write_trylock(&anon_vma->root->rwsem);
-}
-
-static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
-{
-	up_write(&anon_vma->root->rwsem);
-}
-
-static inline void anon_vma_lock_read(struct anon_vma *anon_vma)
-{
-	down_read(&anon_vma->root->rwsem);
-}
-
-static inline int anon_vma_trylock_read(struct anon_vma *anon_vma)
-{
-	return down_read_trylock(&anon_vma->root->rwsem);
-}
-
-static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
-{
-	up_read(&anon_vma->root->rwsem);
-}
-
-
-/*
- * anon_vma helper functions.
- */
 void anon_vma_init(void);	/* create anon_vma_cachep */
-int  __anon_vma_prepare(struct vm_area_struct *);
-void unlink_anon_vmas(struct vm_area_struct *);
-int anon_vma_clone(struct vm_area_struct *, struct vm_area_struct *);
-int anon_vma_fork(struct vm_area_struct *, struct vm_area_struct *);
-
-static inline int anon_vma_prepare(struct vm_area_struct *vma)
-{
-	if (likely(vma->anon_vma))
-		return 0;
-
-	return __anon_vma_prepare(vma);
-}
-
-struct anon_vma *folio_get_anon_vma(const struct folio *folio);
 
 #ifdef CONFIG_MM_ID
 static __always_inline void folio_lock_large_mapcount(struct folio *folio)
diff --git a/mm/internal.h b/mm/internal.h
index 8319a91e363b..4ba784023a9f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -199,6 +199,64 @@ static inline void vma_close(struct vm_area_struct *vma)
 
 #ifdef CONFIG_MMU
 
+static inline void get_anon_vma(struct anon_vma *anon_vma)
+{
+	atomic_inc(&anon_vma->refcount);
+}
+
+void __put_anon_vma(struct anon_vma *anon_vma);
+
+static inline void put_anon_vma(struct anon_vma *anon_vma)
+{
+	if (atomic_dec_and_test(&anon_vma->refcount))
+		__put_anon_vma(anon_vma);
+}
+
+static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
+{
+	down_write(&anon_vma->root->rwsem);
+}
+
+static inline int anon_vma_trylock_write(struct anon_vma *anon_vma)
+{
+	return down_write_trylock(&anon_vma->root->rwsem);
+}
+
+static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
+{
+	up_write(&anon_vma->root->rwsem);
+}
+
+static inline void anon_vma_lock_read(struct anon_vma *anon_vma)
+{
+	down_read(&anon_vma->root->rwsem);
+}
+
+static inline int anon_vma_trylock_read(struct anon_vma *anon_vma)
+{
+	return down_read_trylock(&anon_vma->root->rwsem);
+}
+
+static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
+{
+	up_read(&anon_vma->root->rwsem);
+}
+
+struct anon_vma *folio_get_anon_vma(const struct folio *folio);
+
+int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src);
+int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma);
+int  __anon_vma_prepare(struct vm_area_struct *vma);
+void unlink_anon_vmas(struct vm_area_struct *vma);
+
+static inline int anon_vma_prepare(struct vm_area_struct *vma)
+{
+	if (likely(vma->anon_vma))
+		return 0;
+
+	return __anon_vma_prepare(vma);
+}
+
 /* Flags for folio_pte_batch(). */
 typedef int __bitwise fpb_t;
 
-- 
2.52.0

[PATCH v2 6/8] mm/mmap_lock: add vma_is_attached() helper

[Lorenzo Stoakes]

This makes it easy to explicitly check for VMA detachment, which is useful
for things like asserts.

Note that we intentionally do not allow this function to be available
should CONFIG_PER_VMA_LOCK be set - this is because vma_assert_attached()
and vma_assert_detached() are no-ops if !CONFIG_PER_VMA_LOCK, so there is
no correct state for vma_is_attached() to be in if this configuration
option is not specified.

Therefore users elsewhere must invoke this function only after checking for
CONFIG_PER_VMA_LOCK.

We rework the assert functions to utilise this.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mmap_lock.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index d53f72dba7fe..b50416fbba20 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -251,6 +251,11 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
 		      !__is_vma_write_locked(vma, &mm_lock_seq), vma);
 }
 
+static inline bool vma_is_attached(struct vm_area_struct *vma)
+{
+	return refcount_read(&vma->vm_refcnt);
+}
+
 /*
  * WARNING: to avoid racing with vma_mark_attached()/vma_mark_detached(), these
  * assertions should be made either under mmap_write_lock or when the object
@@ -258,12 +263,12 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
  */
 static inline void vma_assert_attached(struct vm_area_struct *vma)
 {
-	WARN_ON_ONCE(!refcount_read(&vma->vm_refcnt));
+	WARN_ON_ONCE(!vma_is_attached(vma));
 }
 
 static inline void vma_assert_detached(struct vm_area_struct *vma)
 {
-	WARN_ON_ONCE(refcount_read(&vma->vm_refcnt));
+	WARN_ON_ONCE(vma_is_attached(vma));
 }
 
 static inline void vma_mark_attached(struct vm_area_struct *vma)
-- 
2.52.0

[PATCH v2 7/8] mm/rmap: allocate anon_vma_chain objects unlocked when possible

[Lorenzo Stoakes]

There is no reason to allocate the anon_vma_chain under the anon_vma write
lock when cloning - we can in fact assign these to the destination VMA
safely as we hold the exclusive mmap lock and therefore preclude anybody
else accessing these fields.

We only need take the anon_vma write lock when we link rbtree edges from
the anon_vma to the newly established AVCs.

This also allows us to eliminate the weird GFP_NOWAIT, GFP_KERNEL dance
introduced in commit dd34739c03f2 ("mm: avoid anon_vma_chain allocation
under anon_vma lock"), further simplifying this logic.

This should reduce lock anon_vma contention, and clarifies exactly where
the anon_vma lock is required.

We cannot adjust __anon_vma_prepare() in the same way as this is only
protected by VMA read lock, so we have to perform the allocation here under
the anon_vma write lock and page_table_lock (to protect against racing
threads), and we wish to retain the lock ordering.

With this change we can simplify cleanup_partial_anon_vmas() even further -
since we allocate AVC's without any lock taken and do not insert anything
into the interval tree until after the allocations are tried, we can remove
all logic pertaining to this and just free up AVC's only.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 mm/rmap.c | 78 +++++++++++++++++++++++++------------------------------
 1 file changed, 35 insertions(+), 43 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 6ac42671bedd..8f4393546bce 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -147,14 +147,13 @@ static void anon_vma_chain_free(struct anon_vma_chain *anon_vma_chain)
 	kmem_cache_free(anon_vma_chain_cachep, anon_vma_chain);
 }
 
-static void anon_vma_chain_link(struct vm_area_struct *vma,
-				struct anon_vma_chain *avc,
-				struct anon_vma *anon_vma)
+static void anon_vma_chain_assign(struct vm_area_struct *vma,
+				  struct anon_vma_chain *avc,
+				  struct anon_vma *anon_vma)
 {
 	avc->vma = vma;
 	avc->anon_vma = anon_vma;
 	list_add(&avc->same_vma, &vma->anon_vma_chain);
-	anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 }
 
 /**
@@ -211,7 +210,8 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
 	spin_lock(&mm->page_table_lock);
 	if (likely(!vma->anon_vma)) {
 		vma->anon_vma = anon_vma;
-		anon_vma_chain_link(vma, avc, anon_vma);
+		anon_vma_chain_assign(vma, avc, anon_vma);
+		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 		anon_vma->num_active_vmas++;
 		allocated = NULL;
 		avc = NULL;
@@ -292,21 +292,31 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 
 	check_anon_vma_clone(dst, src);
 
-	/* All anon_vma's share the same root. */
+	/*
+	 * Allocate AVCs. We don't need an anon_vma lock for this as we
+	 * are not updating the anon_vma rbtree nor are we changing
+	 * anon_vma statistics.
+	 *
+	 * We hold the exclusive mmap write lock so there's no possibliity of
+	 * the unlinked AVC's being observed yet.
+	 */
+	list_for_each_entry(pavc, &src->anon_vma_chain, same_vma) {
+		avc = anon_vma_chain_alloc(GFP_KERNEL);
+		if (!avc)
+			goto enomem_failure;
+
+		anon_vma_chain_assign(dst, avc, pavc->anon_vma);
+	}
+
+	/*
+	 * Now link the anon_vma's back to the newly inserted AVCs.
+	 * Note that all anon_vma's share the same root.
+	 */
 	anon_vma_lock_write(src->anon_vma);
-	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
-		struct anon_vma *anon_vma;
-
-		avc = anon_vma_chain_alloc(GFP_NOWAIT);
-		if (unlikely(!avc)) {
-			anon_vma_unlock_write(src->anon_vma);
-			avc = anon_vma_chain_alloc(GFP_KERNEL);
-			if (!avc)
-				goto enomem_failure;
-			anon_vma_lock_write(src->anon_vma);
-		}
-		anon_vma = pavc->anon_vma;
-		anon_vma_chain_link(dst, avc, anon_vma);
+	list_for_each_entry_reverse(avc, &dst->anon_vma_chain, same_vma) {
+		struct anon_vma *anon_vma = avc->anon_vma;
+
+		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 
 		/*
 		 * Reuse existing anon_vma if it has no vma and only one
@@ -322,7 +332,6 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 	}
 	if (dst->anon_vma)
 		dst->anon_vma->num_active_vmas++;
-
 	anon_vma_unlock_write(src->anon_vma);
 	return 0;
 
@@ -384,8 +393,10 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	get_anon_vma(anon_vma->root);
 	/* Mark this anon_vma as the one where our new (COWed) pages go. */
 	vma->anon_vma = anon_vma;
+	anon_vma_chain_assign(vma, avc, anon_vma);
+	/* Now let rmap see it. */
 	anon_vma_lock_write(anon_vma);
-	anon_vma_chain_link(vma, avc, anon_vma);
+	anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 	anon_vma->parent->num_children++;
 	anon_vma_unlock_write(anon_vma);
 
@@ -402,34 +413,15 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
  * In the unfortunate case of anon_vma_clone() failing to allocate memory we
  * have to clean things up.
  *
- * On clone we hold the exclusive mmap write lock, so we can't race
- * unlink_anon_vmas(). Since we're cloning, we know we can't have empty
- * anon_vma's, since existing anon_vma's are what we're cloning from.
- *
- * So this function needs only traverse the anon_vma_chain and free each
- * allocated anon_vma_chain.
+ * Since we allocate anon_vma_chain's before we insert them into the interval
+ * trees, we simply have to free up the AVC's and remove the entries from the
+ * VMA's anon_vma_chain.
  */
 static void cleanup_partial_anon_vmas(struct vm_area_struct *vma)
 {
 	struct anon_vma_chain *avc, *next;
-	bool locked = false;
-
-	/*
-	 * We exclude everybody else from being able to modify anon_vma's
-	 * underneath us.
-	 */
-	mmap_assert_locked(vma->vm_mm);
 
 	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
-		struct anon_vma *anon_vma = avc->anon_vma;
-
-		/* All anon_vma's share the same root. */
-		if (!locked) {
-			anon_vma_lock_write(anon_vma);
-			locked = true;
-		}
-
-		anon_vma_interval_tree_remove(avc, &anon_vma->rb_root);
 		list_del(&avc->same_vma);
 		anon_vma_chain_free(avc);
 	}
-- 
2.52.0

[PATCH v2 8/8] mm/rmap: separate out fork-only logic on anon_vma_clone()

[Lorenzo Stoakes]

Specify which operation is being performed to anon_vma_clone(), which
allows us to do checks specific to each operation type, as well as to
separate out and make clear that the anon_vma reuse logic is absolutely
specific to fork only.

This opens the door to further refactorings and refinements later as we
have more information to work with.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/internal.h                    | 11 ++++-
 mm/rmap.c                        | 74 ++++++++++++++++++++++----------
 mm/vma.c                         |  6 +--
 tools/testing/vma/vma_internal.h | 11 ++++-
 4 files changed, 74 insertions(+), 28 deletions(-)

diff --git a/mm/internal.h b/mm/internal.h
index 4ba784023a9f..8baa7bd2b8f7 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -244,7 +244,16 @@ static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
 
 struct anon_vma *folio_get_anon_vma(const struct folio *folio);
 
-int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src);
+/* Operations which modify VMAs. */
+enum vma_operation {
+	VMA_OP_SPLIT,
+	VMA_OP_MERGE_UNFAULTED,
+	VMA_OP_REMAP,
+	VMA_OP_FORK,
+};
+
+int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
+	enum vma_operation operation);
 int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma);
 int  __anon_vma_prepare(struct vm_area_struct *vma);
 void unlink_anon_vmas(struct vm_area_struct *vma);
diff --git a/mm/rmap.c b/mm/rmap.c
index 8f4393546bce..336b27e00238 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -233,12 +233,13 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
 }
 
 static void check_anon_vma_clone(struct vm_area_struct *dst,
-				 struct vm_area_struct *src)
+				 struct vm_area_struct *src,
+				 enum vma_operation operation)
 {
 	/* The write lock must be held. */
 	mmap_assert_write_locked(src->vm_mm);
-	/* If not a fork (implied by dst->anon_vma) then must be on same mm. */
-	VM_WARN_ON_ONCE(dst->anon_vma && dst->vm_mm != src->vm_mm);
+	/* If not a fork then must be on same mm. */
+	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && dst->vm_mm != src->vm_mm);
 
 	/* If we have anything to do src->anon_vma must be provided. */
 	VM_WARN_ON_ONCE(!src->anon_vma && !list_empty(&src->anon_vma_chain));
@@ -250,6 +251,40 @@ static void check_anon_vma_clone(struct vm_area_struct *dst,
 	 * must be the same across dst and src.
 	 */
 	VM_WARN_ON_ONCE(dst->anon_vma && dst->anon_vma != src->anon_vma);
+	/*
+	 * Essentially equivalent to above - if not a no-op, we should expect
+	 * dst->anon_vma to be set for everything except a fork.
+	 */
+	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && src->anon_vma &&
+			!dst->anon_vma);
+	/* For the anon_vma to be compatible, it can only be singular. */
+	VM_WARN_ON_ONCE(operation == VMA_OP_MERGE_UNFAULTED &&
+			!list_is_singular(&src->anon_vma_chain));
+#ifdef CONFIG_PER_VMA_LOCK
+	/* Only merging an unfaulted VMA leaves the destination attached. */
+	VM_WARN_ON_ONCE(operation != VMA_OP_MERGE_UNFAULTED &&
+			vma_is_attached(dst));
+#endif
+}
+
+static void try_to_reuse_anon_vma(struct vm_area_struct *dst,
+				  struct anon_vma *anon_vma)
+{
+	/* If already populated, nothing to do.*/
+	if (dst->anon_vma)
+		return;
+
+	/*
+	 * We reuse an anon_vma if any linking VMAs were unmapped and it has
+	 * only a single child at most.
+	 */
+	if (anon_vma->num_active_vmas > 0)
+		return;
+	if (anon_vma->num_children > 1)
+		return;
+
+	dst->anon_vma = anon_vma;
+	anon_vma->num_active_vmas++;
 }
 
 static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
@@ -259,6 +294,7 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
  * all of the anon_vma objects contained within @src anon_vma_chain's.
  * @dst: The destination VMA with an empty anon_vma_chain.
  * @src: The source VMA we wish to duplicate.
+ * @operation: The type of operation which resulted in the clone.
  *
  * This is the heart of the VMA side of the anon_vma implementation - we invoke
  * this function whenever we need to set up a new VMA's anon_vma state.
@@ -281,17 +317,17 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
  *
  * Returns: 0 on success, -ENOMEM on failure.
  */
-int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
+int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
+		   enum vma_operation operation)
 {
 	struct anon_vma_chain *avc, *pavc;
+	struct anon_vma *active_anon_vma = src->anon_vma;
 
-	check_anon_vma_clone(dst, src);
+	check_anon_vma_clone(dst, src, operation);
 
-	if (!src->anon_vma)
+	if (!active_anon_vma)
 		return 0;
 
-	check_anon_vma_clone(dst, src);
-
 	/*
 	 * Allocate AVCs. We don't need an anon_vma lock for this as we
 	 * are not updating the anon_vma rbtree nor are we changing
@@ -317,22 +353,14 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 		struct anon_vma *anon_vma = avc->anon_vma;
 
 		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
-
-		/*
-		 * Reuse existing anon_vma if it has no vma and only one
-		 * anon_vma child.
-		 *
-		 * Root anon_vma is never reused:
-		 * it has self-parent reference and at least one child.
-		 */
-		if (!dst->anon_vma && src->anon_vma &&
-		    anon_vma->num_children < 2 &&
-		    anon_vma->num_active_vmas == 0)
-			dst->anon_vma = anon_vma;
+		if (operation == VMA_OP_FORK)
+			try_to_reuse_anon_vma(dst, anon_vma);
 	}
-	if (dst->anon_vma)
+
+	if (operation != VMA_OP_FORK)
 		dst->anon_vma->num_active_vmas++;
-	anon_vma_unlock_write(src->anon_vma);
+
+	anon_vma_unlock_write(active_anon_vma);
 	return 0;
 
  enomem_failure:
@@ -362,7 +390,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
 	 * First, attach the new VMA to the parent VMA's anon_vmas,
 	 * so rmap can find non-COWed pages in child processes.
 	 */
-	error = anon_vma_clone(vma, pvma);
+	error = anon_vma_clone(vma, pvma, VMA_OP_FORK);
 	if (error)
 		return error;
 
diff --git a/mm/vma.c b/mm/vma.c
index 4294ecdc23a5..2a063d6568d9 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -528,7 +528,7 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
 	if (err)
 		goto out_free_vmi;
 
-	err = anon_vma_clone(new, vma);
+	err = anon_vma_clone(new, vma, VMA_OP_SPLIT);
 	if (err)
 		goto out_free_mpol;
 
@@ -626,7 +626,7 @@ static int dup_anon_vma(struct vm_area_struct *dst,
 
 		vma_assert_write_locked(dst);
 		dst->anon_vma = src->anon_vma;
-		ret = anon_vma_clone(dst, src);
+		ret = anon_vma_clone(dst, src, VMA_OP_MERGE_UNFAULTED);
 		if (ret)
 			return ret;
 
@@ -1899,7 +1899,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 		vma_set_range(new_vma, addr, addr + len, pgoff);
 		if (vma_dup_policy(vma, new_vma))
 			goto out_free_vma;
-		if (anon_vma_clone(new_vma, vma))
+		if (anon_vma_clone(new_vma, vma, VMA_OP_REMAP))
 			goto out_free_mempol;
 		if (new_vma->vm_file)
 			get_file(new_vma->vm_file);
diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
index 93e5792306d9..7fa56dcc53a6 100644
--- a/tools/testing/vma/vma_internal.h
+++ b/tools/testing/vma/vma_internal.h
@@ -600,6 +600,14 @@ struct mmap_action {
 	bool hide_from_rmap_until_complete :1;
 };
 
+/* Operations which modify VMAs. */
+enum vma_operation {
+	VMA_OP_SPLIT,
+	VMA_OP_MERGE_UNFAULTED,
+	VMA_OP_REMAP,
+	VMA_OP_FORK,
+};
+
 /*
  * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
  * manipulate mutable fields which will cause those fields to be updated in the
@@ -1157,7 +1165,8 @@ static inline int vma_dup_policy(struct vm_area_struct *src, struct vm_area_stru
 	return 0;
 }
 
-static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
+static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
+				 enum vma_operation operation)
 {
 	/* For testing purposes. We indicate that an anon_vma has been cloned. */
 	if (src->anon_vma != NULL) {
-- 
2.52.0

Re: [PATCH v2 2/8] mm/rmap: skip unfaulted VMAs on anon_vma clone, unlink

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> For both anon_vma_clone() and unlink_anon_vmas(), if the source VMA or the
> VMA to be linked are unfaulted (e.g. !vma->anon_vma), then the functions do
> nothing. Simply exit early in these cases.
> 
> In the unlink_anon_vmas() case we can also remove a conditional that checks
> whether vma->anon_vma is set.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/rmap.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 54ccf884d90a..de2cbe860566 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -314,6 +314,9 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  
>  	check_anon_vma_clone(dst, src);
>  
> +	if (!src->anon_vma)
> +		return 0;
> +
>  	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
>  		struct anon_vma *anon_vma;
>  
> @@ -474,7 +477,10 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
>  	mmap_assert_locked(vma->vm_mm);
>  
>  	/* Unfaulted is a no-op. */
> -	VM_WARN_ON_ONCE(!vma->anon_vma && !list_empty(&vma->anon_vma_chain));
> +	if (!vma->anon_vma) {
> +		VM_WARN_ON_ONCE(!list_empty(&vma->anon_vma_chain));
> +		return;
> +	}
>  
>  	/*
>  	 * Unlink each anon_vma chained to the VMA.  This list is ordered
> @@ -498,15 +504,13 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
>  		list_del(&avc->same_vma);
>  		anon_vma_chain_free(avc);
>  	}
> -	if (vma->anon_vma) {
> -		vma->anon_vma->num_active_vmas--;
>  
> -		/*
> -		 * vma would still be needed after unlink, and anon_vma will be prepared
> -		 * when handle fault.
> -		 */
> -		vma->anon_vma = NULL;
> -	}
> +	vma->anon_vma->num_active_vmas--;
> +	/*
> +	 * vma would still be needed after unlink, and anon_vma will be prepared
> +	 * when handle fault.
> +	 */
> +	vma->anon_vma = NULL;
>  	unlock_anon_vma_root(root);
>  
>  	/*
> -- 
> 2.52.0
> 

Re: [PATCH v2 3/8] mm/rmap: remove unnecessary root lock dance in anon_vma clone, unmap

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> The root anon_vma of all anon_vma's linked to a VMA must by definition be
> the same - a VMA and all of its descendants/ancestors must exist in the
> same CoW chain.
> 
> Commit bb4aa39676f7 ("mm: avoid repeated anon_vma lock/unlock sequences in
> anon_vma_clone()") introduced paranoid checking of the root anon_vma
> remaining the same throughout all AVC's in 2011.
> 
> I think 15 years later we can safely assume that this is always the case.
> 
> Additionally, since unfaulted VMAs being cloned from or unlinked are
> no-op's, we can simply lock the anon_vma's associated with this rather than
> doing any specific dance around this.
> 
> This removes unnecessary checks and makes it clear that the root anon_vma
> is shared between all anon_vma's in a given VMA's anon_vma_chain.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

One extra whitespace, but lgtm.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/rmap.c | 51 +++++++++++++++------------------------------------
>  1 file changed, 15 insertions(+), 36 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index de2cbe860566..6ac42671bedd 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -232,32 +232,6 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>  	return -ENOMEM;
>  }
>  
> -/*
> - * This is a useful helper function for locking the anon_vma root as
> - * we traverse the vma->anon_vma_chain, looping over anon_vma's that
> - * have the same vma.
> - *
> - * Such anon_vma's should have the same root, so you'd expect to see
> - * just a single mutex_lock for the whole traversal.
> - */
> -static inline struct anon_vma *lock_anon_vma_root(struct anon_vma *root, struct anon_vma *anon_vma)
> -{
> -	struct anon_vma *new_root = anon_vma->root;
> -	if (new_root != root) {
> -		if (WARN_ON_ONCE(root))
> -			up_write(&root->rwsem);
> -		root = new_root;
> -		down_write(&root->rwsem);
> -	}
> -	return root;
> -}
> -
> -static inline void unlock_anon_vma_root(struct anon_vma *root)
> -{
> -	if (root)
> -		up_write(&root->rwsem);
> -}
> -
>  static void check_anon_vma_clone(struct vm_area_struct *dst,
>  				 struct vm_area_struct *src)
>  {
> @@ -310,26 +284,28 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
>  int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  {
>  	struct anon_vma_chain *avc, *pavc;
> -	struct anon_vma *root = NULL;
>  
>  	check_anon_vma_clone(dst, src);
>  
>  	if (!src->anon_vma)
>  		return 0;
>  
> +	check_anon_vma_clone(dst, src);
> +
> +	/* All anon_vma's share the same root. */
> +	anon_vma_lock_write(src->anon_vma);
>  	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
>  		struct anon_vma *anon_vma;
>  
>  		avc = anon_vma_chain_alloc(GFP_NOWAIT);
>  		if (unlikely(!avc)) {
> -			unlock_anon_vma_root(root);
> -			root = NULL;
> +			anon_vma_unlock_write(src->anon_vma);
>  			avc = anon_vma_chain_alloc(GFP_KERNEL);
>  			if (!avc)
>  				goto enomem_failure;
> +			anon_vma_lock_write(src->anon_vma);
>  		}
>  		anon_vma = pavc->anon_vma;
> -		root = lock_anon_vma_root(root, anon_vma);
>  		anon_vma_chain_link(dst, avc, anon_vma);
>  
>  		/*
> @@ -346,7 +322,8 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  	}
>  	if (dst->anon_vma)
>  		dst->anon_vma->num_active_vmas++;
> -	unlock_anon_vma_root(root);
> +
> +	anon_vma_unlock_write(src->anon_vma);
>  	return 0;
>  
>   enomem_failure:
> @@ -471,17 +448,19 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma)
>  void unlink_anon_vmas(struct vm_area_struct *vma)
>  {
>  	struct anon_vma_chain *avc, *next;
> -	struct anon_vma *root = NULL;
> +	struct anon_vma *active_anon_vma = vma->anon_vma;
>  
>  	/* Always hold mmap lock, read-lock on unmap possibly. */
>  	mmap_assert_locked(vma->vm_mm);
>  
>  	/* Unfaulted is a no-op. */
> -	if (!vma->anon_vma) {
> +	if (!active_anon_vma) {
>  		VM_WARN_ON_ONCE(!list_empty(&vma->anon_vma_chain));
>  		return;
>  	}
>  
> +	anon_vma_lock_write(active_anon_vma);
> +
>  	/*
>  	 * Unlink each anon_vma chained to the VMA.  This list is ordered
>  	 * from newest to oldest, ensuring the root anon_vma gets freed last.
> @@ -489,7 +468,6 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
>  	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
>  		struct anon_vma *anon_vma = avc->anon_vma;
>  
> -		root = lock_anon_vma_root(root, anon_vma);
>  		anon_vma_interval_tree_remove(avc, &anon_vma->rb_root);
>  
>  		/*
> @@ -505,13 +483,14 @@ void unlink_anon_vmas(struct vm_area_struct *vma)
>  		anon_vma_chain_free(avc);
>  	}
>  
> -	vma->anon_vma->num_active_vmas--;
> +	active_anon_vma->num_active_vmas--;
>  	/*
>  	 * vma would still be needed after unlink, and anon_vma will be prepared
>  	 * when handle fault.
>  	 */
>  	vma->anon_vma = NULL;
> -	unlock_anon_vma_root(root);
> +	anon_vma_unlock_write(active_anon_vma);
> +

nit: extra whitespace here.

>  
>  	/*
>  	 * Iterate the list once more, it now only contains empty and unlinked
> -- 
> 2.52.0
> 

Re: [PATCH v2 4/8] mm/rmap: remove anon_vma_merge() function

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> This function is confusing, we already have the concept of anon_vma merge
> to adjacent VMA's anon_vma's to increase probability of anon_vma
> compatibility and therefore VMA merge (see is_mergeable_anon_vma() etc.),
> as well as anon_vma reuse, along side the usual VMA merge logic.
> 
> We can remove the anon_vma check as it is redundant - a merge would not
> have been permitted with removal if the anon_vma's were not the same (and
> in the case of an unfaulted/faulted merge, we would have already set the
> unfaulted VMA's anon_vma to vp->remove->anon_vma in dup_anon_vma()).
> 
> Avoid overloading this term when we're very simply unlinking anon_vma state
> from a removed VMA upon merge.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  include/linux/rmap.h             | 7 -------
>  mm/vma.c                         | 2 +-
>  tools/testing/vma/vma_internal.h | 5 -----
>  3 files changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index daa92a58585d..832bfc0ccfc6 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -165,13 +165,6 @@ static inline int anon_vma_prepare(struct vm_area_struct *vma)
>  	return __anon_vma_prepare(vma);
>  }
>  
> -static inline void anon_vma_merge(struct vm_area_struct *vma,
> -				  struct vm_area_struct *next)
> -{
> -	VM_BUG_ON_VMA(vma->anon_vma != next->anon_vma, vma);
> -	unlink_anon_vmas(next);
> -}
> -
>  struct anon_vma *folio_get_anon_vma(const struct folio *folio);
>  
>  #ifdef CONFIG_MM_ID
> diff --git a/mm/vma.c b/mm/vma.c
> index fb45a6be7417..4294ecdc23a5 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -379,7 +379,7 @@ static void vma_complete(struct vma_prepare *vp, struct vma_iterator *vmi,
>  			fput(vp->file);
>  		}
>  		if (vp->remove->anon_vma)
> -			anon_vma_merge(vp->vma, vp->remove);
> +			unlink_anon_vmas(vp->remove);
>  		mm->map_count--;
>  		mpol_put(vma_policy(vp->remove));
>  		if (!vp->remove2)
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 9f0a9f5ed0fe..93e5792306d9 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -1265,11 +1265,6 @@ static inline void i_mmap_unlock_write(struct address_space *mapping)
>  {
>  }
>  
> -static inline void anon_vma_merge(struct vm_area_struct *vma,
> -				  struct vm_area_struct *next)
> -{
> -}
> -
>  static inline int userfaultfd_unmap_prep(struct vm_area_struct *vma,
>  					 unsigned long start,
>  					 unsigned long end,
> -- 
> 2.52.0
> 

Re: [PATCH v2 5/8] mm/rmap: make anon_vma functions internal

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> The bulk of the anon_vma operations are only used by mm, so formalise this
> by putting the function prototypes and inlines in mm/internal.h. This
> allows us to make changes without having to worry about the rest of the
> kernel.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

nit, One extra space in this one too

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  include/linux/rmap.h | 60 --------------------------------------------
>  mm/internal.h        | 58 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 58 insertions(+), 60 deletions(-)
> 
> diff --git a/include/linux/rmap.h b/include/linux/rmap.h
> index 832bfc0ccfc6..dd764951b03d 100644
> --- a/include/linux/rmap.h
> +++ b/include/linux/rmap.h
> @@ -104,68 +104,8 @@ enum ttu_flags {
>  };
>  
>  #ifdef CONFIG_MMU
> -static inline void get_anon_vma(struct anon_vma *anon_vma)
> -{
> -	atomic_inc(&anon_vma->refcount);
> -}
> -
> -void __put_anon_vma(struct anon_vma *anon_vma);
> -
> -static inline void put_anon_vma(struct anon_vma *anon_vma)
> -{
> -	if (atomic_dec_and_test(&anon_vma->refcount))
> -		__put_anon_vma(anon_vma);
> -}
> -
> -static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
> -{
> -	down_write(&anon_vma->root->rwsem);
> -}
>  
> -static inline int anon_vma_trylock_write(struct anon_vma *anon_vma)
> -{
> -	return down_write_trylock(&anon_vma->root->rwsem);
> -}
> -
> -static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
> -{
> -	up_write(&anon_vma->root->rwsem);
> -}
> -
> -static inline void anon_vma_lock_read(struct anon_vma *anon_vma)
> -{
> -	down_read(&anon_vma->root->rwsem);
> -}
> -
> -static inline int anon_vma_trylock_read(struct anon_vma *anon_vma)
> -{
> -	return down_read_trylock(&anon_vma->root->rwsem);
> -}
> -
> -static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
> -{
> -	up_read(&anon_vma->root->rwsem);
> -}
> -
> -
> -/*
> - * anon_vma helper functions.
> - */
>  void anon_vma_init(void);	/* create anon_vma_cachep */
> -int  __anon_vma_prepare(struct vm_area_struct *);
> -void unlink_anon_vmas(struct vm_area_struct *);
> -int anon_vma_clone(struct vm_area_struct *, struct vm_area_struct *);
> -int anon_vma_fork(struct vm_area_struct *, struct vm_area_struct *);
> -
> -static inline int anon_vma_prepare(struct vm_area_struct *vma)
> -{
> -	if (likely(vma->anon_vma))
> -		return 0;
> -
> -	return __anon_vma_prepare(vma);
> -}
> -
> -struct anon_vma *folio_get_anon_vma(const struct folio *folio);
>  
>  #ifdef CONFIG_MM_ID
>  static __always_inline void folio_lock_large_mapcount(struct folio *folio)
> diff --git a/mm/internal.h b/mm/internal.h
> index 8319a91e363b..4ba784023a9f 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -199,6 +199,64 @@ static inline void vma_close(struct vm_area_struct *vma)
>  
>  #ifdef CONFIG_MMU
>  
> +static inline void get_anon_vma(struct anon_vma *anon_vma)
> +{
> +	atomic_inc(&anon_vma->refcount);
> +}
> +
> +void __put_anon_vma(struct anon_vma *anon_vma);
> +
> +static inline void put_anon_vma(struct anon_vma *anon_vma)
> +{
> +	if (atomic_dec_and_test(&anon_vma->refcount))
> +		__put_anon_vma(anon_vma);
> +}
> +
> +static inline void anon_vma_lock_write(struct anon_vma *anon_vma)
> +{
> +	down_write(&anon_vma->root->rwsem);
> +}
> +
> +static inline int anon_vma_trylock_write(struct anon_vma *anon_vma)
> +{
> +	return down_write_trylock(&anon_vma->root->rwsem);
> +}
> +
> +static inline void anon_vma_unlock_write(struct anon_vma *anon_vma)
> +{
> +	up_write(&anon_vma->root->rwsem);
> +}
> +
> +static inline void anon_vma_lock_read(struct anon_vma *anon_vma)
> +{
> +	down_read(&anon_vma->root->rwsem);
> +}
> +
> +static inline int anon_vma_trylock_read(struct anon_vma *anon_vma)
> +{
> +	return down_read_trylock(&anon_vma->root->rwsem);
> +}
> +
> +static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
> +{
> +	up_read(&anon_vma->root->rwsem);
> +}
> +
> +struct anon_vma *folio_get_anon_vma(const struct folio *folio);
> +
> +int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src);
> +int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma);
> +int  __anon_vma_prepare(struct vm_area_struct *vma);
> +void unlink_anon_vmas(struct vm_area_struct *vma);
> +
> +static inline int anon_vma_prepare(struct vm_area_struct *vma)
> +{
> +	if (likely(vma->anon_vma))
> +		return 0;
> +
> +	return __anon_vma_prepare(vma);
> +}
> +
>  /* Flags for folio_pte_batch(). */
>  typedef int __bitwise fpb_t;
>  
> -- 
> 2.52.0
> 

Re: [PATCH v2 6/8] mm/mmap_lock: add vma_is_attached() helper

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> This makes it easy to explicitly check for VMA detachment, which is useful
> for things like asserts.
> 
> Note that we intentionally do not allow this function to be available
> should CONFIG_PER_VMA_LOCK be set - this is because vma_assert_attached()
> and vma_assert_detached() are no-ops if !CONFIG_PER_VMA_LOCK, so there is
> no correct state for vma_is_attached() to be in if this configuration
> option is not specified.
> 
> Therefore users elsewhere must invoke this function only after checking for
> CONFIG_PER_VMA_LOCK.
> 
> We rework the assert functions to utilise this.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  include/linux/mmap_lock.h | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index d53f72dba7fe..b50416fbba20 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -251,6 +251,11 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
>  		      !__is_vma_write_locked(vma, &mm_lock_seq), vma);
>  }
>  
> +static inline bool vma_is_attached(struct vm_area_struct *vma)
> +{
> +	return refcount_read(&vma->vm_refcnt);
> +}
> +
>  /*
>   * WARNING: to avoid racing with vma_mark_attached()/vma_mark_detached(), these
>   * assertions should be made either under mmap_write_lock or when the object
> @@ -258,12 +263,12 @@ static inline void vma_assert_locked(struct vm_area_struct *vma)
>   */
>  static inline void vma_assert_attached(struct vm_area_struct *vma)
>  {
> -	WARN_ON_ONCE(!refcount_read(&vma->vm_refcnt));
> +	WARN_ON_ONCE(!vma_is_attached(vma));
>  }
>  
>  static inline void vma_assert_detached(struct vm_area_struct *vma)
>  {
> -	WARN_ON_ONCE(refcount_read(&vma->vm_refcnt));
> +	WARN_ON_ONCE(vma_is_attached(vma));
>  }
>  
>  static inline void vma_mark_attached(struct vm_area_struct *vma)
> -- 
> 2.52.0
> 

Re: [PATCH v2 7/8] mm/rmap: allocate anon_vma_chain objects unlocked when possible

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:04]:
> There is no reason to allocate the anon_vma_chain under the anon_vma write
> lock when cloning - we can in fact assign these to the destination VMA
> safely as we hold the exclusive mmap lock and therefore preclude anybody
> else accessing these fields.
> 
> We only need take the anon_vma write lock when we link rbtree edges from
> the anon_vma to the newly established AVCs.
> 
> This also allows us to eliminate the weird GFP_NOWAIT, GFP_KERNEL dance
> introduced in commit dd34739c03f2 ("mm: avoid anon_vma_chain allocation
> under anon_vma lock"), further simplifying this logic.
> 
> This should reduce lock anon_vma contention, and clarifies exactly where
> the anon_vma lock is required.
> 
> We cannot adjust __anon_vma_prepare() in the same way as this is only
> protected by VMA read lock, so we have to perform the allocation here under
> the anon_vma write lock and page_table_lock (to protect against racing
> threads), and we wish to retain the lock ordering.
> 
> With this change we can simplify cleanup_partial_anon_vmas() even further -
> since we allocate AVC's without any lock taken and do not insert anything
> into the interval tree until after the allocations are tried, we can remove
> all logic pertaining to this and just free up AVC's only.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> Reviewed-by: Suren Baghdasaryan <surenb@google.com>

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/rmap.c | 78 +++++++++++++++++++++++++------------------------------
>  1 file changed, 35 insertions(+), 43 deletions(-)
> 
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 6ac42671bedd..8f4393546bce 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -147,14 +147,13 @@ static void anon_vma_chain_free(struct anon_vma_chain *anon_vma_chain)
>  	kmem_cache_free(anon_vma_chain_cachep, anon_vma_chain);
>  }
>  
> -static void anon_vma_chain_link(struct vm_area_struct *vma,
> -				struct anon_vma_chain *avc,
> -				struct anon_vma *anon_vma)
> +static void anon_vma_chain_assign(struct vm_area_struct *vma,
> +				  struct anon_vma_chain *avc,
> +				  struct anon_vma *anon_vma)
>  {
>  	avc->vma = vma;
>  	avc->anon_vma = anon_vma;
>  	list_add(&avc->same_vma, &vma->anon_vma_chain);
> -	anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
>  }
>  
>  /**
> @@ -211,7 +210,8 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>  	spin_lock(&mm->page_table_lock);
>  	if (likely(!vma->anon_vma)) {
>  		vma->anon_vma = anon_vma;
> -		anon_vma_chain_link(vma, avc, anon_vma);
> +		anon_vma_chain_assign(vma, avc, anon_vma);
> +		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
>  		anon_vma->num_active_vmas++;
>  		allocated = NULL;
>  		avc = NULL;
> @@ -292,21 +292,31 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  
>  	check_anon_vma_clone(dst, src);
>  
> -	/* All anon_vma's share the same root. */
> +	/*
> +	 * Allocate AVCs. We don't need an anon_vma lock for this as we
> +	 * are not updating the anon_vma rbtree nor are we changing
> +	 * anon_vma statistics.
> +	 *
> +	 * We hold the exclusive mmap write lock so there's no possibliity of
> +	 * the unlinked AVC's being observed yet.
> +	 */
> +	list_for_each_entry(pavc, &src->anon_vma_chain, same_vma) {
> +		avc = anon_vma_chain_alloc(GFP_KERNEL);
> +		if (!avc)
> +			goto enomem_failure;
> +
> +		anon_vma_chain_assign(dst, avc, pavc->anon_vma);
> +	}
> +
> +	/*
> +	 * Now link the anon_vma's back to the newly inserted AVCs.
> +	 * Note that all anon_vma's share the same root.
> +	 */
>  	anon_vma_lock_write(src->anon_vma);
> -	list_for_each_entry_reverse(pavc, &src->anon_vma_chain, same_vma) {
> -		struct anon_vma *anon_vma;
> -
> -		avc = anon_vma_chain_alloc(GFP_NOWAIT);
> -		if (unlikely(!avc)) {
> -			anon_vma_unlock_write(src->anon_vma);
> -			avc = anon_vma_chain_alloc(GFP_KERNEL);
> -			if (!avc)
> -				goto enomem_failure;
> -			anon_vma_lock_write(src->anon_vma);
> -		}
> -		anon_vma = pavc->anon_vma;
> -		anon_vma_chain_link(dst, avc, anon_vma);
> +	list_for_each_entry_reverse(avc, &dst->anon_vma_chain, same_vma) {
> +		struct anon_vma *anon_vma = avc->anon_vma;
> +
> +		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
>  
>  		/*
>  		 * Reuse existing anon_vma if it has no vma and only one
> @@ -322,7 +332,6 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  	}
>  	if (dst->anon_vma)
>  		dst->anon_vma->num_active_vmas++;
> -
>  	anon_vma_unlock_write(src->anon_vma);
>  	return 0;
>  
> @@ -384,8 +393,10 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
>  	get_anon_vma(anon_vma->root);
>  	/* Mark this anon_vma as the one where our new (COWed) pages go. */
>  	vma->anon_vma = anon_vma;
> +	anon_vma_chain_assign(vma, avc, anon_vma);
> +	/* Now let rmap see it. */
>  	anon_vma_lock_write(anon_vma);
> -	anon_vma_chain_link(vma, avc, anon_vma);
> +	anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
>  	anon_vma->parent->num_children++;
>  	anon_vma_unlock_write(anon_vma);
>  
> @@ -402,34 +413,15 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
>   * In the unfortunate case of anon_vma_clone() failing to allocate memory we
>   * have to clean things up.
>   *
> - * On clone we hold the exclusive mmap write lock, so we can't race
> - * unlink_anon_vmas(). Since we're cloning, we know we can't have empty
> - * anon_vma's, since existing anon_vma's are what we're cloning from.
> - *
> - * So this function needs only traverse the anon_vma_chain and free each
> - * allocated anon_vma_chain.
> + * Since we allocate anon_vma_chain's before we insert them into the interval
> + * trees, we simply have to free up the AVC's and remove the entries from the
> + * VMA's anon_vma_chain.
>   */
>  static void cleanup_partial_anon_vmas(struct vm_area_struct *vma)
>  {
>  	struct anon_vma_chain *avc, *next;
> -	bool locked = false;
> -
> -	/*
> -	 * We exclude everybody else from being able to modify anon_vma's
> -	 * underneath us.
> -	 */
> -	mmap_assert_locked(vma->vm_mm);
>  
>  	list_for_each_entry_safe(avc, next, &vma->anon_vma_chain, same_vma) {
> -		struct anon_vma *anon_vma = avc->anon_vma;
> -
> -		/* All anon_vma's share the same root. */
> -		if (!locked) {
> -			anon_vma_lock_write(anon_vma);
> -			locked = true;
> -		}
> -
> -		anon_vma_interval_tree_remove(avc, &anon_vma->rb_root);
>  		list_del(&avc->same_vma);
>  		anon_vma_chain_free(avc);
>  	}
> -- 
> 2.52.0
> 

Re: [PATCH v2 8/8] mm/rmap: separate out fork-only logic on anon_vma_clone()

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:05]:
> Specify which operation is being performed to anon_vma_clone(), which
> allows us to do checks specific to each operation type, as well as to
> separate out and make clear that the anon_vma reuse logic is absolutely
> specific to fork only.
> 
> This opens the door to further refactorings and refinements later as we
> have more information to work with.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

A few minor things, but this looks correct.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> ---
>  mm/internal.h                    | 11 ++++-
>  mm/rmap.c                        | 74 ++++++++++++++++++++++----------
>  mm/vma.c                         |  6 +--
>  tools/testing/vma/vma_internal.h | 11 ++++-
>  4 files changed, 74 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/internal.h b/mm/internal.h
> index 4ba784023a9f..8baa7bd2b8f7 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -244,7 +244,16 @@ static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
>  
>  struct anon_vma *folio_get_anon_vma(const struct folio *folio);
>  
> -int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src);
> +/* Operations which modify VMAs. */
> +enum vma_operation {
> +	VMA_OP_SPLIT,
> +	VMA_OP_MERGE_UNFAULTED,
> +	VMA_OP_REMAP,
> +	VMA_OP_FORK,
> +};
> +
> +int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> +	enum vma_operation operation);
>  int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma);
>  int  __anon_vma_prepare(struct vm_area_struct *vma);
>  void unlink_anon_vmas(struct vm_area_struct *vma);
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 8f4393546bce..336b27e00238 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -233,12 +233,13 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
>  }
>  
>  static void check_anon_vma_clone(struct vm_area_struct *dst,
> -				 struct vm_area_struct *src)
> +				 struct vm_area_struct *src,
> +				 enum vma_operation operation)

You could save a line here by putting src and operation on the same line
and tabbing only twice, but sure.  This is true in earlier patches as
well.

>  {
>  	/* The write lock must be held. */
>  	mmap_assert_write_locked(src->vm_mm);
> -	/* If not a fork (implied by dst->anon_vma) then must be on same mm. */
> -	VM_WARN_ON_ONCE(dst->anon_vma && dst->vm_mm != src->vm_mm);
> +	/* If not a fork then must be on same mm. */
> +	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && dst->vm_mm != src->vm_mm);
>  
>  	/* If we have anything to do src->anon_vma must be provided. */
>  	VM_WARN_ON_ONCE(!src->anon_vma && !list_empty(&src->anon_vma_chain));
> @@ -250,6 +251,40 @@ static void check_anon_vma_clone(struct vm_area_struct *dst,
>  	 * must be the same across dst and src.
>  	 */
>  	VM_WARN_ON_ONCE(dst->anon_vma && dst->anon_vma != src->anon_vma);
> +	/*
> +	 * Essentially equivalent to above - if not a no-op, we should expect
> +	 * dst->anon_vma to be set for everything except a fork.
> +	 */
> +	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && src->anon_vma &&
> +			!dst->anon_vma);
> +	/* For the anon_vma to be compatible, it can only be singular. */
> +	VM_WARN_ON_ONCE(operation == VMA_OP_MERGE_UNFAULTED &&
> +			!list_is_singular(&src->anon_vma_chain));
> +#ifdef CONFIG_PER_VMA_LOCK
> +	/* Only merging an unfaulted VMA leaves the destination attached. */
> +	VM_WARN_ON_ONCE(operation != VMA_OP_MERGE_UNFAULTED &&
> +			vma_is_attached(dst));
> +#endif
> +}
> +

try seems to imply we can return something saying it didn't work out,
but this is void.  Naming is hard.  reuse_anon_vma_if_necessary seems
insane, so I don't really have anything better.

> +static void try_to_reuse_anon_vma(struct vm_area_struct *dst,
> +				  struct anon_vma *anon_vma)
> +{
> +	/* If already populated, nothing to do.*/
> +	if (dst->anon_vma)
> +		return;

This is only used on VMA_OP_FORK, how is it populated?

I assume this is a later refinement?

> +
> +	/*
> +	 * We reuse an anon_vma if any linking VMAs were unmapped and it has
> +	 * only a single child at most.
> +	 */
> +	if (anon_vma->num_active_vmas > 0)
> +		return;
> +	if (anon_vma->num_children > 1)
> +		return;
> +
> +	dst->anon_vma = anon_vma;
> +	anon_vma->num_active_vmas++;
>  }
>  
>  static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
> @@ -259,6 +294,7 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
>   * all of the anon_vma objects contained within @src anon_vma_chain's.
>   * @dst: The destination VMA with an empty anon_vma_chain.
>   * @src: The source VMA we wish to duplicate.
> + * @operation: The type of operation which resulted in the clone.
>   *
>   * This is the heart of the VMA side of the anon_vma implementation - we invoke
>   * this function whenever we need to set up a new VMA's anon_vma state.
> @@ -281,17 +317,17 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
>   *
>   * Returns: 0 on success, -ENOMEM on failure.
>   */
> -int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
> +int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> +		   enum vma_operation operation)
>  {
>  	struct anon_vma_chain *avc, *pavc;
> +	struct anon_vma *active_anon_vma = src->anon_vma;
>  
> -	check_anon_vma_clone(dst, src);
> +	check_anon_vma_clone(dst, src, operation);
>  
> -	if (!src->anon_vma)
> +	if (!active_anon_vma)
>  		return 0;
>  
> -	check_anon_vma_clone(dst, src);
> -
>  	/*
>  	 * Allocate AVCs. We don't need an anon_vma lock for this as we
>  	 * are not updating the anon_vma rbtree nor are we changing
> @@ -317,22 +353,14 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
>  		struct anon_vma *anon_vma = avc->anon_vma;
>  
>  		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
> -
> -		/*
> -		 * Reuse existing anon_vma if it has no vma and only one
> -		 * anon_vma child.
> -		 *
> -		 * Root anon_vma is never reused:
> -		 * it has self-parent reference and at least one child.
> -		 */
> -		if (!dst->anon_vma && src->anon_vma &&
> -		    anon_vma->num_children < 2 &&
> -		    anon_vma->num_active_vmas == 0)
> -			dst->anon_vma = anon_vma;
> +		if (operation == VMA_OP_FORK)
> +			try_to_reuse_anon_vma(dst, anon_vma);
>  	}
> -	if (dst->anon_vma)
> +
> +	if (operation != VMA_OP_FORK)
>  		dst->anon_vma->num_active_vmas++;
> -	anon_vma_unlock_write(src->anon_vma);
> +
> +	anon_vma_unlock_write(active_anon_vma);
>  	return 0;
>  
>   enomem_failure:
> @@ -362,7 +390,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
>  	 * First, attach the new VMA to the parent VMA's anon_vmas,
>  	 * so rmap can find non-COWed pages in child processes.
>  	 */
> -	error = anon_vma_clone(vma, pvma);
> +	error = anon_vma_clone(vma, pvma, VMA_OP_FORK);
>  	if (error)
>  		return error;
>  
> diff --git a/mm/vma.c b/mm/vma.c
> index 4294ecdc23a5..2a063d6568d9 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -528,7 +528,7 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	if (err)
>  		goto out_free_vmi;
>  
> -	err = anon_vma_clone(new, vma);
> +	err = anon_vma_clone(new, vma, VMA_OP_SPLIT);
>  	if (err)
>  		goto out_free_mpol;
>  
> @@ -626,7 +626,7 @@ static int dup_anon_vma(struct vm_area_struct *dst,
>  
>  		vma_assert_write_locked(dst);
>  		dst->anon_vma = src->anon_vma;
> -		ret = anon_vma_clone(dst, src);
> +		ret = anon_vma_clone(dst, src, VMA_OP_MERGE_UNFAULTED);
>  		if (ret)
>  			return ret;
>  
> @@ -1899,7 +1899,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>  		vma_set_range(new_vma, addr, addr + len, pgoff);
>  		if (vma_dup_policy(vma, new_vma))
>  			goto out_free_vma;
> -		if (anon_vma_clone(new_vma, vma))
> +		if (anon_vma_clone(new_vma, vma, VMA_OP_REMAP))
>  			goto out_free_mempol;
>  		if (new_vma->vm_file)
>  			get_file(new_vma->vm_file);
> diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> index 93e5792306d9..7fa56dcc53a6 100644
> --- a/tools/testing/vma/vma_internal.h
> +++ b/tools/testing/vma/vma_internal.h
> @@ -600,6 +600,14 @@ struct mmap_action {
>  	bool hide_from_rmap_until_complete :1;
>  };
>  
> +/* Operations which modify VMAs. */
> +enum vma_operation {
> +	VMA_OP_SPLIT,
> +	VMA_OP_MERGE_UNFAULTED,
> +	VMA_OP_REMAP,
> +	VMA_OP_FORK,
> +};
> +
>  /*
>   * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
>   * manipulate mutable fields which will cause those fields to be updated in the
> @@ -1157,7 +1165,8 @@ static inline int vma_dup_policy(struct vm_area_struct *src, struct vm_area_stru
>  	return 0;
>  }
>  
> -static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
> +static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> +				 enum vma_operation operation)
>  {
>  	/* For testing purposes. We indicate that an anon_vma has been cloned. */
>  	if (src->anon_vma != NULL) {
> -- 
> 2.52.0
> 

Re: [PATCH v2 8/8] mm/rmap: separate out fork-only logic on anon_vma_clone()

[Lorenzo Stoakes]

On Tue, Jan 06, 2026 at 02:27:37PM -0500, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@oracle.com> [260106 10:05]:
> > Specify which operation is being performed to anon_vma_clone(), which
> > allows us to do checks specific to each operation type, as well as to
> > separate out and make clear that the anon_vma reuse logic is absolutely
> > specific to fork only.
> >
> > This opens the door to further refactorings and refinements later as we
> > have more information to work with.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> A few minor things, but this looks correct.
>
> Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>
>
> > ---
> >  mm/internal.h                    | 11 ++++-
> >  mm/rmap.c                        | 74 ++++++++++++++++++++++----------
> >  mm/vma.c                         |  6 +--
> >  tools/testing/vma/vma_internal.h | 11 ++++-
> >  4 files changed, 74 insertions(+), 28 deletions(-)
> >
> > diff --git a/mm/internal.h b/mm/internal.h
> > index 4ba784023a9f..8baa7bd2b8f7 100644
> > --- a/mm/internal.h
> > +++ b/mm/internal.h
> > @@ -244,7 +244,16 @@ static inline void anon_vma_unlock_read(struct anon_vma *anon_vma)
> >
> >  struct anon_vma *folio_get_anon_vma(const struct folio *folio);
> >
> > -int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src);
> > +/* Operations which modify VMAs. */
> > +enum vma_operation {
> > +	VMA_OP_SPLIT,
> > +	VMA_OP_MERGE_UNFAULTED,
> > +	VMA_OP_REMAP,
> > +	VMA_OP_FORK,
> > +};
> > +
> > +int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> > +	enum vma_operation operation);
> >  int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma);
> >  int  __anon_vma_prepare(struct vm_area_struct *vma);
> >  void unlink_anon_vmas(struct vm_area_struct *vma);
> > diff --git a/mm/rmap.c b/mm/rmap.c
> > index 8f4393546bce..336b27e00238 100644
> > --- a/mm/rmap.c
> > +++ b/mm/rmap.c
> > @@ -233,12 +233,13 @@ int __anon_vma_prepare(struct vm_area_struct *vma)
> >  }
> >
> >  static void check_anon_vma_clone(struct vm_area_struct *dst,
> > -				 struct vm_area_struct *src)
> > +				 struct vm_area_struct *src,
> > +				 enum vma_operation operation)
>
> You could save a line here by putting src and operation on the same line
> and tabbing only twice, but sure.  This is true in earlier patches as
> well.
>
> >  {
> >  	/* The write lock must be held. */
> >  	mmap_assert_write_locked(src->vm_mm);
> > -	/* If not a fork (implied by dst->anon_vma) then must be on same mm. */
> > -	VM_WARN_ON_ONCE(dst->anon_vma && dst->vm_mm != src->vm_mm);
> > +	/* If not a fork then must be on same mm. */
> > +	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && dst->vm_mm != src->vm_mm);
> >
> >  	/* If we have anything to do src->anon_vma must be provided. */
> >  	VM_WARN_ON_ONCE(!src->anon_vma && !list_empty(&src->anon_vma_chain));
> > @@ -250,6 +251,40 @@ static void check_anon_vma_clone(struct vm_area_struct *dst,
> >  	 * must be the same across dst and src.
> >  	 */
> >  	VM_WARN_ON_ONCE(dst->anon_vma && dst->anon_vma != src->anon_vma);
> > +	/*
> > +	 * Essentially equivalent to above - if not a no-op, we should expect
> > +	 * dst->anon_vma to be set for everything except a fork.
> > +	 */
> > +	VM_WARN_ON_ONCE(operation != VMA_OP_FORK && src->anon_vma &&
> > +			!dst->anon_vma);
> > +	/* For the anon_vma to be compatible, it can only be singular. */
> > +	VM_WARN_ON_ONCE(operation == VMA_OP_MERGE_UNFAULTED &&
> > +			!list_is_singular(&src->anon_vma_chain));
> > +#ifdef CONFIG_PER_VMA_LOCK
> > +	/* Only merging an unfaulted VMA leaves the destination attached. */
> > +	VM_WARN_ON_ONCE(operation != VMA_OP_MERGE_UNFAULTED &&
> > +			vma_is_attached(dst));
> > +#endif
> > +}
> > +
>
> try seems to imply we can return something saying it didn't work out,
> but this is void.  Naming is hard.  reuse_anon_vma_if_necessary seems
> insane, so I don't really have anything better.

As discussed on irc, this is probably better as maybe_reuse_anon_vma().

Will send a fix-patch.

>
> > +static void try_to_reuse_anon_vma(struct vm_area_struct *dst,
> > +				  struct anon_vma *anon_vma)
> > +{
> > +	/* If already populated, nothing to do.*/
> > +	if (dst->anon_vma)
> > +		return;
>
> This is only used on VMA_OP_FORK, how is it populated?

Below,

See:

+	dst->anon_vma = anon_vma;

So this just means we use the first anon_vma we encounter in the hierarchy and
don't waste time trying to find another.

>
> I assume this is a later refinement?
>
> > +
> > +	/*
> > +	 * We reuse an anon_vma if any linking VMAs were unmapped and it has
> > +	 * only a single child at most.
> > +	 */
> > +	if (anon_vma->num_active_vmas > 0)
> > +		return;
> > +	if (anon_vma->num_children > 1)
> > +		return;
> > +
> > +	dst->anon_vma = anon_vma;
> > +	anon_vma->num_active_vmas++;
> >  }
> >
> >  static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
> > @@ -259,6 +294,7 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
> >   * all of the anon_vma objects contained within @src anon_vma_chain's.
> >   * @dst: The destination VMA with an empty anon_vma_chain.
> >   * @src: The source VMA we wish to duplicate.
> > + * @operation: The type of operation which resulted in the clone.
> >   *
> >   * This is the heart of the VMA side of the anon_vma implementation - we invoke
> >   * this function whenever we need to set up a new VMA's anon_vma state.
> > @@ -281,17 +317,17 @@ static void cleanup_partial_anon_vmas(struct vm_area_struct *vma);
> >   *
> >   * Returns: 0 on success, -ENOMEM on failure.
> >   */
> > -int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
> > +int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> > +		   enum vma_operation operation)
> >  {
> >  	struct anon_vma_chain *avc, *pavc;
> > +	struct anon_vma *active_anon_vma = src->anon_vma;
> >
> > -	check_anon_vma_clone(dst, src);
> > +	check_anon_vma_clone(dst, src, operation);
> >
> > -	if (!src->anon_vma)
> > +	if (!active_anon_vma)
> >  		return 0;
> >
> > -	check_anon_vma_clone(dst, src);
> > -
> >  	/*
> >  	 * Allocate AVCs. We don't need an anon_vma lock for this as we
> >  	 * are not updating the anon_vma rbtree nor are we changing
> > @@ -317,22 +353,14 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
> >  		struct anon_vma *anon_vma = avc->anon_vma;
> >
> >  		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
> > -
> > -		/*
> > -		 * Reuse existing anon_vma if it has no vma and only one
> > -		 * anon_vma child.
> > -		 *
> > -		 * Root anon_vma is never reused:
> > -		 * it has self-parent reference and at least one child.
> > -		 */
> > -		if (!dst->anon_vma && src->anon_vma &&
> > -		    anon_vma->num_children < 2 &&
> > -		    anon_vma->num_active_vmas == 0)
> > -			dst->anon_vma = anon_vma;
> > +		if (operation == VMA_OP_FORK)
> > +			try_to_reuse_anon_vma(dst, anon_vma);
> >  	}
> > -	if (dst->anon_vma)
> > +
> > +	if (operation != VMA_OP_FORK)
> >  		dst->anon_vma->num_active_vmas++;
> > -	anon_vma_unlock_write(src->anon_vma);
> > +
> > +	anon_vma_unlock_write(active_anon_vma);
> >  	return 0;
> >
> >   enomem_failure:
> > @@ -362,7 +390,7 @@ int anon_vma_fork(struct vm_area_struct *vma, struct vm_area_struct *pvma)
> >  	 * First, attach the new VMA to the parent VMA's anon_vmas,
> >  	 * so rmap can find non-COWed pages in child processes.
> >  	 */
> > -	error = anon_vma_clone(vma, pvma);
> > +	error = anon_vma_clone(vma, pvma, VMA_OP_FORK);
> >  	if (error)
> >  		return error;
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index 4294ecdc23a5..2a063d6568d9 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -528,7 +528,7 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
> >  	if (err)
> >  		goto out_free_vmi;
> >
> > -	err = anon_vma_clone(new, vma);
> > +	err = anon_vma_clone(new, vma, VMA_OP_SPLIT);
> >  	if (err)
> >  		goto out_free_mpol;
> >
> > @@ -626,7 +626,7 @@ static int dup_anon_vma(struct vm_area_struct *dst,
> >
> >  		vma_assert_write_locked(dst);
> >  		dst->anon_vma = src->anon_vma;
> > -		ret = anon_vma_clone(dst, src);
> > +		ret = anon_vma_clone(dst, src, VMA_OP_MERGE_UNFAULTED);
> >  		if (ret)
> >  			return ret;
> >
> > @@ -1899,7 +1899,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >  		vma_set_range(new_vma, addr, addr + len, pgoff);
> >  		if (vma_dup_policy(vma, new_vma))
> >  			goto out_free_vma;
> > -		if (anon_vma_clone(new_vma, vma))
> > +		if (anon_vma_clone(new_vma, vma, VMA_OP_REMAP))
> >  			goto out_free_mempol;
> >  		if (new_vma->vm_file)
> >  			get_file(new_vma->vm_file);
> > diff --git a/tools/testing/vma/vma_internal.h b/tools/testing/vma/vma_internal.h
> > index 93e5792306d9..7fa56dcc53a6 100644
> > --- a/tools/testing/vma/vma_internal.h
> > +++ b/tools/testing/vma/vma_internal.h
> > @@ -600,6 +600,14 @@ struct mmap_action {
> >  	bool hide_from_rmap_until_complete :1;
> >  };
> >
> > +/* Operations which modify VMAs. */
> > +enum vma_operation {
> > +	VMA_OP_SPLIT,
> > +	VMA_OP_MERGE_UNFAULTED,
> > +	VMA_OP_REMAP,
> > +	VMA_OP_FORK,
> > +};
> > +
> >  /*
> >   * Describes a VMA that is about to be mmap()'ed. Drivers may choose to
> >   * manipulate mutable fields which will cause those fields to be updated in the
> > @@ -1157,7 +1165,8 @@ static inline int vma_dup_policy(struct vm_area_struct *src, struct vm_area_stru
> >  	return 0;
> >  }
> >
> > -static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
> > +static inline int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,
> > +				 enum vma_operation operation)
> >  {
> >  	/* For testing purposes. We indicate that an anon_vma has been cloned. */
> >  	if (src->anon_vma != NULL) {
> > --
> > 2.52.0
> >

Re: [PATCH v2 7/8] mm/rmap: allocate anon_vma_chain objects unlocked when possible

[Lorenzo Stoakes]

Hi Andrew,

Please apply the below fix-patch which addresses the issue raised by Suren
regarding the veracity of a comment around locks held.

Thanks, Lorenzo

----8<----
From dace24fb96c7fbf812e6cca2fb4aa0b6489ce8e3 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 8 Jan 2026 18:49:07 +0000
Subject: [PATCH] fix comment

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/rmap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index 8f4393546bce..576c7cde6464 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -297,8 +297,10 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src)
 	* are not updating the anon_vma rbtree nor are we changing
 	* anon_vma statistics.
 	*
-	* We hold the exclusive mmap write lock so there's no possibliity of
-	* the unlinked AVC's being observed yet.
+	* Either src, dst have the same mm for which we hold an exclusive mmap
+	* write lock, or we are forking and we hold it on src->vm_mm and dst is
+	* not yet accessible to other threads so there's no possibliity of the
+	* unlinked AVC's being observed yet.
 	*/
 	list_for_each_entry(pavc, &src->anon_vma_chain, same_vma) {
 		avc = anon_vma_chain_alloc(GFP_KERNEL);
--
2.52.0

Re: [PATCH v2 8/8] mm/rmap: separate out fork-only logic on anon_vma_clone()

[Lorenzo Stoakes]

Hi Andrew,

Please apply the below fix-patch to rename try_to_reuse_anon_vma() to
maybe_reuse_anon_vma() to address Liam's concern about this naming.

Thanks, Lorenzo

----8<----
From c2bd360718da23c62a4aaa0addfa48955b2007f9 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
Date: Thu, 8 Jan 2026 18:49:44 +0000
Subject: [PATCH] fix rename try_to_reuse_anon_vma()

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/rmap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index f66f98416a7c..5da3b47b1251 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -267,8 +267,8 @@ static void check_anon_vma_clone(struct vm_area_struct *dst,
 #endif
 }

-static void try_to_reuse_anon_vma(struct vm_area_struct *dst,
-				 struct anon_vma *anon_vma)
+static void maybe_reuse_anon_vma(struct vm_area_struct *dst,
+		struct anon_vma *anon_vma)
 {
 	/* If already populated, nothing to do.*/
 	if (dst->anon_vma)
@@ -356,7 +356,7 @@ int anon_vma_clone(struct vm_area_struct *dst, struct vm_area_struct *src,

 		anon_vma_interval_tree_insert(avc, &anon_vma->rb_root);
 		if (operation == VMA_OP_FORK)
-			try_to_reuse_anon_vma(dst, anon_vma);
+			maybe_reuse_anon_vma(dst, anon_vma);
 	}

 	if (operation != VMA_OP_FORK)
--
2.52.0