linux-kernel - Re: [PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <71b7cbeb-1301-491c-9637-e6d48938ddaf@lucifer.local>
Date: Fri, 4 Oct 2024 18:01:07 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Breno Leitao <leitao@...ian.org>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
        "Liam R. Howlett" <Liam.Howlett@...cle.com>,
        Vlastimil Babka <vbabka@...e.cz>, kernel-team@...a.com,
        "open list:VMA" <linux-mm@...ck.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] mm: Remove misleading 'unlikely' hint in
 vms_gather_munmap_vmas()

On Fri, Oct 04, 2024 at 09:48:31AM -0700, Breno Leitao wrote:
> Performance analysis using branch annotation on a fleet of 200 hosts
> running web servers revealed that the 'likely' hint in

To be pedantic: *unlikely

> vms_gather_munmap_vmas() was 100% consistently incorrect. In all
> observed cases, the branch behavior contradicted the hint.

OK so this is probably because vm_mmap_pgoff() declares the userfaultfd
list head on the stack then passes it into do_mmap() and threads all the
way to this code... and yeah, so that would be 100%.

There are other code paths that aren't 100%, but the system call one is.

Nice spot!

>
> Remove the 'unlikely' qualifier from the condition checking 'vms->uf'.
> By doing so, we allow the compiler to make optimization decisions based
> on its own heuristics and profiling data, rather than relying on a
> static hint that has proven to be inaccurate in real-world scenarios.

Yeah I'm generally not in favour of 'vibes' based likely()/unlikely(), I
think it should always be based on profiling.

It's understandable that there would be this expectation, and it may have
migrated from other code that already had this check in where perhaps it
wasn't always referencing a stack object, but yeah this is just wrong.

>
> Signed-off-by: Breno Leitao <leitao@...ian.org>

Liam will want a look too when he's back next week.

Looks good to me though!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

> ---
>  mm/vma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/mm/vma.c b/mm/vma.c
> index 4737afcb064c..9d4fe794dd07 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -1250,7 +1250,7 @@ int vms_gather_munmap_vmas(struct vma_munmap_struct *vms,
>  		else if (is_data_mapping(next->vm_flags))
>  			vms->data_vm += nrpages;
>
> -		if (unlikely(vms->uf)) {
> +		if (vms->uf) {
>  			/*
>  			 * If userfaultfd_unmap_prep returns an error the vmas
>  			 * will remain split, but userland will get a
> --
> 2.43.5
>