Re: [PATCH 1/3] mm/vma: fix incorrectly disallowed anonymous VMA merges

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Mar 17, 2025 at 09:15:03PM +0000, Lorenzo Stoakes wrote:
[...]
>However, we have a problem here - typically the vma passed here is the
>destination VMA.
>
>For instance in vma_merge_existing_range() we invoke:
>
>can_vma_merge_left()
>-> [ check that there is an immediately adjacent prior VMA ]
>-> can_vma_merge_after()
>  -> is_mergeable_vma() for general attribute check
>-> is_mergeable_anon_vma([ proposed anon_vma ], prev->anon_vma, prev)
>
>So if we were considering a target unfaulted 'prev':
>
>	  unfaulted    faulted
>	|-----------|-----------|
>	|    prev   |    vma    |
>	|-----------|-----------|
>
>This would call is_mergeable_anon_vma(NULL, vma->anon_vma, prev).
>
>The list_is_singular() check for vma->anon_vma_chain, an empty list on
>fault, would cause this merge to _fail_ even though all else indicates a
>merge.
>

Great spot. It is hiding there for 15 years.

>Equally a simple merge into a next VMA would hit the same problem:
>
>	   faulted    unfaulted
>	|-----------|-----------|
>	|    vma    |    next   |
>	|-----------|-----------|
>
[...]
>---
> mm/vma.c                |  81 +++++++++++++++++++++++---------
> tools/testing/vma/vma.c | 100 +++++++++++++++++++++-------------------
> 2 files changed, 111 insertions(+), 70 deletions(-)
>
>diff --git a/mm/vma.c b/mm/vma.c
>index 5cdc5612bfc1..5418eef3a852 100644
>--- a/mm/vma.c
>+++ b/mm/vma.c
>@@ -57,6 +57,22 @@ struct mmap_state {
> 		.state = VMA_MERGE_START,				\
> 	}
> 
>+/*
>+ * If, at any point, the VMA had unCoW'd mappings from parents, it will maintain
>+ * more than one anon_vma_chain connecting it to more than one anon_vma. A merge
>+ * would mean a wider range of folios sharing the root anon_vma lock, and thus
>+ * potential lock contention, we do not wish to encourage merging such that this
>+ * scales to a problem.
>+ */

I don't follow here. Take a look into do_wp_page(), where CoW happens. But I
don't find where it will unlink parent anon_vma from vma->anon_vma_chain.

Per my understanding, the unlink behavior happens in unlink_anon_vma() which
unlink all anon_vma on vma->anon_vma_chain. And the normal caller of
unlink_anon_vma() is free_pgtables(). Other callers are on error path to
release prepared data. From this perspective, I don't see the chance to unlink
parent anon_vma from vma->anon_vma_chain either.

But maybe I missed something. If it is not too bother, would you mind giving
me a hint?

>+static bool vma_had_uncowed_parents(struct vm_area_struct *vma)
>+{
>+	/*
>+	 * The list_is_singular() test is to avoid merging VMA cloned from
>+	 * parents. This can improve scalability caused by anon_vma lock.
>+	 */
>+	return vma && vma->anon_vma && !list_is_singular(&vma->anon_vma_chain);
>+}
>+
> static inline bool is_mergeable_vma(struct vma_merge_struct *vmg, bool merge_next)
> {
> 	struct vm_area_struct *vma = merge_next ? vmg->next : vmg->prev;
>@@ -82,24 +98,28 @@ static inline bool is_mergeable_vma(struct vma_merge_struct *vmg, bool merge_nex
> 	return true;
> }
> 
>-static inline bool is_mergeable_anon_vma(struct anon_vma *anon_vma1,
>-		 struct anon_vma *anon_vma2, struct vm_area_struct *vma)
>+static bool is_mergeable_anon_vma(struct vma_merge_struct *vmg, bool merge_next)
> {
>+	struct vm_area_struct *tgt = merge_next ? vmg->next : vmg->prev;
>+	struct vm_area_struct *src = vmg->middle; /* exisitng merge case. */
                                                     ^^^

A trivial typo here.

>+	struct anon_vma *tgt_anon = tgt->anon_vma;
>+	struct anon_vma *src_anon = vmg->anon_vma;
>+
> 	/*
>-	 * The list_is_singular() test is to avoid merging VMA cloned from
>-	 * parents. This can improve scalability caused by anon_vma lock.
>+	 * We _can_ have !src, vmg->anon_vma via copy_vma(). In this instance we
>+	 * will remove the existing VMA's anon_vma's so there's no scalability
>+	 * concerns.
> 	 */
>-	if ((!anon_vma1 || !anon_vma2) && (!vma ||
>-		list_is_singular(&vma->anon_vma_chain)))
>-		return true;
>-	return anon_vma1 == anon_vma2;
>-}
>+	VM_WARN_ON(src && src_anon != src->anon_vma);
> 
>-/* Are the anon_vma's belonging to each VMA compatible with one another? */
>-static inline bool are_anon_vmas_compatible(struct vm_area_struct *vma1,
>-					    struct vm_area_struct *vma2)
>-{
>-	return is_mergeable_anon_vma(vma1->anon_vma, vma2->anon_vma, NULL);
>+	/* Case 1 - we will dup_anon_vma() from src into tgt. */
>+	if (!tgt_anon && src_anon)
>+		return !vma_had_uncowed_parents(src);
>+	/* Case 2 - we will simply use tgt's anon_vma. */
>+	if (tgt_anon && !src_anon)
>+		return !vma_had_uncowed_parents(tgt);
>+	/* Case 3 - the anon_vma's are already shared. */
>+	return src_anon == tgt_anon;
> }
> 
> /*
>@@ -164,7 +184,7 @@ static bool can_vma_merge_before(struct vma_merge_struct *vmg)
> 	pgoff_t pglen = PHYS_PFN(vmg->end - vmg->start);
> 
> 	if (is_mergeable_vma(vmg, /* merge_next = */ true) &&
>-	    is_mergeable_anon_vma(vmg->anon_vma, vmg->next->anon_vma, vmg->next)) {
>+	    is_mergeable_anon_vma(vmg, /* merge_next = */ true)) {
> 		if (vmg->next->vm_pgoff == vmg->pgoff + pglen)
> 			return true;
> 	}
>@@ -184,7 +204,7 @@ static bool can_vma_merge_before(struct vma_merge_struct *vmg)
> static bool can_vma_merge_after(struct vma_merge_struct *vmg)
> {
> 	if (is_mergeable_vma(vmg, /* merge_next = */ false) &&
>-	    is_mergeable_anon_vma(vmg->anon_vma, vmg->prev->anon_vma, vmg->prev)) {
>+	    is_mergeable_anon_vma(vmg, /* merge_next = */ false)) {
> 		if (vmg->prev->vm_pgoff + vma_pages(vmg->prev) == vmg->pgoff)
> 			return true;
> 	}

We have two sets API to check vma's mergeability:

  * can_vma_merge_before/after
  * can_vma_merge_left/right

And xxx_merge_right() calls xxx_merge_before(), which is a little confusing.

Now can_vma_merge_before/after looks almost same. Do you think it would be
easier for reading to consolidate to one function?

-- 
Wei Yang
Help you, Help me