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

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

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.

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.

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!

> CC: Liam R. Howlett <Liam.Howlett@Oracle.com>
> CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> CC: Vlastimil Babka <vbabka@suse.cz>
> CC: Jann Horn <jannh@google.com>
> ---
>  mm/vma.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 8a454a7bbc80..80b1bd404f23 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -2456,14 +2456,14 @@ unsigned long __mmap_region(struct file *file, unsigned long addr,
>  		error = __mmap_new_vma(&map, &vma);
>  		if (error)
>  			goto unacct_error;
> -	}
>
> -	/* If flags changed, we might be able to merge, so try again. */
> -	if (map.retry_merge) {
> -		VMG_MMAP_STATE(vmg, &map, vma);
> +		/* If flags changed, we might be able to merge, so try again. */
> +		if (map.retry_merge) {
> +			VMG_MMAP_STATE(vmg, &map, vma);
>
> -		vma_iter_config(map.vmi, map.addr, map.end);
> -		vma_merge_existing_range(&vmg);
> +			vma_iter_config(map.vmi, map.addr, map.end);
> +			vma_merge_existing_range(&vmg);
> +		}
>  	}
>
>  	__mmap_complete(&map, vma);
> --
> 2.34.1
>
>