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

[Lorenzo Stoakes]

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