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