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()?
> }
>
> - /* 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>
