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@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.
Do you suggest me to add this in change log?
--
Wei Yang
Help you, Help me
