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