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

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

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@gmail.com> [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
>