Re: [PATCH] mm/vma: the pgoff is correct if can_merge_right

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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.