Re: [PATCH] mm/mmap: Add case 9 in vma_merge()

["Liam R. Howlett" <Liam.Howlett@Oracle.com>]

* Yajun Deng <yajun.deng@linux.dev> [240219 22:00]:
> 
> On 2024/2/19 07:03, Lorenzo Stoakes wrote:
> > On Sun, Feb 18, 2024 at 04:50:28PM +0800, Yajun Deng wrote:
> > > If the prev vma exists and the end is less than the end of prev, we
> > > can return NULL immediately. This reduces unnecessary operations.
> > > 
> > > Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> > Adding Vlastimil, while get_maintainers.pl might not show it very clearly,
> > myself, Vlastimil and Liam often work with vma_merge() so it's handy to cc
> > us on these if you can!
> Okay.
> > > ---
> > >   mm/mmap.c | 5 ++++-
> > >   1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index 8f176027583c..b738849321c0 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -827,7 +827,7 @@ can_vma_merge_after(struct vm_area_struct *vma, unsigned long vm_flags,
> > >    *
> > >    *     ****             ****                   ****
> > >    *    PPPPPPNNNNNN    PPPPPPNNNNNN       PPPPPPCCCCCC
> > > - *    cannot merge    might become       might become
> > > + *    cannot merge 9  might become       might become
> > While I welcome your interest here :) I am not a fan of the 'case' approach
> > to this function as-is and plan to heavily refactor this when I get a chance.
> > 
> > But at any rate, an early-exit situation is not a merge case, merge cases
> > describe cases where we _can_ merge, so we can drop this case 9 stuff (this
> > is not your fault, it's understandable why you would label this, this
> > function is just generally unclear).
> 
> Yes, it's not a merge case. I label this to make it easier to understand.

But it isn't.  It's not a case at all, it's a failure to merge.

> 
> > >    *                    PPNNNNNNNNNN       PPPPPPPPPPCC
> > >    *    mmap, brk or    case 4 below       case 5 below
> > >    *    mremap move:
> > > @@ -890,6 +890,9 @@ static struct vm_area_struct
> > >   	if (vm_flags & VM_SPECIAL)
> > >   		return NULL;
> > > 
> > > +	if (prev && end < prev->vm_end) /* case 9 */
> > > +		return NULL;
> > > +
> > I need to get back into vma_merge() head space, but I don't actually think
> > a caller that's behaving correctly should ever do this. I know the ASCII
> > diagram above lists it as a thing that can happen, but I think we
> > implicitly avoid this from the way we invoke callers. Either prev == vma as
> > per vma_merge_extend(), or the loops that invoke vma_merge_new_vma()
> > wouldn't permit this to occur.
> No, it will actually happen. That's why I submitted this patch.

Can you elaborate on where it happens?  I mean, you seem to have already
looked into it but haven't shared what you found of where it reduces the
unnecessary operations.

Such a detail should also be added to the commit log so that, when the
call sites change, this check could be dropped - or be seen as
necessary.