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

[Vlastimil Babka (SUSE)]

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

<snip>

> +struct vm_area_struct *vma_merge_new_vma(struct vma_merge_struct *vmg)
>  {
> -	struct vma_merge_struct vmg = {
> -		.vmi = vmi,
> -		.prev = prev,
> -		.vma = vma,

This line not being addded in the wrapper felt like a mistake. I see in the
other subthread that it's not so I'll just support your decision to tackle
it so the code is less surprising.

> -		.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);
> +	}

Admit this is another surprise. The old code didn't do it anywhere AFAICS so
I don't see why it's now done. Maybe it's necessary for futher changes.
Could you add a comment or explain it in the changelog, please?

> -	return vma_merge(&vmg);
> +	return vma_merge(vmg);
>  }
> 
>  /*