[PATCH] mm/vma: next is already retrieved

[PATCH] mm/vma: next is already retrieved

[Wei Yang]

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;
 	new_vma = vma_merge_new_range(&vmg);
 
 	if (new_vma) {
-- 
2.34.1

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

[Lorenzo Stoakes]

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

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

[Wei Yang]

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

>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

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

[Lorenzo Stoakes]

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
>

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

[Wei Yang]

On Thu, Nov 28, 2024 at 03:48:48PM +0000, Lorenzo Stoakes wrote:
>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.
>

Hi, Lorenzo

Is it proper to resend it now?

-- 
Wei Yang
Help you, Help me

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

[Lorenzo Stoakes]

On Wed, Jan 22, 2025 at 09:49:49AM +0000, Wei Yang wrote:
> On Thu, Nov 28, 2024 at 03:48:48PM +0000, Lorenzo Stoakes wrote:
> >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.
> >
>
> Hi, Lorenzo
>
> Is it proper to resend it now?

Hi Wei,

Would you mind waiting a little longer? Sorry to be a pain but I have a
refactoring that I'm going to push through shortly! Otherwise we will clash
:)

Cheers, Lorenzo

>
> --
> Wei Yang
> Help you, Help me
>

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

[Wei Yang]

On Wed, Jan 22, 2025 at 04:15:25PM +0000, Lorenzo Stoakes wrote:
>On Wed, Jan 22, 2025 at 09:49:49AM +0000, Wei Yang wrote:
>> On Thu, Nov 28, 2024 at 03:48:48PM +0000, Lorenzo Stoakes wrote:
>> >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.
>> >
>>
>> Hi, Lorenzo
>>
>> Is it proper to resend it now?
>
>Hi Wei,
>
>Would you mind waiting a little longer? Sorry to be a pain but I have a
>refactoring that I'm going to push through shortly! Otherwise we will clash
>:)

No problem. Would you mind letting me know when it is ready?

>
>Cheers, Lorenzo
>
>>
>> --
>> Wei Yang
>> Help you, Help me
>>

-- 
Wei Yang
Help you, Help me

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

[Lorenzo Stoakes]

On Thu, Jan 23, 2025 at 01:47:50AM +0000, Wei Yang wrote:
> On Wed, Jan 22, 2025 at 04:15:25PM +0000, Lorenzo Stoakes wrote:
> >On Wed, Jan 22, 2025 at 09:49:49AM +0000, Wei Yang wrote:
> >> On Thu, Nov 28, 2024 at 03:48:48PM +0000, Lorenzo Stoakes wrote:
> >> >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.
> >> >
> >>
> >> Hi, Lorenzo
> >>
> >> Is it proper to resend it now?
> >
> >Hi Wei,
> >
> >Would you mind waiting a little longer? Sorry to be a pain but I have a
> >refactoring that I'm going to push through shortly! Otherwise we will clash
> >:)
>
> No problem. Would you mind letting me know when it is ready?

Sure will do, once it's taken to unstable then we should be good to
revisit. Sorry to be a pain and thank you very much for your understanding!

I am working on this now so shouldn't be long.

Cheers, Lorenzo

>
> >
> >Cheers, Lorenzo
> >
> >>
> >> --
> >> Wei Yang
> >> Help you, Help me
> >>
>
> --
> Wei Yang
> Help you, Help me
>