On 8/5/24 14:13, Lorenzo Stoakes wrote:
> Equally use struct vma_merge_struct to abstract parameters for VMA
> expansion and shrinking.
>
> This leads the way to further refactoring and de-duplication by
> standardising the interface.
>
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> mm/mmap.c | 30 +++++++++++--------
> mm/vma.c | 66 ++++++++++++++++++-----------------------
> mm/vma.h | 8 ++---
> tools/testing/vma/vma.c | 18 +++++++++--
> 4 files changed, 65 insertions(+), 57 deletions(-)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 721ced6e37b0..04145347c245 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1367,7 +1367,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> pgoff_t pglen = len >> PAGE_SHIFT;
> unsigned long charged = 0;
> unsigned long end = addr + len;
> - unsigned long merge_start = addr, merge_end = end;
> bool writable_file_mapping = false;
> int error;
> VMA_ITERATOR(vmi, mm, addr);
> @@ -1423,28 +1422,26 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> /* Attempt to expand an old mapping */
> /* Check next */
> if (next && next->vm_start == end && can_vma_merge_before(&vmg)) {
> - merge_end = next->vm_end;
> - vma = next;
> + /* We can adjust this as can_vma_merge_after() doesn't touch */
> + vmg.end = next->vm_end;
Ugh, ok but wonder how fragile that is.
> + vma = vmg.vma = next;
> vmg.pgoff = next->vm_pgoff - pglen;
> - }
>
> - if (vma) {
> + /* We may merge our NULL anon_vma with non-NULL in next. */
Hm now I realize the if (vma) block probably didn't need to be added in
patch 2 only to removed here, it could have been part of the if (next &&
...) block above already? Which is not that important, but...
> vmg.anon_vma = vma->anon_vma;
> - vmg.uffd_ctx = vma->vm_userfaultfd_ctx;
I don't see why it's now ok to remove this line? Was it intended? In patch 2
it made sense to me to add it so the can_vma_merge_after() still has the
right ctx for comparing, and this didn't change?
> }
>
> /* Check prev */
> if (prev && prev->vm_end == addr && can_vma_merge_after(&vmg)) {
> - merge_start = prev->vm_start;
> - vma = prev;
> + vmg.start = prev->vm_start;
> + vma = vmg.vma = prev;
> vmg.pgoff = prev->vm_pgoff;
> } else if (prev) {
> vma_iter_next_range(&vmi);
> }
>
> /* Actually expand, if possible */
> - if (vma &&
> - !vma_expand(&vmi, vma, merge_start, merge_end, vmg.pgoff, next)) {
> + if (vma && !vma_expand(&vmg)) {
> khugepaged_enter_vma(vma, vm_flags);
> goto expanded;
> }
> @@ -2359,6 +2356,13 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> VMA_ITERATOR(vmi, mm, new_start);
> struct vm_area_struct *next;
> struct mmu_gather tlb;
> + struct vma_merge_struct vmg = {
> + .vmi = &vmi,
> + .vma = vma,
> + .start = new_start,
> + .end = old_end,
> + .pgoff = vma->vm_pgoff,
> + };
>
> BUG_ON(new_start > new_end);
>
> @@ -2373,7 +2377,7 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> /*
> * cover the whole range: [new_start, old_end)
> */
> - if (vma_expand(&vmi, vma, new_start, old_end, vma->vm_pgoff, NULL))
> + if (vma_expand(&vmg))
> return -ENOMEM;
>
> /*
> @@ -2406,6 +2410,8 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
> tlb_finish_mmu(&tlb);
>
> vma_prev(&vmi);
> + vmg.end = new_end;
> +
> /* Shrink the vma to just the new range */
> - return vma_shrink(&vmi, vma, new_start, new_end, vma->vm_pgoff);
> + return vma_shrink(&vmg);
The vma_shrink() doesn't seem to benefit that much from vmg conversion but I
guess why not. Maybe this will further change anyway...
