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