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

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

[Wei Yang]

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(-)

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

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

[Lorenzo Stoakes]

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.

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

[Wei Yang]

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?

-- 
Wei Yang
Help you, Help me

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

[Lorenzo Stoakes]

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
>

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

[Wei Yang]

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
>>

-- 
Wei Yang
Help you, Help me