On Mon, Nov 18, 2024 at 09:31:09AM +0000, Wei Yang wrote: > On Sun, Nov 17, 2024 at 10:56:55PM -0500, Liam R. Howlett wrote: > >* Wei Yang <richard.weiyang@xxxxxxxxx> [241117 21:21]: > >> 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) > > > >Please don't quote code in your commit log. We can see the code in the > >diff section. > > > > Sure, maybe I misunderstand Lorenzo's suggestion in pre-previous review. > > Will not add these in change log in the future. > > >> > >> 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. > > > >This makes sense, but this is a complex block of code. > > > >I'm all for optimisations, but there is already a bug in the code that > >you relocated in your patch, and the backport of these changes isn't > >even complete. > > > >Maybe we can give the existing code some time to soak before optimising? > > > > Sure, when is a proper time to re-send it if everything is fine? I've NACKed, so this patch isn't something we want, but to highlight - ideally when new things like this have been introduced that fundamentally change core stuff, keep in mind we may be very busy ensuring it settles down correctly, and really do not want to be constantly reorganising the code. So for things that are rejigs or small improvements, it's better to wait until the thing has landed in mainline and fully settled down. Bugs of course we want to know about asap, and your bug reports are much appreciated! Thanks. > > > -- > Wei Yang > Help you, Help me >
