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@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. 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. 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 - mmap complete, do 'after mmap' tasks. It's about humans reading this and being able to understand what is going on. > > 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? I don't know what a 'universal check state' is? You mean we check it regardless of whether we previously merged or not, well obviously I do want to do that, but for readability reasons as outlined above, and previously. > > >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 > I would strongly suggest you look at mmap_region() prior to my refactorings (say in v6.11) and after, and honestly assess whether you think the new version is more readable than the previous one. And consider that we have run into _countless_ bugs because of this complexity, much of which very much tried to do exactly what you are trying to do here. Again, we appreciate your help, but I strongly suggest you focus on bugs and performance optimisations (backed by numbers), rather than nitpicking things like this when the code is incredibly new.
