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

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

On Thu, Nov 28, 2024 at 01:10:59AM +0000, Wei Yang wrote:
> On Wed, Nov 27, 2024 at 12:21:24PM +0000, Lorenzo Stoakes wrote:
> >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?
> >
>
> Sure.

Thanks.

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

Though if you find any bugs or serious problems, please do report them
right away!

> >
> >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 && ...
> >
>
> Sure. I thought you may prefer a small footprint, so I did current version.
>
> Will update it in next version next year.

Thanks!

>
> >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?
> >
>
> Currently we have two scenario with merge/non-merge after copy_vma().
>
> I am not sure what exact case you want to have. Would you mind giving more
> detail?

Both please, I don't believe this is curently tested.

It may also be worth testing the 'should never get here' error case.

>
> >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
>
> --
> Wei Yang
> Help you, Help me
>