| Message ID | 20241024093347.18057-1-richard.weiyang@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/vma: the pgoff is correct if can_merge_right | expand |
On Thu, Oct 24, 2024 at 09:33:47AM +0000, Wei Yang wrote: > 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. > > Below is a more detail explanation. > > Current definition of can_vma_merge_right() is: > > 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. > > [lorenzo: rephrase the change log] Thanks for this, LGTM! > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > --- > v2: rephrase the change log per Lorenzo's suggestion. > --- > 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 > >
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. */
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. Below is a more detail explanation. Current definition of can_vma_merge_right() is: 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. [lorenzo: rephrase the change log] Signed-off-by: Wei Yang <richard.weiyang@gmail.com> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> --- v2: rephrase the change log per Lorenzo's suggestion. --- mm/vma.c | 2 -- 1 file changed, 2 deletions(-)