Re: [PATCH 04/10] mm: abstract parameters for vma_expand/shrink()

[Vlastimil Babka (SUSE)]

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...