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@xxxxxxxxx>
> >> CC: Liam R. Howlett <Liam.Howlett@xxxxxxxxxx>
> >> CC: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> >> CC: Vlastimil Babka <vbabka@xxxxxxx>
> >> CC: Jann Horn <jannh@xxxxxxxxxx>
> >> ---
> >> 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
>
