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

["Petr Tesařík" <petr@tesarici.cz>]

On Tue, 6 Aug 2024 14:48:33 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> On Tue, Aug 06, 2024 at 03:41:16PM GMT, Petr Tesařík wrote:
> > On Mon,  5 Aug 2024 13:13:55 +0100
> > Lorenzo Stoakes <lorenzo.stoakes@oracle.com> 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>
> > > ---
> > >  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)
> > > +{
> > > +	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);
> > > +	} else {
> > > +		vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> > > +				adjust->vm_end);
> > > +	}  
> >
> > It's hard to follow the logic if you the "expanded" parameter is always
> > true. I have to look at PATCH 09/10 first to see how it is expected to
> > be used. Is there no other way?
> >
> > Note that this is not needed for adjust and adj_start, because they are
> > merely moved here from vma_expand() and passed down as parameters to
> > other functions.  
> 
> See the next patch to understand how these are used, as the commit message
> says, this lays the groundwork for the next patch which actually uses both
> of these.
> 
> I have tried hard to clarify how these are used, however there is some
> unavoidable and inherent complexity in this logic. If you don't believe me,
> I suggest trying to follow the logic of the existing code :)
> 
> And if you want to _really_ have fun, I suggest you try to understand the
> logic around v6.0 prior to Liam's interventions.
> 
> We might be able to try to improve the logic flow further, but it's one
> step at a time with this.

What I mean is: Is there no way to arrange the patch series so that I
don't have to look at PATH 09/10 before I can understand code in patch
08/10?

This PATCH 08/10 adds only one call to commit_merge() and that one
always sets expanded to true. Maybe you could introduce commit_merge()
here without the parameter and add it in PATCH 09/10?

Petr T