| Message ID | 20241127114302.9650-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vma: next is already retrieved | expand |
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
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
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 >
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) {
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(-)