Re: [PATCH] mm/vma: check retry_merge only for new vma case

[Wei Yang <richard.weiyang@gmail.com>]

On Mon, Nov 18, 2024 at 10:04:44AM +0000, Lorenzo Stoakes wrote:
>On Mon, Nov 18, 2024 at 02:18:23AM +0000, Wei Yang wrote:
>> Current code logic looks like this:
>>
>> __mmap_region()
>>   vma = vma_merge_new_range(&vmg)
>>   if (!vma)
>>     __mmap_new_vma(&map, &vma)
>>       __mmap_new_file_vma(map, vma)
>>         map->retry_merge = xxx               --- (1)
>>   if (map.retry_merge)
>>     vma_merge_existing_range(vmg, &map, vma)
>>
>> Location (1) is the only place where map.retry_merge is set, this means
>> it is not necessary to check it if already merged with adjacent vma.
>>
>> Let's move the check and following operation into new vma case.
>>
>> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
>
>Nack.
>
>As I said to you in another patch, the aim isn't to find the mathematically
>smallest size of patch. The aim is to keep things readable and nesting
>things like this works against that aim.
>

IMHO, by nesting it, readers will notice this case only happens for new vma
case. So we don't need to retry if vma_merge_new_range() succeed.

It seems to be more readable, but maybe you have other thoughts I missed. If
you would like to share some thoughts, I'd appreciate.

>Unless you can demonstrate a _significant_ perf improvement for not
>checking a boolean here, then you're making the code more complicated than
>it needs to be for no gain.
>
>Right now it's (intentionally!) simple - try to merge, ok we can't, try to
>map a new VMA. Finally, if the state says retry a merge, do it.
>

So you want to make retry_merge a universal check state?

>It's easy to read and easy to understand.
>
>Nesting as you say takes away from that.
>
>I also could have gone through this mmap() code and tried to reach some
>mathematically perfect level of avoiding unnecessary things, but as this
>wasn't the aim, I didn't.
>
>Please try to focus on finding bugs, or _demonstrable_ perf issues rather
>than stuff like this.
>
>Thanks!
>

-- 
Wei Yang
Help you, Help me