[PATCH] perf: Fix the mlock accounting, again

[PATCH] perf: Fix the mlock accounting, again

[Alexander Shishkin]

Commit

  5e6c3c7b1ec2 ("perf/aux: Fix tracking of auxiliary trace buffer allocation")

tried to guess the correct combination of arithmetic operations that would
undo the AUX buffer's mlock accounting, and failed, leaking the bottom part
when an allocation needs to be changed partially to both user->locked_vm
and mm->pinned_vm, eventually leaving the user with no locked bonus:

$ perf record -e intel_pt//u -m1,128 uname
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 0.061 MB perf.data ]
$ perf record -e intel_pt//u -m1,128 uname
Permission error mapping pages.
Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
or try again with a smaller value of -m/--mmap_pages.
(current value: 1,128)

Fix this by subtracting both locked and pinned counts when AUX buffer is
unmapped.

Signefdoff-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 kernel/events/core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 16d80ad8d6d7..522438887206 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5664,10 +5664,8 @@ static void perf_mmap_close(struct vm_area_struct *vma)
 		perf_pmu_output_stop(event);
 
 		/* now it's safe to free the pages */
-		if (!rb->aux_mmap_locked)
-			atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm);
-		else
-			atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
+		atomic_long_sub(rb->aux_nr_pages - rb->aux_mmap_locked, &mmap_user->locked_vm);
+		atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
 
 		/* this has to be the last one */
 		rb_free_aux(rb);
-- 
2.24.0

Re: [PATCH] perf: Fix the mlock accounting, again

[Alexander Shishkin]

Alexander Shishkin <alexander.shishkin@linux.intel.com> writes:

> Commit
>
>   5e6c3c7b1ec2 ("perf/aux: Fix tracking of auxiliary trace buffer allocation")
>
> tried to guess the correct combination of arithmetic operations that would
> undo the AUX buffer's mlock accounting, and failed, leaking the bottom part
> when an allocation needs to be changed partially to both user->locked_vm

s/changed/charged/

Re: [PATCH] perf: Fix the mlock accounting, again

[Thomas Richter]

On 11/15/19 5:08 PM, Alexander Shishkin wrote:
> Commit
> 
>   5e6c3c7b1ec2 ("perf/aux: Fix tracking of auxiliary trace buffer allocation")
> 
> tried to guess the correct combination of arithmetic operations that would
> undo the AUX buffer's mlock accounting, and failed, leaking the bottom part
> when an allocation needs to be changed partially to both user->locked_vm
> and mm->pinned_vm, eventually leaving the user with no locked bonus:
> 
> $ perf record -e intel_pt//u -m1,128 uname
> [ perf record: Woken up 1 times to write data ]
> [ perf record: Captured and wrote 0.061 MB perf.data ]
> $ perf record -e intel_pt//u -m1,128 uname
> Permission error mapping pages.
> Consider increasing /proc/sys/kernel/perf_event_mlock_kb,
> or try again with a smaller value of -m/--mmap_pages.
> (current value: 1,128)
> 
> Fix this by subtracting both locked and pinned counts when AUX buffer is
> unmapped.
> 
> Signefdoff-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  kernel/events/core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 16d80ad8d6d7..522438887206 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -5664,10 +5664,8 @@ static void perf_mmap_close(struct vm_area_struct *vma)
>  		perf_pmu_output_stop(event);
>  
>  		/* now it's safe to free the pages */
> -		if (!rb->aux_mmap_locked)
> -			atomic_long_sub(rb->aux_nr_pages, &mmap_user->locked_vm);
> -		else
> -			atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
> +		atomic_long_sub(rb->aux_nr_pages - rb->aux_mmap_locked, &mmap_user->locked_vm);
> +		atomic64_sub(rb->aux_mmap_locked, &vma->vm_mm->pinned_vm);
>  
>  		/* this has to be the last one */
>  		rb_free_aux(rb);
> 

I have tested your patch on our test suite which discovered the original issue.
Your patch works nicely, thanks for fixing this.

-- 
Thomas Richter, Dept 3252, IBM s390 Linux Development, Boeblingen, Germany
--
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen / Registergericht: Amtsgericht Stuttgart, HRB 243294