[PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[Breno Leitao]

Performance analysis using branch annotation on a fleet of 200 hosts
running web servers revealed that the 'likely' hint in
vms_gather_munmap_vmas() was 100% consistently incorrect. In all
observed cases, the branch behavior contradicted the hint.

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.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 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

Re: [PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[Lorenzo Stoakes]

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@debian.org>

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

Looks good to me though!

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.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
>

Re: [PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[Vlastimil Babka]

On 10/4/24 6:48 PM, Breno Leitao wrote:
> Performance analysis using branch annotation on a fleet of 200 hosts
> running web servers revealed that the 'likely' hint in
> vms_gather_munmap_vmas() was 100% consistently incorrect. In all
> observed cases, the branch behavior contradicted the hint.
> 
> 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.
> 
> Signed-off-by: Breno Leitao <leitao@debian.org>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  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

Re: [PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[Breno Leitao]

On Fri, Oct 04, 2024 at 06:01:07PM +0100, Lorenzo Stoakes wrote:
> 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@debian.org>
> 
> Liam will want a look too when he's back next week.

Liam, are you OK with this one? I suspect that Andrew is waiting for
your review before merging it.

> 
> Looks good to me though!
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.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
> >

Re: [PATCH] mm: Remove misleading 'unlikely' hint in vms_gather_munmap_vmas()

[Liam R. Howlett]

* Breno Leitao <leitao@debian.org> [241029 07:52]:
> On Fri, Oct 04, 2024 at 06:01:07PM +0100, Lorenzo Stoakes wrote:
> > 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@debian.org>
> > 
> > Liam will want a look too when he's back next week.
> 
> Liam, are you OK with this one? I suspect that Andrew is waiting for
> your review before merging it.

I'm fine with removing the unlikely.

This begs for a refactoring considering we are calling a function in
fs/userfaultfd on every vma on every munmap operation in this call path.

Reviewed-by: Liam R. Howlett <Liam.Howlett@oracle.com>

> 
> > 
> > Looks good to me though!
> > 
> > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.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
> > >