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