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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Aug 08, 2024 at 04:55:07PM GMT, Vlastimil Babka (SUSE) wrote:
> 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@oracle.com>
>
> <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.

This is just a wrapper that gets replaced in a subsequent commit.

For new VMAs we don't strictly need vma, as vma_merge() no longer references
this.

This is purposeful for testing purposes.

>
> > -		.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?

So in the future (actual non-wrapper) version of vma_merge_new_vma(), we do not
require that the previous VMA is specified.

vma_merge() _does_ require this. So we have to go look it up if it's not there.

This is so we can add the tests in the next commit, and have them be completely
identical for the functions _before_ the refactor and afterwards so we can
assert that they pass in both cases.

I'll add a comment accordingly.

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