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

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

On Tue, Aug 06, 2024 at 04:13:21PM GMT, Petr Tesařík wrote:
> 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?

No.

>
> 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?

No, I won't do that, you haven't made a case for it.

>
> Petr T

I appreciate you are doing a drive-by review on code you aren't familiar
with, but it's worth appreciating that there is some context here - this is
intentionally isolating _existing_ logic from vma_expand() and vma_merge()
in such a way that we have a _generic_ function we can use for this
operation.

I think it'd be _more_ confusing and (surprising given your rather pedantic
interpretation of churn elsewhere) churny to rewrite this again with a
bunch of added logic in the next commit.

I think this is highly subjective, and I'm not sure it's a great use of
either of our time to get too stuck in the weeds on this kind of thing.

Of course if you or others can present a more compelling argument for
reworking this I'm happy to hear.