Message-ID: <zckithqj7lpa6cbvyfg2vtxwbpruwd5bqrgowy25ci6jaa3u2t@2nni4fjohkov>
Date: Wed, 28 Aug 2024 16:03:55 -0400
From: "Liam R. Howlett" <Liam.Howlett@...cle.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH v2 05/10] mm: abstract vma_expand() to use
 vma_merge_struct

* Lorenzo Stoakes <lorenzo.stoakes@...cle.com> [240823 16:07]:
> The purpose of the vmg is to thread merge state through functions and avoid
> egregious parameter lists. We expand this to vma_expand(), which is used
> for a number of merge cases.
> 
> Accordingly, adjust its callers, mmap_region() and relocate_vma_down(), to
> use a vmg.
> 
> An added purpose of this change is the ability in a future commit to
> perform all new VMA range merging using vma_expand().
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

Just one extra whitespace below.

Reviewed-by: Liam R. Howlett <Liam.Howlett@...cle.com>

> ---
>  mm/mmap.c               | 16 +++++++++-------
>  mm/vma.c                | 39 +++++++++++++++++----------------------
>  mm/vma.h                |  5 +----
>  tools/testing/vma/vma.c |  3 +--
>  4 files changed, 28 insertions(+), 35 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index c72f50feb98a..0d242c9b1f4c 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1371,7 +1371,6 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  	struct ma_state mas_detach;
>  	struct maple_tree mt_detach;
>  	unsigned long end = addr + len;
> -	unsigned long merge_start = addr, merge_end = end;
>  	bool writable_file_mapping = false;
>  	int error = -ENOMEM;
>  	VMA_ITERATOR(vmi, mm, addr);
> @@ -1424,8 +1423,8 @@ 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;
> +		vmg.end = next->vm_end;
> +		vma = vmg.vma = next;
>  		vmg.pgoff = next->vm_pgoff - pglen;
>  		/*
>  		 * We set this here so if we will merge with the previous VMA in
> @@ -1438,15 +1437,15 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  
>  	/* 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;
>  		vma_prev(&vmi); /* Equivalent to going to the previous range */
>  	}
>  
>  	if (vma) {
>  		/* Actually expand, if possible */
> -		if (!vma_expand(&vmi, vma, merge_start, merge_end, vmg.pgoff, next)) {
> +		if (!vma_expand(&vmg)) {
>  			khugepaged_enter_vma(vma, vm_flags);
>  			goto expanded;
>  		}
> @@ -2319,6 +2318,7 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
>  	unsigned long new_start = old_start - shift;
>  	unsigned long new_end = old_end - shift;
>  	VMA_ITERATOR(vmi, mm, new_start);
> +	VMG_STATE(vmg, mm, &vmi, new_start, old_end, 0, vma->vm_pgoff);
>  	struct vm_area_struct *next;
>  	struct mmu_gather tlb;
>  
> @@ -2335,7 +2335,8 @@ 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))
> +	vmg.vma = vma;
> +	if (vma_expand(&vmg))
>  		return -ENOMEM;
>  
>  	/*
> @@ -2368,6 +2369,7 @@ int relocate_vma_down(struct vm_area_struct *vma, unsigned long shift)
>  	tlb_finish_mmu(&tlb);
>  
>  	vma_prev(&vmi);
> +

Unnecessary whitespace change.

>  	/* Shrink the vma to just the new range */
>  	return vma_shrink(&vmi, vma, new_start, new_end, vma->vm_pgoff);
>  }
> diff --git a/mm/vma.c b/mm/vma.c
> index b1ec412fac7f..4867ae722a9a 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -467,30 +467,25 @@ void validate_mm(struct mm_struct *mm)
>  /*
>   * vma_expand - Expand an existing VMA
>   *
> - * @vmi: The vma iterator
> - * @vma: The vma to expand
> - * @start: The start of the vma
> - * @end: The exclusive end of the vma
> - * @pgoff: The page offset of vma
> - * @next: The current of next vma.
> + * @vmg: Describes a VMA expansion operation.
>   *
> - * Expand @vma to @start and @end.  Can expand off the start and end.  Will
> - * expand over @next if it's different from @vma and @end == @next->vm_end.
> - * Checking if the @vma can expand and merge with @next needs to be handled by
> - * the caller.
> + * Expand @vma to vmg->start and vmg->end.  Can expand off the start and end.
> + * Will expand over vmg->next if it's different from vmg->vma and vmg->end ==
> + * vmg->next->vm_end.  Checking if the vmg->vma can expand and merge with
> + * vmg->next needs to be handled by the caller.
>   *
>   * Returns: 0 on success
>   */
> -int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> -	       unsigned long start, unsigned long end, pgoff_t pgoff,
> -	       struct vm_area_struct *next)
> +int vma_expand(struct vma_merge_struct *vmg)
>  {
>  	struct vm_area_struct *anon_dup = NULL;
>  	bool remove_next = false;
> +	struct vm_area_struct *vma = vmg->vma;
> +	struct vm_area_struct *next = vmg->next;
>  	struct vma_prepare vp;
>  
>  	vma_start_write(vma);
> -	if (next && (vma != next) && (end == next->vm_end)) {
> +	if (next && (vma != next) && (vmg->end == next->vm_end)) {
>  		int ret;
>  
>  		remove_next = true;
> @@ -503,21 +498,21 @@ int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	init_multi_vma_prep(&vp, vma, NULL, remove_next ? next : NULL, NULL);
>  	/* Not merging but overwriting any part of next is not handled. */
>  	VM_WARN_ON(next && !vp.remove &&
> -		  next != vma && end > next->vm_start);
> +		  next != vma && vmg->end > next->vm_start);
>  	/* Only handles expanding */
> -	VM_WARN_ON(vma->vm_start < start || vma->vm_end > end);
> +	VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
>  
>  	/* Note: vma iterator must be pointing to 'start' */
> -	vma_iter_config(vmi, start, end);
> -	if (vma_iter_prealloc(vmi, vma))
> +	vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> +	if (vma_iter_prealloc(vmg->vmi, vma))
>  		goto nomem;
>  
>  	vma_prepare(&vp);
> -	vma_adjust_trans_huge(vma, start, end, 0);
> -	vma_set_range(vma, start, end, pgoff);
> -	vma_iter_store(vmi, vma);
> +	vma_adjust_trans_huge(vma, vmg->start, vmg->end, 0);
> +	vma_set_range(vma, vmg->start, vmg->end, vmg->pgoff);
> +	vma_iter_store(vmg->vmi, vma);
>  
> -	vma_complete(&vp, vmi, vma->vm_mm);
> +	vma_complete(&vp, vmg->vmi, vma->vm_mm);
>  	return 0;
>  
>  nomem:
> diff --git a/mm/vma.h b/mm/vma.h
> index 9060a1a1edfa..8f01fbc20fe7 100644
> --- a/mm/vma.h
> +++ b/mm/vma.h
> @@ -128,10 +128,7 @@ void init_vma_prep(struct vma_prepare *vp,
>  void vma_complete(struct vma_prepare *vp,
>  		  struct vma_iterator *vmi, struct mm_struct *mm);
>  
> -int vma_expand(struct vma_iterator *vmi, struct vm_area_struct *vma,
> -	       unsigned long start, unsigned long end, pgoff_t pgoff,
> -	       struct vm_area_struct *next);
> -
> +int vma_expand(struct vma_merge_struct *vmg);
>  int vma_shrink(struct vma_iterator *vmi, struct vm_area_struct *vma,
>  	       unsigned long start, unsigned long end, pgoff_t pgoff);
>  
> diff --git a/tools/testing/vma/vma.c b/tools/testing/vma/vma.c
> index b12f72979478..cce1ba78c57f 100644
> --- a/tools/testing/vma/vma.c
> +++ b/tools/testing/vma/vma.c
> @@ -121,8 +121,7 @@ static struct vm_area_struct *merge_existing(struct vma_merge_struct *vmg)
>   */
>  static int expand_existing(struct vma_merge_struct *vmg)
>  {
> -	return vma_expand(vmg->vmi, vmg->vma, vmg->start, vmg->end, vmg->pgoff,
> -			  vmg->next);
> +	return vma_expand(vmg);
>  }
>  
>  /*
> -- 
> 2.46.0
> 

Powered by blists - more mailing lists