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

[Liam R. Howlett]

* Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> [240708 07:08]:
> 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?

Okay, I'll add something.

> 
> >
> > >
> > > >  	}
> > > >
> > > > -	/* 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!