Re: [PATCH 08/10] mm: introduce commit_merge(), abstracting merge operation

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Fri, Aug 09, 2024 at 12:15:24PM GMT, Vlastimil Babka wrote:
> On 8/5/24 14:13, Lorenzo Stoakes wrote:
> > Pull this operation into its own function and have vma_expand() call
> > commit_merge() instead.
> >
> > This lays the groundwork for a subsequent patch which replaces vma_merge()
> > with a simpler function which can share the same code.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> In general,
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
>
> If you consider the following suggestions, great:
>
> > ---
> >  mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 12 deletions(-)
> >
> > diff --git a/mm/vma.c b/mm/vma.c
> > index a404cf718f9e..b7e3c64d5d68 100644
> > --- a/mm/vma.c
> > +++ b/mm/vma.c
> > @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm)
> >  }
> >  #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */
> >
> > +/* Actually perform the VMA merge operation. */
> > +static int commit_merge(struct vma_merge_struct *vmg,
> > +			struct vm_area_struct *adjust,
> > +			struct vm_area_struct *remove,
> > +			struct vm_area_struct *remove2,
> > +			long adj_start,
> > +			bool expanded)
>
> I've read the subthread with Petr. I understand it's hard to organize such
> big changes in self-contained units. But maybe it would still be possible to
> introduce this function now without the parameters, and as part of the the
> next patch add the two parameters and the code using them. Maybe it would
> even make git detect the added code as code move from where it's now, so it
> would be more obviousl.

Since both you and Petr make the same point (sorry Petr, I should have
perhaps been a little less resistant to this), I will do this.

As discussed on IRC my position on this is that we're introducing a _really
fundamental_ and important bit of the logic here, intentionally broken out
as a separate commit, and this is why I preferred to introduce it 'fully
formed'.

This function is absolutely fundamental to eliminating the duplication in
do_brk_flags() + mmap_region() and to maintain two separate new/modified
vma versions of vma_merge().

HOWEVER, I totally accept that this makes review much more of a pain in the
arse, and in practice almost certainly the only thing that matters is
reviewability here as to how I structure this.

So TL;DR: I'll do what you both ask and introduce new params only when we
use them.

>
> > +{
> > +	struct vma_prepare vp;
> > +
> > +	init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2);
> > +
> > +	if (expanded) {
> > +		vma_iter_config(vmg->vmi, vmg->start, vmg->end);
>
> This originally had a comment
>
> /* Note: vma iterator must be pointing to 'start' */
>
> and now it's gone.

Will check and re-add if it makes sense. I mean we're now setting the iterator
to start anyway so I don't see that this has value? Maybe I'm missing something
and Liam has thoughts...

>
> > +	} else {
> > +		vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> > +				adjust->vm_end);
>
> And this less obvious one has none either :(

I will add a comment.

>
> > +	}
> > +
> > +	if (vma_iter_prealloc(vmg->vmi, vmg->vma))
> > +		return -ENOMEM;
> > +
> > +	vma_prepare(&vp);
> > +	vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start);
> > +	vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff);
> > +
> > +	if (expanded)
> > +		vma_iter_store(vmg->vmi, vmg->vma);
> > +
> > +	if (adj_start) {
> > +		adjust->vm_start += adj_start;
> > +		adjust->vm_pgoff += PHYS_PFN(adj_start);
> > +		if (adj_start < 0) {
> > +			WARN_ON(expanded);
> > +			vma_iter_store(vmg->vmi, adjust);
> > +		}
> > +	}
> > +
> > +	vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm);
> > +
> > +	return 0;
> > +}
> > +
> >  /*
> >   * vma_merge_new_vma - Attempt to merge a new VMA into address space
> >   *
> > @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg)
> >  	bool remove_next = false;
> >  	struct vm_area_struct *vma = vmg->vma;
> >  	struct vm_area_struct *next = vmg->next;
> > -	struct vma_prepare vp;
> >
> >  	vma_start_write(vma);
> >  	if (next && (vma != next) && (vmg->end == next->vm_end)) {
> > @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg)
> >  			return ret;
> >  	}
> >
> > -	init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL);
> >  	/* Not merging but overwriting any part of next is not handled. */
> > -	VM_WARN_ON(next && !vp.remove &&
> > +	VM_WARN_ON(next && !remove_next &&
> >  		  next != vma && vmg->end > next->vm_start);
> >  	/* Only handles expanding */
> >  	VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
> >
> > -	/* Note: vma iterator must be pointing to 'start' */
> > -	vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> > -	if (vma_iter_prealloc(vmg->vmi, vma))
> > +	if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true))
> >  		goto nomem;
> >
> > -	vma_prepare(&vp);
> > -	vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0);
> > -	vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff);
> > -	vma_iter_store(vmg->vmi, vma);
> > -
> > -	vma_complete(&vp, vmg->vmi, vma->vm_mm);
> >  	return 0;
> >
> >  nomem:
>