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

[Vlastimil Babka <vbabka@suse.cz>]

On 8/5/24 14:13, Lorenzo Stoakes 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>

In general,

Acked-by: Vlastimil Babka <vbabka@suse.cz>

If you consider the following suggestions, great:

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

I've read the subthread with Petr. I understand it's hard to organize such
big changes in self-contained units. But maybe it would still be possible to
introduce this function now without the parameters, and as part of the the
next patch add the two parameters and the code using them. Maybe it would
even make git detect the added code as code move from where it's now, so it
would be more obviousl.

> +{
> +	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);

This originally had a comment

/* Note: vma iterator must be pointing to 'start' */

and now it's gone.

> +	} else {
> +		vma_iter_config(vmg->vmi, adjust->vm_start + adj_start,
> +				adjust->vm_end);

And this less obvious one has none either :(

> +	}
> +
> +	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: