Re: [PATCH 08/10] mm: introduce commit_merge(), abstracting merge operation

["Petr Tesařík" <petr@tesarici.cz>]

On Mon,  5 Aug 2024 13:13:55 +0100
Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote:

> Pull this operation into its own function and have vma_expand() call
> commit_merge() instead.
> 
> This lays the groundwork for a subsequent patch which replaces vma_merge()
> with a simpler function which can share the same code.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> ---
>  mm/vma.c | 57 ++++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/vma.c b/mm/vma.c
> index a404cf718f9e..b7e3c64d5d68 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -564,6 +564,49 @@ void validate_mm(struct mm_struct *mm)
>  }
>  #endif /* CONFIG_DEBUG_VM_MAPLE_TREE */
>  
> +/* Actually perform the VMA merge operation. */
> +static int commit_merge(struct vma_merge_struct *vmg,
> +			struct vm_area_struct *adjust,
> +			struct vm_area_struct *remove,
> +			struct vm_area_struct *remove2,
> +			long adj_start,
> +			bool expanded)
> +{
> +	struct vma_prepare vp;
> +
> +	init_multi_vma_prep(&vp, vmg->vma, adjust, remove, remove2);
> +
> +	if (expanded) {
> +		vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> +	} else {
> +		vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> +				adjust->vm_end);
> +	}

It's hard to follow the logic if you the "expanded" parameter is always
true. I have to look at PATCH 09/10 first to see how it is expected to
be used. Is there no other way?

Note that this is not needed for adjust and adj_start, because they are
merely moved here from vma_expand() and passed down as parameters to
other functions.

Petr T

> +
> +	if (vma_iter_prealloc(vmg->vmi, vmg->vma))
> +		return -ENOMEM;
> +
> +	vma_prepare(&vp);
> +	vma_adjust_trans_huge(vmg->vma, vmg->start, vmg->end, adj_start);
> +	vma_set_range(vmg->vma, vmg->start, vmg->end, vmg->pgoff);
> +
> +	if (expanded)
> +		vma_iter_store(vmg->vmi, vmg->vma);
> +
> +	if (adj_start) {
> +		adjust->vm_start += adj_start;
> +		adjust->vm_pgoff += PHYS_PFN(adj_start);
> +		if (adj_start < 0) {
> +			WARN_ON(expanded);
> +			vma_iter_store(vmg->vmi, adjust);
> +		}
> +	}
> +
> +	vma_complete(&vp, vmg->vmi, vmg->vma->vm_mm);
> +
> +	return 0;
> +}
> +
>  /*
>   * vma_merge_new_vma - Attempt to merge a new VMA into address space
>   *
> @@ -700,7 +743,6 @@ int vma_expand(struct vma_merge_struct *vmg)
>  	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) && (vmg->end == next->vm_end)) {
> @@ -713,24 +755,15 @@ int vma_expand(struct vma_merge_struct *vmg)
>  			return ret;
>  	}
>  
> -	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 &&
> +	VM_WARN_ON(next && !remove_next &&
>  		  next != vma && vmg->end > next->vm_start);
>  	/* Only handles expanding */
>  	VM_WARN_ON(vma->vm_start < vmg->start || vma->vm_end > vmg->end);
>  
> -	/* Note: vma iterator must be pointing to 'start' */
> -	vma_iter_config(vmg->vmi, vmg->start, vmg->end);
> -	if (vma_iter_prealloc(vmg->vmi, vma))
> +	if (commit_merge(vmg, NULL, remove_next ? next : NULL, NULL, 0, true))
>  		goto nomem;
>  
> -	vma_prepare(&vp);
> -	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, vmg->vmi, vma->vm_mm);
>  	return 0;
>  
>  nomem: