diff mbox series

[05/10] mm: abstract vma_merge_new_vma() to use vma_merge_struct

Message ID f4f4b5d441379b22f92d7cd188c2095198cdbc86.1722849859.git.lorenzo.stoakes@oracle.com (mailing list archive)
State New
Headers show
Series mm: remove vma_merge() | expand

Commit Message

Lorenzo Stoakes Aug. 5, 2024, 12:13 p.m. UTC 
Abstract this function to so we can write tests which use the newly
abstracted interface and maintain a stable interface for tests before/after
refactoring.

We introduce a temporary wrapper vma_merge_new_vma_wrapper() to minimise
the code changes, in a subsequent commit we will entirely refactor this
function.

We also introduce a temporary implementation of vma_merge_modified() for
the same reason - maintaining a common interface to the tests, this will be
removed when vma_merge_modified() is correctly implemented in a subsequent
commit.

Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
---
 mm/mmap.c               |  6 +++---
 mm/vma.c                | 33 ++++++++++++---------------------
 mm/vma.h                | 33 ++++++++++++++++++++++++++++++---
 tools/testing/vma/vma.c | 12 ++++++++----
 4 files changed, 53 insertions(+), 31 deletions(-)

--
2.45.2

Comments

Lorenzo Stoakes Aug. 8, 2024, 3:52 p.m. UTC | #1 
On Thu, Aug 08, 2024 at 04:55:07PM GMT, Vlastimil Babka (SUSE) wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > Abstract this function to so we can write tests which use the newly
> > abstracted interface and maintain a stable interface for tests before/after
> > refactoring.
> >
> > We introduce a temporary wrapper vma_merge_new_vma_wrapper() to minimise
> > the code changes, in a subsequent commit we will entirely refactor this
> > function.
> >
> > We also introduce a temporary implementation of vma_merge_modified() for
> > the same reason - maintaining a common interface to the tests, this will be
> > removed when vma_merge_modified() is correctly implemented in a subsequent
> > commit.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> <snip>
>
> > +struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
> >  {
> > -	struct vma_merge_struct vmg = {
> > -		.vmi = vmi,
> > -		.prev = prev,
> > -		.vma = vma,
>
> This line not being addded in the wrapper felt like a mistake. I see in the
> other subthread that it's not so I'll just support your decision to tackle
> it so the code is less surprising.

This is just a wrapper that gets replaced in a subsequent commit.

For new VMAs we don't strictly need vma, as vma_merge() no longer references
this.

This is purposeful for testing purposes.

>
> > -		.start = start,
> > -		.end = end,
> > -		.flags = vma->vm_flags,
> > -		.file = vma->vm_file,
> > -		.anon_vma = vma->anon_vma,
> > -		.pgoff = pgoff,
> > -		.policy = vma_policy(vma),
> > -		.uffd_ctx = vma->vm_userfaultfd_ctx,
> > -		.anon_name = anon_vma_name(vma),
> > -	};
> > +	if (!vmg->prev) {
> > +		vmg->prev = vma_prev(vmg->vmi);
> > +		vma_iter_set(vmg->vmi, vmg->start);
> > +	}
>
> Admit this is another surprise. The old code didn't do it anywhere AFAICS so
> I don't see why it's now done. Maybe it's necessary for futher changes.
> Could you add a comment or explain it in the changelog, please?

So in the future (actual non-wrapper) version of vma_merge_new_vma(), we do not
require that the previous VMA is specified.

vma_merge() _does_ require this. So we have to go look it up if it's not there.

This is so we can add the tests in the next commit, and have them be completely
identical for the functions _before_ the refactor and afterwards so we can
assert that they pass in both cases.

I'll add a comment accordingly.

>
> > -	return vma_merge(&vmg);
> > +	return vma_merge(vmg);
> >  }
> >
> >  /*
>
diff mbox series

Patch

diff --git a/mm/mmap.c b/mm/mmap.c
index 04145347c245..f6593a81f73d 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1494,9 +1494,9 @@  unsigned long mmap_region(struct file *file, unsigned long addr,
 		 * vma again as we may succeed this time.
 		 */
 		if (unlikely(vm_flags != vma->vm_flags && prev)) {
-			merge = vma_merge_new_vma(&vmi, prev, vma,
-						  vma->vm_start, vma->vm_end,
-						  vma->vm_pgoff);
+			merge = vma_merge_new_vma_wrapper(&vmi, prev, vma,
+							  vma->vm_start, vma->vm_end,
+							  vma->vm_pgoff);
 			if (merge) {
 				/*
 				 * ->mmap() can change vma->vm_file and fput
diff --git a/mm/vma.c b/mm/vma.c
index 3d6ce04f1b9c..55615392e8d2 100644
--- a/mm/vma.c
+++ b/mm/vma.c
@@ -1106,6 +1106,11 @@  static struct vm_area_struct *vma_merge(struct vma_merge_struct *vmg)
 	return NULL;
 }

+struct vm_area_struct *vma_merge_modified(struct vma_merge_struct *vmg)
+{
+	return vma_merge(vmg);
+}
+
 /*
  * We are about to modify one or multiple of a VMA's flags, policy, userfaultfd
  * context and anonymous VMA name within the range [start, end).
@@ -1260,27 +1265,14 @@  struct vm_area_struct
  * Attempt to merge a newly mapped VMA with those adjacent to it. The caller
  * must ensure that [start, end) does not overlap any existing VMA.
  */
-struct vm_area_struct
-*vma_merge_new_vma(struct vma_iterator *vmi, struct vm_area_struct *prev,
-		   struct vm_area_struct *vma, unsigned long start,
-		   unsigned long end, pgoff_t pgoff)
+struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
 {
-	struct vma_merge_struct vmg = {
-		.vmi = vmi,
-		.prev = prev,
-		.vma = vma,
-		.start = start,
-		.end = end,
-		.flags = vma->vm_flags,
-		.file = vma->vm_file,
-		.anon_vma = vma->anon_vma,
-		.pgoff = pgoff,
-		.policy = vma_policy(vma),
-		.uffd_ctx = vma->vm_userfaultfd_ctx,
-		.anon_name = anon_vma_name(vma),
-	};
+	if (!vmg->prev) {
+		vmg->prev = vma_prev(vmg->vmi);
+		vma_iter_set(vmg->vmi, vmg->start);
+	}

-	return vma_merge(&vmg);
+	return vma_merge(vmg);
 }

 /*
@@ -1295,7 +1287,6 @@  struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi,
 	struct vma_merge_struct vmg = {
 		.vmi = vmi,
 		.prev = vma,
-		.vma = vma,
 		.start = vma->vm_end,
 		.end = vma->vm_end + delta,
 		.flags = vma->vm_flags,
@@ -1425,7 +1416,7 @@  struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
 	if (new_vma && new_vma->vm_start < addr + len)
 		return NULL;	/* should never get here */

-	new_vma = vma_merge_new_vma(&vmi, prev, vma, addr, addr + len, pgoff);
+	new_vma = vma_merge_new_vma_wrapper(&vmi, prev, vma, addr, addr + len, pgoff);
 	if (new_vma) {
 		/*
 		 * Source vma may have been merged into new_vma
diff --git a/mm/vma.h b/mm/vma.h
index c464d25da120..50459f9e4c7f 100644
--- a/mm/vma.h
+++ b/mm/vma.h
@@ -134,9 +134,36 @@  struct vm_area_struct
 		       struct vm_userfaultfd_ctx new_ctx);

 struct vm_area_struct
-*vma_merge_new_vma(struct vma_iterator *vmi, struct vm_area_struct *prev,
-		   struct vm_area_struct *vma, unsigned long start,
-		   unsigned long end, pgoff_t pgoff);
+*vma_merge_new_vma(struct vma_merge_struct *vmg);
+
+/* Temporary convenience wrapper. */
+static inline struct vm_area_struct
+*vma_merge_new_vma_wrapper(struct vma_iterator *vmi, struct vm_area_struct *prev,
+			   struct vm_area_struct *vma, unsigned long start,
+			   unsigned long end, pgoff_t pgoff)
+{
+	struct vma_merge_struct vmg = {
+		.vmi = vmi,
+		.prev = prev,
+		.start = start,
+		.end = end,
+		.flags = vma->vm_flags,
+		.file = vma->vm_file,
+		.anon_vma = vma->anon_vma,
+		.pgoff = pgoff,
+		.policy = vma_policy(vma),
+		.uffd_ctx = vma->vm_userfaultfd_ctx,
+		.anon_name = anon_vma_name(vma),
+	};
+
+	return vma_merge_new_vma(&vmg);
+}
+
+/*
+ * Temporary wrapper around vma_merge() so we can have a common interface for
+ * tests.
+ */
+struct vm_area_struct *vma_merge_modified(struct vma_merge_struct *vmg);

 struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi,
 					struct vm_area_struct *vma,
diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
index d216e51206c1..4416cfa93056 100644
--- a/tools/testing/vma/vma.c
+++ b/tools/testing/vma/vma.c
@@ -53,16 +53,20 @@  static bool test_simple_merge(void)
 	unsigned long flags = VM_READ | VM_WRITE | VM_MAYREAD | VM_MAYWRITE;
 	struct mm_struct mm = {};
 	struct vm_area_struct *vma_left = alloc_vma(&mm, 0, 0x1000, 0, flags);
-	struct vm_area_struct *vma_middle = alloc_vma(&mm, 0x1000, 0x2000, 1, flags);
 	struct vm_area_struct *vma_right = alloc_vma(&mm, 0x2000, 0x3000, 2, flags);
 	VMA_ITERATOR(vmi, &mm, 0x1000);
+	struct vma_merge_struct vmg = {
+		.vmi = &vmi,
+		.start = 0x1000,
+		.end = 0x2000,
+		.flags = flags,
+		.pgoff = 1,
+	};

 	ASSERT_FALSE(vma_link(&mm, vma_left));
-	ASSERT_FALSE(vma_link(&mm, vma_middle));
 	ASSERT_FALSE(vma_link(&mm, vma_right));

-	vma = vma_merge_new_vma(&vmi, vma_left, vma_middle, 0x1000,
-				0x2000, 1);
+	vma = vma_merge_new_vma(&vmg);
 	ASSERT_NE(vma, NULL);

 	ASSERT_EQ(vma->vm_start, 0);