Re: [PATCH 05/10] mm: abstract vma_merge_new_vma() to use vma_merge_struct

[Petr Tesařík]

On Mon,  5 Aug 2024 13:13:52 +0100
Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx> wrote:

> Abstract this function to so we can write tests which use the newly
> abstracted interface and maintain a stable interface for tests before/after
> refactoring.
> 
> We introduce a temporary wrapper vma_merge_new_vma_wrapper() to minimise
> the code changes, in a subsequent commit we will entirely refactor this
> function.
> 
> We also introduce a temporary implementation of vma_merge_modified() for
> the same reason - maintaining a common interface to the tests, this will be
> removed when vma_merge_modified() is correctly implemented in a subsequent
> commit.
> 
> Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
>  mm/mmap.c               |  6 +++---
>  mm/vma.c                | 33 ++++++++++++---------------------
>  mm/vma.h                | 33 ++++++++++++++++++++++++++++++---
>  tools/testing/vma/vma.c | 12 ++++++++----
>  4 files changed, 53 insertions(+), 31 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 04145347c245..f6593a81f73d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1494,9 +1494,9 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
>  		 * vma again as we may succeed this time.
>  		 */
>  		if (unlikely(vm_flags != vma->vm_flags && prev)) {
> -			merge = vma_merge_new_vma(&vmi, prev, vma,
> -						  vma->vm_start, vma->vm_end,
> -						  vma->vm_pgoff);
> +			merge = vma_merge_new_vma_wrapper(&vmi, prev, vma,
> +							  vma->vm_start, vma->vm_end,
> +							  vma->vm_pgoff);
>  			if (merge) {
>  				/*
>  				 * ->mmap() can change vma->vm_file and fput
> diff --git a/mm/vma.c b/mm/vma.c
> index 3d6ce04f1b9c..55615392e8d2 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -1106,6 +1106,11 @@ static struct vm_area_struct *vma_merge(struct vma_merge_struct *vmg)
>  	return NULL;
>  }
> 
> +struct vm_area_struct *vma_merge_modified(struct vma_merge_struct *vmg)
> +{
> +	return vma_merge(vmg);
> +}
> +
>  /*
>   * We are about to modify one or multiple of a VMA's flags, policy, userfaultfd
>   * context and anonymous VMA name within the range [start, end).
> @@ -1260,27 +1265,14 @@ struct vm_area_struct
>   * Attempt to merge a newly mapped VMA with those adjacent to it. The caller
>   * must ensure that [start, end) does not overlap any existing VMA.
>   */
> -struct vm_area_struct
> -*vma_merge_new_vma(struct vma_iterator *vmi, struct vm_area_struct *prev,
> -		   struct vm_area_struct *vma, unsigned long start,
> -		   unsigned long end, pgoff_t pgoff)
> +struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
>  {
> -	struct vma_merge_struct vmg = {
> -		.vmi = vmi,
> -		.prev = prev,
> -		.vma = vma,
> -		.start = start,
> -		.end = end,
> -		.flags = vma->vm_flags,
> -		.file = vma->vm_file,
> -		.anon_vma = vma->anon_vma,
> -		.pgoff = pgoff,
> -		.policy = vma_policy(vma),
> -		.uffd_ctx = vma->vm_userfaultfd_ctx,
> -		.anon_name = anon_vma_name(vma),
> -	};
> +	if (!vmg->prev) {
> +		vmg->prev = vma_prev(vmg->vmi);
> +		vma_iter_set(vmg->vmi, vmg->start);
> +	}
> 
> -	return vma_merge(&vmg);
> +	return vma_merge(vmg);
>  }
> 
>  /*
> @@ -1295,7 +1287,6 @@ struct vm_area_struct *vma_merge_extend(struct vma_iterator *vmi,
>  	struct vma_merge_struct vmg = {
>  		.vmi = vmi,
>  		.prev = vma,
> -		.vma = vma,

Yes, this member is not used later by vma_merge(), so it need not be
initialized. What about not adding this line in PATCH 02/10 of this
series? AFAICS vmg->vma was never used by vma_merge(). The net result
is the same, but it would make it easier to understand that this patch
in the series does not change the use of vmg->vma by vma_merge_extend().

Petr T