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@xxxxxxxxx>
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> CC: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
> ---
> 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.
