Re: [PATCH] mm/vma: next is already retrieved

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

Hi Wei,

Thanks for the patch, though I would again ask if you could please stop
fiddling around with the VMA stuff until things are a _little_ more
settled, perhaps best to leave until the new year?

I'm talking about small refactorings and optimisations here - for bugs or
major issues - please do, of course, submit immediately.

On Wed, Nov 27, 2024 at 11:43:02AM +0000, Wei Yang wrote:
> vma_iter_next_rewind() here gets next vma and rewind iterator.
>
> Actually the next vma is already been retrieved to new_vma. Since the
> iterator is initialized to addr, we don't need to adjust it.
>
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Vlastimil Babka <vbabka@suse.cz>
> CC: Jann Horn <jannh@google.com>
> ---
>  mm/vma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 8a454a7bbc80..d419e3700fa7 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -1727,7 +1727,7 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
>
>  	vmg.vma = NULL; /* New VMA range. */
>  	vmg.pgoff = pgoff;
> -	vmg.next = vma_iter_next_rewind(&vmi, NULL);
> +	vmg.next = new_vma;

HOWEVER, this is a good spot and you're right (I mean we explicitly _check_
that the found VMA is after the point at which we intend to insert the new
one).

But while we're here, can we fix this HORRIBLE variable naming? We reuse
'new_vma' to refer to the _next_ VMA and newly merged one/one we have to
create.

So could you update your code to do something like:

	vmg.next = find_vma_prev(mm, addr, &vmg.prev);
	if (vmg.next && ...

This self-documents your fix, avoids the need for the
vma_iter_next_rewind(), fixes the inefficiency you've found, and avoids
stupidly overloading what new_vma refers to.

Also update the commit message to refer to this.

And can you update the test_copy_vma() test in tools/testing/vma/vma.c to
explicitly test for VMAs that come afterwards both merge and non-merge
cases?

It's time to really start taking advantage of this huge power we have to
userland test things! :)

>  	new_vma = vma_merge_new_range(&vmg);
>
>  	if (new_vma) {
> --
> 2.34.1
>
>

Thanks, Lorenzo