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(-)
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);
--
2.45.2On 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.
> - .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?
> - return vma_merge(&vmg);
> + return vma_merge(vmg);
> }
>
> /*
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);
> > }
> >
> > /*
>
On Mon, 5 Aug 2024 13:13:52 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> 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>
> ---
> 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(-)
>
> 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,
Yes, this member is not used later by vma_merge(), so it need not be
initialized. What about not adding this line in PATCH 02/10 of this
series? AFAICS vmg->vma was never used by vma_merge(). The net result
is the same, but it would make it easier to understand that this patch
in the series does not change the use of vmg->vma by vma_merge_extend().
Petr T
On Thu, Aug 08, 2024 at 03:02:08PM GMT, Petr Tesařík wrote:
> On Mon, 5 Aug 2024 13:13:52 +0100
> Lorenzo Stoakes <lorenzo.stoakes@oracle.com> 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>
> > ---
> > 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(-)
> >
> > 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,
>
> Yes, this member is not used later by vma_merge(), so it need not be
> initialized. What about not adding this line in PATCH 02/10 of this
> series? AFAICS vmg->vma was never used by vma_merge(). The net result
> is the same, but it would make it easier to understand that this patch
> in the series does not change the use of vmg->vma by vma_merge_extend().
>
Yup fine. This is again not hugely important so I'll tackle it if a respin
on something more substantial comes up.
This is because previously the vma was used to reference mm, but I changed
how that worked to help testability.
> Petr T
© 2016 - 2026 Red Hat, Inc.