Re: [PATCH v3 10/16] mm/mmap: Reposition vma iterator in mmap_region()

[Lorenzo Stoakes]

On Fri, Jul 05, 2024 at 04:56:34PM GMT, Liam R. Howlett wrote:
> * Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [240705 16:18]:
> > On Thu, Jul 04, 2024 at 02:27:12PM GMT, Liam R. Howlett wrote:
> > > From: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > >
> > > Instead of moving (or leaving) the vma iterator pointing at the previous
> > > vma, leave it pointing at the insert location.  Pointing the vma
> > > iterator at the insert location allows for a cleaner walk of the vma
> > > tree for MAP_FIXED and the no expansion cases.
> >
> > I mean, it's funny I litearlly just asked why it was being left pointing at
> > prev and I guess this answers that basically :)
> >
> > >
> > > Signed-off-by: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> > > ---
> > >  mm/mmap.c | 28 ++++++++++++++--------------
> > >  1 file changed, 14 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > index f5b33de4e717..ecf55d32e804 100644
> > > --- a/mm/mmap.c
> > > +++ b/mm/mmap.c
> > > @@ -2963,11 +2963,12 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > >  		vms_complete_munmap_vmas(&vms, &mas_detach);
> > >  		next = vms.next;
> > >  		prev = vms.prev;
> > > -		vma_prev(&vmi);
> > >  		vma = NULL;
> > >  	} else {
> > >  		next = vma_next(&vmi);
> > >  		prev = vma_prev(&vmi);
> > > +		if (prev)
> > > +			vma_iter_next_range(&vmi);
> > >  	}
> > >
> > >  	/*
> > > @@ -2980,11 +2981,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > >  		vm_flags |= VM_ACCOUNT;
> > >  	}
> > >
> > > -	if (vm_flags & VM_SPECIAL) {
> > > -		if (prev)
> > > -			vma_iter_next_range(&vmi);
> > > +	if (vm_flags & VM_SPECIAL)
> > >  		goto cannot_expand;
> > > -	}
> > >
> > >  	/* Attempt to expand an old mapping */
> > >  	/* Check next */
> > > @@ -3005,19 +3003,21 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > >  		merge_start = prev->vm_start;
> > >  		vma = prev;
> > >  		vm_pgoff = prev->vm_pgoff;
> > > -	} else if (prev) {
> > > -		vma_iter_next_range(&vmi);
> > > +		vma_prev(&vmi);
> >
> > Why not vma_iter_prev_range()?
>
> In any situation where we arrive at prev being able to merge with the
> current, the prev->end == addr and we are already pointing at the gap
> with addr.  So vma_iter_prev_range() and vma_prev() will do the same
> thing here.
>
> I thought a lot about this (and created a small test application since
> it is so difficult to test vma merging right now..).  If we do not clear
> the gap, it is possible we will be pointing at the first vma in the
> range, and not addr at all when we call vms_gather_munmap_vmas(), but
> the loop at the end of the gather function ensures the vmi is at the
> correct address (vms->start is in the range). So the result is that we
> consistently point to the correct location and so these two calls will
> do the same thing.

Might it be worth a comment here?

>
> >
> > >  	}
> > >
> > > -	/* Actually expand, if possible */
> > > -	if (vma &&
> > > -	    !vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
> > > -		khugepaged_enter_vma(vma, vm_flags);
> > > -		goto expanded;
> > > +	if (vma) {
> > > +		/* Actually expand, if possible */
> > > +		if (!vma_expand(&vmi, vma, merge_start, merge_end, vm_pgoff, next)) {
> > > +			khugepaged_enter_vma(vma, vm_flags);
> > > +			goto expanded;
> > > +		}
> > > +
> > > +		/* If the expand fails, then reposition the vma iterator */
> > > +		if (unlikely(vma == prev))
> > > +			vma_iter_set(&vmi, addr);
> > >  	}
> > >
> > > -	if (vma == prev)
> > > -		vma_iter_set(&vmi, addr);
> > >  cannot_expand:
> > >
> > >  	/*
> > > --
> > > 2.43.0
> > >
> > Looks good to me,
> >
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>
> Thanks!