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

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

On Mon, Nov 18, 2024 at 12:52:48PM +0000, Lorenzo Stoakes wrote:
>On Mon, Nov 18, 2024 at 12:34:39PM +0000, Wei Yang wrote:
>> 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.
>
>By this logic we should not split up the functions any more, because the
>reader won't know that it's only if you create a new VMA AND it's
>file-backed AND that succeeded AND etc. etc. etc.
>
>So by your logic, we should undo every refactoring here since we need the
>reader to understand precisely the conditions under which this can occur
>right?
>
>Or where do we stop? Because now it's 'misleading' to imply it might always
>happen in this situation without stipulating that there are other
>conditions?
>
>Do you see how this breaks down?
>
>We encapsulate the circumstances under which a retry_merge occurs, using
>the map.retry_merge variable. That's it.
>
>It's simple, it documents itself, and a reader wishing to know when this
>happens can very easily find out.
>
>The purpose of this code is - again - not to be as small as possible or to
>avoid all possible branches (unless perf numbers guide us to do so) - it's
>to be readable.
>
>In the original refactoring I can see:
>
>- prepare for mmap
>- try to merge
>- if merge failed, create new
>- maybe do a deferred merge
>- mmap complete, do 'after mmap' tasks.

Thanks for your explanation. This looks neat.

So it hide the reason of deferred merge from user, we would think both two
cases would lead to a deferred merge. This is what we want to have, right?

>
>In your proposed code I see:
>
>- prepare for mmap
>- try to merge
>- if merge failed, create new
>  - if merge failed, created new, and we required a deferred merge, do a deferred merge

Maybe here is

   - maybe do a deferred merge

>- mmap complete, do 'after mmap' tasks.
>
>It's about humans reading this and being able to understand what is going
>on.
>
-- 
Wei Yang
Help you, Help me