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!
