| Message ID | 20241024084222.17201-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/vma: the pgoff is correct if can_merge_right | expand |
On Thu, Oct 24, 2024 at 08:42:22AM +0000, Wei Yang wrote: > can_merge_right implies can_vma_merge_right() has checked the pgoff. > > Don't need to assign it again. Would prefer a bigger commit message something like: By this point can_vma_merge_right() must have returned true, which implies can_vma_merge_before() also returned true, which already asserts that the pgoff is as expected for a merge with the following VMA, thus this assignment is redundant. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > mm/vma.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/mm/vma.c b/mm/vma.c > index 4737afcb064c..fb4f1863f88e 100644 > --- a/mm/vma.c > +++ b/mm/vma.c > @@ -915,7 +915,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > unsigned long start = vmg->start; > unsigned long end = vmg->end; > pgoff_t pgoff = vmg->pgoff; > - pgoff_t pglen = PHYS_PFN(end - start); > bool can_merge_left, can_merge_right; > > mmap_assert_write_locked(vmg->mm); > @@ -936,7 +935,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > if (can_merge_right) { > vmg->end = next->vm_end; > vmg->vma = next; > - vmg->pgoff = next->vm_pgoff - pglen; > } > > /* If we can merge with the previous VMA, adjust vmg accordingly. */ > -- > 2.34.1 > > Thanks, nice spot! For the purposes of explaining it on-list this is because: static bool can_vma_merge_right(struct vma_merge_struct *vmg, bool can_merge_left) { if (!vmg->next || vmg->end != vmg->next->vm_start || !can_vma_merge_before(vmg)) return false; ... } And: static bool can_vma_merge_before(struct vma_merge_struct *vmg) { pgoff_t pglen = PHYS_PFN(vmg->end - vmg->start); ... if (vmg->next->vm_pgoff == vmg->pgoff + pglen) return true; ... } Which implies vmg->pgoff == vmg->next->vm_pgoff - pglen. None of these values are changed between the check and prior assignment, so this was an entirely redundant assignment.
On Thu, Oct 24, 2024 at 10:03:34AM +0100, Lorenzo Stoakes wrote: >On Thu, Oct 24, 2024 at 08:42:22AM +0000, Wei Yang wrote: >> can_merge_right implies can_vma_merge_right() has checked the pgoff. >> >> Don't need to assign it again. > >Would prefer a bigger commit message something like: > >By this point can_vma_merge_right() must have returned true, which implies >can_vma_merge_before() also returned true, which already asserts that the >pgoff is as expected for a merge with the following VMA, thus this >assignment is redundant. > Will change to this in next version. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > >> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> >> --- >> mm/vma.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/mm/vma.c b/mm/vma.c >> index 4737afcb064c..fb4f1863f88e 100644 >> --- a/mm/vma.c >> +++ b/mm/vma.c >> @@ -915,7 +915,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> unsigned long start = vmg->start; >> unsigned long end = vmg->end; >> pgoff_t pgoff = vmg->pgoff; >> - pgoff_t pglen = PHYS_PFN(end - start); >> bool can_merge_left, can_merge_right; >> >> mmap_assert_write_locked(vmg->mm); >> @@ -936,7 +935,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> if (can_merge_right) { >> vmg->end = next->vm_end; >> vmg->vma = next; >> - vmg->pgoff = next->vm_pgoff - pglen; >> } >> >> /* If we can merge with the previous VMA, adjust vmg accordingly. */ >> -- >> 2.34.1 >> >> > >Thanks, nice spot! > >For the purposes of explaining it on-list this is because: > >static bool can_vma_merge_right(struct vma_merge_struct *vmg, > bool can_merge_left) >{ > if (!vmg->next || vmg->end != vmg->next->vm_start || > !can_vma_merge_before(vmg)) > return false; > ... >} > >And: > >static bool can_vma_merge_before(struct vma_merge_struct *vmg) >{ > pgoff_t pglen = PHYS_PFN(vmg->end - vmg->start); >... > if (vmg->next->vm_pgoff == vmg->pgoff + pglen) > return true; >... >} > >Which implies vmg->pgoff == vmg->next->vm_pgoff - pglen. > >None of these values are changed between the check and prior assignment, so >this was an entirely redundant assignment. Do you suggest me to add this in change log?
On Thu, Oct 24, 2024 at 09:10:12AM +0000, Wei Yang wrote: > On Thu, Oct 24, 2024 at 10:03:34AM +0100, Lorenzo Stoakes wrote: > >On Thu, Oct 24, 2024 at 08:42:22AM +0000, Wei Yang wrote: > >> can_merge_right implies can_vma_merge_right() has checked the pgoff. > >> > >> Don't need to assign it again. > > > >Would prefer a bigger commit message something like: > > > >By this point can_vma_merge_right() must have returned true, which implies > >can_vma_merge_before() also returned true, which already asserts that the > >pgoff is as expected for a merge with the following VMA, thus this > >assignment is redundant. > > > > Will change to this in next version. Thanks. The actual change itself looks good! > > >> > >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > > > >Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > >> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > >> --- > >> mm/vma.c | 2 -- > >> 1 file changed, 2 deletions(-) > >> > >> diff --git a/mm/vma.c b/mm/vma.c > >> index 4737afcb064c..fb4f1863f88e 100644 > >> --- a/mm/vma.c > >> +++ b/mm/vma.c > >> @@ -915,7 +915,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > >> unsigned long start = vmg->start; > >> unsigned long end = vmg->end; > >> pgoff_t pgoff = vmg->pgoff; > >> - pgoff_t pglen = PHYS_PFN(end - start); > >> bool can_merge_left, can_merge_right; > >> > >> mmap_assert_write_locked(vmg->mm); > >> @@ -936,7 +935,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) > >> if (can_merge_right) { > >> vmg->end = next->vm_end; > >> vmg->vma = next; > >> - vmg->pgoff = next->vm_pgoff - pglen; > >> } > >> > >> /* If we can merge with the previous VMA, adjust vmg accordingly. */ > >> -- > >> 2.34.1 > >> > >> > > > >Thanks, nice spot! > > > >For the purposes of explaining it on-list this is because: > > > >static bool can_vma_merge_right(struct vma_merge_struct *vmg, > > bool can_merge_left) > >{ > > if (!vmg->next || vmg->end != vmg->next->vm_start || > > !can_vma_merge_before(vmg)) > > return false; > > ... > >} > > > >And: > > > >static bool can_vma_merge_before(struct vma_merge_struct *vmg) > >{ > > pgoff_t pglen = PHYS_PFN(vmg->end - vmg->start); > >... > > if (vmg->next->vm_pgoff == vmg->pgoff + pglen) > > return true; > >... > >} > > > >Which implies vmg->pgoff == vmg->next->vm_pgoff - pglen. > > > >None of these values are changed between the check and prior assignment, so > >this was an entirely redundant assignment. > > Do you suggest me to add this in change log? Sure, I am a big believer in putting as much detail as possible in commit messages, we definitely need to explain why we're doing this to future observers (including me... ;) > > -- > Wei Yang > Help you, Help me >
On Thu, Oct 24, 2024 at 10:18:33AM +0100, Lorenzo Stoakes wrote: >On Thu, Oct 24, 2024 at 09:10:12AM +0000, Wei Yang wrote: >> On Thu, Oct 24, 2024 at 10:03:34AM +0100, Lorenzo Stoakes wrote: >> >On Thu, Oct 24, 2024 at 08:42:22AM +0000, Wei Yang wrote: >> >> can_merge_right implies can_vma_merge_right() has checked the pgoff. >> >> >> >> Don't need to assign it again. >> > >> >Would prefer a bigger commit message something like: >> > >> >By this point can_vma_merge_right() must have returned true, which implies >> >can_vma_merge_before() also returned true, which already asserts that the >> >pgoff is as expected for a merge with the following VMA, thus this >> >assignment is redundant. >> > >> >> Will change to this in next version. > >Thanks. The actual change itself looks good! > >> >> >> >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> > >> >Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> >> > >> >> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> >> >> --- >> >> mm/vma.c | 2 -- >> >> 1 file changed, 2 deletions(-) >> >> >> >> diff --git a/mm/vma.c b/mm/vma.c >> >> index 4737afcb064c..fb4f1863f88e 100644 >> >> --- a/mm/vma.c >> >> +++ b/mm/vma.c >> >> @@ -915,7 +915,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> >> unsigned long start = vmg->start; >> >> unsigned long end = vmg->end; >> >> pgoff_t pgoff = vmg->pgoff; >> >> - pgoff_t pglen = PHYS_PFN(end - start); >> >> bool can_merge_left, can_merge_right; >> >> >> >> mmap_assert_write_locked(vmg->mm); >> >> @@ -936,7 +935,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) >> >> if (can_merge_right) { >> >> vmg->end = next->vm_end; >> >> vmg->vma = next; >> >> - vmg->pgoff = next->vm_pgoff - pglen; >> >> } >> >> >> >> /* If we can merge with the previous VMA, adjust vmg accordingly. */ >> >> -- >> >> 2.34.1 >> >> >> >> >> > >> >Thanks, nice spot! >> > >> >For the purposes of explaining it on-list this is because: >> > >> >static bool can_vma_merge_right(struct vma_merge_struct *vmg, >> > bool can_merge_left) >> >{ >> > if (!vmg->next || vmg->end != vmg->next->vm_start || >> > !can_vma_merge_before(vmg)) >> > return false; >> > ... >> >} >> > >> >And: >> > >> >static bool can_vma_merge_before(struct vma_merge_struct *vmg) >> >{ >> > pgoff_t pglen = PHYS_PFN(vmg->end - vmg->start); >> >... >> > if (vmg->next->vm_pgoff == vmg->pgoff + pglen) >> > return true; >> >... >> >} >> > >> >Which implies vmg->pgoff == vmg->next->vm_pgoff - pglen. >> > >> >None of these values are changed between the check and prior assignment, so >> >this was an entirely redundant assignment. >> >> Do you suggest me to add this in change log? > >Sure, I am a big believer in putting as much detail as possible in commit >messages, we definitely need to explain why we're doing this to future >observers (including me... ;) > Sure, will arrange to put these in change log too. >> >> -- >> Wei Yang >> Help you, Help me >>
diff --git a/mm/vma.c b/mm/vma.c index 4737afcb064c..fb4f1863f88e 100644 --- a/mm/vma.c +++ b/mm/vma.c @@ -915,7 +915,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) unsigned long start = vmg->start; unsigned long end = vmg->end; pgoff_t pgoff = vmg->pgoff; - pgoff_t pglen = PHYS_PFN(end - start); bool can_merge_left, can_merge_right; mmap_assert_write_locked(vmg->mm); @@ -936,7 +935,6 @@ struct vm_area_struct *vma_merge_new_range(struct vma_merge_struct *vmg) if (can_merge_right) { vmg->end = next->vm_end; vmg->vma = next; - vmg->pgoff = next->vm_pgoff - pglen; } /* If we can merge with the previous VMA, adjust vmg accordingly. */
can_merge_right implies can_vma_merge_right() has checked the pgoff. Don't need to assign it again. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- mm/vma.c | 2 -- 1 file changed, 2 deletions(-)