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

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

On Tue, 6 Aug 2024 15:30:49 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

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

The history you make today becomes the learning material for the next
generation of kernel hackers (who will also lack a lot of context).

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

Yep. We can all agre this is the best way to convey the idea behind the
changes. Don't get me wrong; this whole series does a lot of good in
terms of code readability, even for a bystander like myself.

Petr T