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

[Suren Baghdasaryan]

On Thu, Jul 4, 2024 at 11:27 AM Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> 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.
>
> 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);
>         }
>
> -       /* 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);

Before this change we would reposition vmi if vma == prev == NULL.
After this change we don't do that. Is this situation possible and if
so, will vmi be correct?

>  cannot_expand:
>
>         /*
> --
> 2.43.0
>