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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
