[PATCH 1/2] perf/aux: Make aux_{head,wakeup} ring_buffer members long

[PATCH 1/2] perf/aux: Make aux_{head,wakeup} ring_buffer members long

[Will Deacon]

The aux_head and aux_wakeup members of struct ring_buffer are defined
using the local_t type, despite the fact that they are only accessed via
the perf_aux_output_* functions, which cannot race with each other for a
given ring buffer.

This patch changes the type of the members to long, so we can avoid
using the local_* API where it isn't needed.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 kernel/events/internal.h    |  4 ++--
 kernel/events/ring_buffer.c | 27 +++++++++++++--------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 486fd78eb8d5..2941b868353c 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -38,9 +38,9 @@ struct ring_buffer {
 	struct user_struct		*mmap_user;
 
 	/* AUX area */
-	local_t				aux_head;
+	long				aux_head;
 	local_t				aux_nest;
-	local_t				aux_wakeup;
+	long				aux_wakeup;
 	unsigned long			aux_pgoff;
 	int				aux_nr_pages;
 	int				aux_overwrite;
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 14a240ff439d..330df5a7f762 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -367,7 +367,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	if (WARN_ON_ONCE(local_xchg(&rb->aux_nest, 1)))
 		goto err_put;
 
-	aux_head = local_read(&rb->aux_head);
+	aux_head = rb->aux_head;
 
 	handle->rb = rb;
 	handle->event = event;
@@ -382,7 +382,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	 */
 	if (!rb->aux_overwrite) {
 		aux_tail = ACCESS_ONCE(rb->user_page->aux_tail);
-		handle->wakeup = local_read(&rb->aux_wakeup) + rb->aux_watermark;
+		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 		if (aux_head - aux_tail < perf_aux_size(rb))
 			handle->size = CIRC_SPACE(aux_head, aux_tail, perf_aux_size(rb));
 
@@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
 
 		aux_head = handle->head;
-		local_set(&rb->aux_head, aux_head);
+		rb->aux_head = aux_head;
 	} else {
 		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
 
-		aux_head = local_read(&rb->aux_head);
-		local_add(size, &rb->aux_head);
+		aux_head = rb->aux_head;
+		rb->aux_head += size;
 	}
 
 	if (size || handle->aux_flags) {
@@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		                     handle->aux_flags);
 	}
 
-	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
+	aux_head = rb->user_page->aux_head = rb->aux_head;
 
-	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
+	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		wakeup = true;
-		local_add(rb->aux_watermark, &rb->aux_wakeup);
+		rb->aux_wakeup += rb->aux_watermark;
 	}
 
 	if (wakeup) {
@@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
 	if (size > handle->size)
 		return -ENOSPC;
 
-	local_add(size, &rb->aux_head);
+	rb->aux_head += size;
 
-	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
-	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
+	aux_head = rb->user_page->aux_head = rb->aux_head;
+	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		perf_output_wakeup(handle);
-		local_add(rb->aux_watermark, &rb->aux_wakeup);
-		handle->wakeup = local_read(&rb->aux_wakeup) +
-				 rb->aux_watermark;
+		rb->aux_wakeup += rb->aux_watermark;
+		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 	}
 
 	handle->head = aux_head;
-- 
2.1.4

[PATCH 1/2] perf/aux: Make aux_{head, wakeup} ring_buffer members long

[Will Deacon]

The aux_head and aux_wakeup members of struct ring_buffer are defined
using the local_t type, despite the fact that they are only accessed via
the perf_aux_output_* functions, which cannot race with each other for a
given ring buffer.

This patch changes the type of the members to long, so we can avoid
using the local_* API where it isn't needed.

Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 kernel/events/internal.h    |  4 ++--
 kernel/events/ring_buffer.c | 27 +++++++++++++--------------
 2 files changed, 15 insertions(+), 16 deletions(-)

diff --git a/kernel/events/internal.h b/kernel/events/internal.h
index 486fd78eb8d5..2941b868353c 100644
--- a/kernel/events/internal.h
+++ b/kernel/events/internal.h
@@ -38,9 +38,9 @@ struct ring_buffer {
 	struct user_struct		*mmap_user;
 
 	/* AUX area */
-	local_t				aux_head;
+	long				aux_head;
 	local_t				aux_nest;
-	local_t				aux_wakeup;
+	long				aux_wakeup;
 	unsigned long			aux_pgoff;
 	int				aux_nr_pages;
 	int				aux_overwrite;
diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 14a240ff439d..330df5a7f762 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -367,7 +367,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	if (WARN_ON_ONCE(local_xchg(&rb->aux_nest, 1)))
 		goto err_put;
 
-	aux_head = local_read(&rb->aux_head);
+	aux_head = rb->aux_head;
 
 	handle->rb = rb;
 	handle->event = event;
@@ -382,7 +382,7 @@ void *perf_aux_output_begin(struct perf_output_handle *handle,
 	 */
 	if (!rb->aux_overwrite) {
 		aux_tail = ACCESS_ONCE(rb->user_page->aux_tail);
-		handle->wakeup = local_read(&rb->aux_wakeup) + rb->aux_watermark;
+		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 		if (aux_head - aux_tail < perf_aux_size(rb))
 			handle->size = CIRC_SPACE(aux_head, aux_tail, perf_aux_size(rb));
 
@@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
 
 		aux_head = handle->head;
-		local_set(&rb->aux_head, aux_head);
+		rb->aux_head = aux_head;
 	} else {
 		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
 
-		aux_head = local_read(&rb->aux_head);
-		local_add(size, &rb->aux_head);
+		aux_head = rb->aux_head;
+		rb->aux_head += size;
 	}
 
 	if (size || handle->aux_flags) {
@@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		                     handle->aux_flags);
 	}
 
-	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
+	aux_head = rb->user_page->aux_head = rb->aux_head;
 
-	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
+	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		wakeup = true;
-		local_add(rb->aux_watermark, &rb->aux_wakeup);
+		rb->aux_wakeup += rb->aux_watermark;
 	}
 
 	if (wakeup) {
@@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
 	if (size > handle->size)
 		return -ENOSPC;
 
-	local_add(size, &rb->aux_head);
+	rb->aux_head += size;
 
-	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
-	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
+	aux_head = rb->user_page->aux_head = rb->aux_head;
+	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		perf_output_wakeup(handle);
-		local_add(rb->aux_watermark, &rb->aux_wakeup);
-		handle->wakeup = local_read(&rb->aux_wakeup) +
-				 rb->aux_watermark;
+		rb->aux_wakeup += rb->aux_watermark;
+		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 	}
 
 	handle->head = aux_head;
-- 
2.1.4

[PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Will Deacon]

The aux_watermark member of struct ring_buffer represents the period (in
terms of bytes) at which wakeup events should be generated when data is
written to the aux buffer in non-snapshot mode. On hardware that cannot
generate an interrupt when the aux_head reaches an arbitrary wakeup index
(such as ARM SPE), the aux_head sampled from handle->head in
perf_aux_output_{skip,end} may in fact be past the wakeup index. This
can lead to wakeup slowly falling behind the head. For example, consider
the case where hardware can only generate an interrupt on a page-boundary
and the aux buffer is initialised as follows:

  // Buffer size is 2 * PAGE_SIZE
  rb->aux_head = rb->aux_wakeup = 0
  rb->aux_watermark = PAGE_SIZE / 2

following the first perf_aux_output_begin call, the handle is
initialised with:

  handle->head = 0
  handle->size = 2 * PAGE_SIZE
  handle->wakeup = PAGE_SIZE / 2

and the hardware will be programmed to generate an interrupt at
PAGE_SIZE.

When the interrupt is raised, the hardware head will be at PAGE_SIZE,
so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
into the following state:

  rb->aux_head = PAGE_SIZE
  rb->aux_wakeup = PAGE_SIZE / 2
  rb->aux_watermark = PAGE_SIZE / 2

and then the next call to perf_aux_output_begin will result in:

  handle->head = handle->wakeup = PAGE_SIZE

for which the semantics are unclear and, for a smaller aux_watermark
(e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
this point.

This patch fixes the problem by rounding down the aux_head (as sampled
from the handle) to the nearest aux_watermark boundary when updating
rb->aux_wakeup, therefore taking into account any overruns by the
hardware.

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 kernel/events/ring_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 330df5a7f762..8e511e52fc1b 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -455,7 +455,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 
 	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		wakeup = true;
-		rb->aux_wakeup += rb->aux_watermark;
+		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
 	}
 
 	if (wakeup) {
@@ -490,7 +490,7 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
 	aux_head = rb->user_page->aux_head = rb->aux_head;
 	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		perf_output_wakeup(handle);
-		rb->aux_wakeup += rb->aux_watermark;
+		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
 		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 	}
 
-- 
2.1.4

[PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Will Deacon]

The aux_watermark member of struct ring_buffer represents the period (in
terms of bytes) at which wakeup events should be generated when data is
written to the aux buffer in non-snapshot mode. On hardware that cannot
generate an interrupt when the aux_head reaches an arbitrary wakeup index
(such as ARM SPE), the aux_head sampled from handle->head in
perf_aux_output_{skip,end} may in fact be past the wakeup index. This
can lead to wakeup slowly falling behind the head. For example, consider
the case where hardware can only generate an interrupt on a page-boundary
and the aux buffer is initialised as follows:

  // Buffer size is 2 * PAGE_SIZE
  rb->aux_head = rb->aux_wakeup = 0
  rb->aux_watermark = PAGE_SIZE / 2

following the first perf_aux_output_begin call, the handle is
initialised with:

  handle->head = 0
  handle->size = 2 * PAGE_SIZE
  handle->wakeup = PAGE_SIZE / 2

and the hardware will be programmed to generate an interrupt at
PAGE_SIZE.

When the interrupt is raised, the hardware head will be at PAGE_SIZE,
so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
into the following state:

  rb->aux_head = PAGE_SIZE
  rb->aux_wakeup = PAGE_SIZE / 2
  rb->aux_watermark = PAGE_SIZE / 2

and then the next call to perf_aux_output_begin will result in:

  handle->head = handle->wakeup = PAGE_SIZE

for which the semantics are unclear and, for a smaller aux_watermark
(e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
this point.

This patch fixes the problem by rounding down the aux_head (as sampled
from the handle) to the nearest aux_watermark boundary when updating
rb->aux_wakeup, therefore taking into account any overruns by the
hardware.

Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Reported-by: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 kernel/events/ring_buffer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 330df5a7f762..8e511e52fc1b 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -455,7 +455,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 
 	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		wakeup = true;
-		rb->aux_wakeup += rb->aux_watermark;
+		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
 	}
 
 	if (wakeup) {
@@ -490,7 +490,7 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
 	aux_head = rb->user_page->aux_head = rb->aux_head;
 	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
 		perf_output_wakeup(handle);
-		rb->aux_wakeup += rb->aux_watermark;
+		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
 		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
 	}
 
-- 
2.1.4

Re: [PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Alexander Shishkin]

Will Deacon <will.deacon@arm.com> writes:

> The aux_watermark member of struct ring_buffer represents the period (in
> terms of bytes) at which wakeup events should be generated when data is
> written to the aux buffer in non-snapshot mode. On hardware that cannot
> generate an interrupt when the aux_head reaches an arbitrary wakeup index

Curious: how do you support non-snapshot trace collection on such
hardware?

> (such as ARM SPE), the aux_head sampled from handle->head in
> perf_aux_output_{skip,end} may in fact be past the wakeup index. This

I think this is also true of hw where the interrupt is not
precise. Thanks for looking at this.

> can lead to wakeup slowly falling behind the head. For example, consider
> the case where hardware can only generate an interrupt on a page-boundary
> and the aux buffer is initialised as follows:
>
>   // Buffer size is 2 * PAGE_SIZE
>   rb->aux_head = rb->aux_wakeup = 0
>   rb->aux_watermark = PAGE_SIZE / 2
>
> following the first perf_aux_output_begin call, the handle is
> initialised with:
>
>   handle->head = 0
>   handle->size = 2 * PAGE_SIZE
>   handle->wakeup = PAGE_SIZE / 2
>
> and the hardware will be programmed to generate an interrupt at
> PAGE_SIZE.
>
> When the interrupt is raised, the hardware head will be at PAGE_SIZE,
> so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
> into the following state:
>
>   rb->aux_head = PAGE_SIZE
>   rb->aux_wakeup = PAGE_SIZE / 2
>   rb->aux_watermark = PAGE_SIZE / 2
>
> and then the next call to perf_aux_output_begin will result in:
>
>   handle->head = handle->wakeup = PAGE_SIZE
>
> for which the semantics are unclear and, for a smaller aux_watermark
> (e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
> this point.
>
> This patch fixes the problem by rounding down the aux_head (as sampled
> from the handle) to the nearest aux_watermark boundary when updating
> rb->aux_wakeup, therefore taking into account any overruns by the
> hardware.

Let's add a small comment to the @aux_wakeup field definition? Other
than that,

Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>

>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  kernel/events/ring_buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 330df5a7f762..8e511e52fc1b 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -455,7 +455,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  
>  	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		wakeup = true;
> -		rb->aux_wakeup += rb->aux_watermark;
> +		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
>  	}
>  
>  	if (wakeup) {
> @@ -490,7 +490,7 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
>  	aux_head = rb->user_page->aux_head = rb->aux_head;
>  	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		perf_output_wakeup(handle);
> -		rb->aux_wakeup += rb->aux_watermark;
> +		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
>  		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
>  	}
>  
> -- 
> 2.1.4

[PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Alexander Shishkin]

Will Deacon <will.deacon@arm.com> writes:

> The aux_watermark member of struct ring_buffer represents the period (in
> terms of bytes) at which wakeup events should be generated when data is
> written to the aux buffer in non-snapshot mode. On hardware that cannot
> generate an interrupt when the aux_head reaches an arbitrary wakeup index

Curious: how do you support non-snapshot trace collection on such
hardware?

> (such as ARM SPE), the aux_head sampled from handle->head in
> perf_aux_output_{skip,end} may in fact be past the wakeup index. This

I think this is also true of hw where the interrupt is not
precise. Thanks for looking at this.

> can lead to wakeup slowly falling behind the head. For example, consider
> the case where hardware can only generate an interrupt on a page-boundary
> and the aux buffer is initialised as follows:
>
>   // Buffer size is 2 * PAGE_SIZE
>   rb->aux_head = rb->aux_wakeup = 0
>   rb->aux_watermark = PAGE_SIZE / 2
>
> following the first perf_aux_output_begin call, the handle is
> initialised with:
>
>   handle->head = 0
>   handle->size = 2 * PAGE_SIZE
>   handle->wakeup = PAGE_SIZE / 2
>
> and the hardware will be programmed to generate an interrupt at
> PAGE_SIZE.
>
> When the interrupt is raised, the hardware head will be at PAGE_SIZE,
> so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
> into the following state:
>
>   rb->aux_head = PAGE_SIZE
>   rb->aux_wakeup = PAGE_SIZE / 2
>   rb->aux_watermark = PAGE_SIZE / 2
>
> and then the next call to perf_aux_output_begin will result in:
>
>   handle->head = handle->wakeup = PAGE_SIZE
>
> for which the semantics are unclear and, for a smaller aux_watermark
> (e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
> this point.
>
> This patch fixes the problem by rounding down the aux_head (as sampled
> from the handle) to the nearest aux_watermark boundary when updating
> rb->aux_wakeup, therefore taking into account any overruns by the
> hardware.

Let's add a small comment to the @aux_wakeup field definition? Other
than that,

Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>

>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Reported-by: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  kernel/events/ring_buffer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 330df5a7f762..8e511e52fc1b 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -455,7 +455,7 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  
>  	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		wakeup = true;
> -		rb->aux_wakeup += rb->aux_watermark;
> +		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
>  	}
>  
>  	if (wakeup) {
> @@ -490,7 +490,7 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
>  	aux_head = rb->user_page->aux_head = rb->aux_head;
>  	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		perf_output_wakeup(handle);
> -		rb->aux_wakeup += rb->aux_watermark;
> +		rb->aux_wakeup = rounddown(aux_head, rb->aux_watermark);
>  		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
>  	}
>  
> -- 
> 2.1.4

Re: [PATCH 1/2] perf/aux: Make aux_{head,wakeup} ring_buffer members long

[Alexander Shishkin]

Will Deacon <will.deacon@arm.com> writes:

> The aux_head and aux_wakeup members of struct ring_buffer are defined
> using the local_t type, despite the fact that they are only accessed via
> the perf_aux_output_* functions, which cannot race with each other for a
> given ring buffer.
>
> This patch changes the type of the members to long, so we can avoid
> using the local_* API where it isn't needed.

Thanks for digging this up! Some minor nits below.

> @@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
>  
>  		aux_head = handle->head;
> -		local_set(&rb->aux_head, aux_head);
> +		rb->aux_head = aux_head;
>  	} else {
>  		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
>  
> -		aux_head = local_read(&rb->aux_head);
> -		local_add(size, &rb->aux_head);
> +		aux_head = rb->aux_head;
> +		rb->aux_head += size;
>  	}
>  
>  	if (size || handle->aux_flags) {
> @@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  		                     handle->aux_flags);
>  	}
>  
> -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> +	aux_head = rb->user_page->aux_head = rb->aux_head;
>  
> -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {

Can't we just do away with aux_head here:

  rb->user_page->aux_head = rb->aux_head;
  if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) { ...

?

> @@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
>  	if (size > handle->size)
>  		return -ENOSPC;
>  
> -	local_add(size, &rb->aux_head);
> +	rb->aux_head += size;
>  
> -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> +	aux_head = rb->user_page->aux_head = rb->aux_head;
> +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		perf_output_wakeup(handle);
> -		local_add(rb->aux_watermark, &rb->aux_wakeup);
> -		handle->wakeup = local_read(&rb->aux_wakeup) +
> -				 rb->aux_watermark;
> +		rb->aux_wakeup += rb->aux_watermark;
> +		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
>  	}
>  
>  	handle->head = aux_head;

And here I think we don't need aux_head at all.

Thanks,
--
Alex

[PATCH 1/2] perf/aux: Make aux_{head, wakeup} ring_buffer members long

[Alexander Shishkin]

Will Deacon <will.deacon@arm.com> writes:

> The aux_head and aux_wakeup members of struct ring_buffer are defined
> using the local_t type, despite the fact that they are only accessed via
> the perf_aux_output_* functions, which cannot race with each other for a
> given ring buffer.
>
> This patch changes the type of the members to long, so we can avoid
> using the local_* API where it isn't needed.

Thanks for digging this up! Some minor nits below.

> @@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
>  
>  		aux_head = handle->head;
> -		local_set(&rb->aux_head, aux_head);
> +		rb->aux_head = aux_head;
>  	} else {
>  		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
>  
> -		aux_head = local_read(&rb->aux_head);
> -		local_add(size, &rb->aux_head);
> +		aux_head = rb->aux_head;
> +		rb->aux_head += size;
>  	}
>  
>  	if (size || handle->aux_flags) {
> @@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
>  		                     handle->aux_flags);
>  	}
>  
> -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> +	aux_head = rb->user_page->aux_head = rb->aux_head;
>  
> -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {

Can't we just do away with aux_head here:

  rb->user_page->aux_head = rb->aux_head;
  if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) { ...

?

> @@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
>  	if (size > handle->size)
>  		return -ENOSPC;
>  
> -	local_add(size, &rb->aux_head);
> +	rb->aux_head += size;
>  
> -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> +	aux_head = rb->user_page->aux_head = rb->aux_head;
> +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
>  		perf_output_wakeup(handle);
> -		local_add(rb->aux_watermark, &rb->aux_wakeup);
> -		handle->wakeup = local_read(&rb->aux_wakeup) +
> -				 rb->aux_watermark;
> +		rb->aux_wakeup += rb->aux_watermark;
> +		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
>  	}
>  
>  	handle->head = aux_head;

And here I think we don't need aux_head at all.

Thanks,
--
Alex

Re: [PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Will Deacon]

Hi Alexander,

Thanks for having a look at these.

On Mon, Jul 31, 2017 at 01:02:16PM +0300, Alexander Shishkin wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > The aux_watermark member of struct ring_buffer represents the period (in
> > terms of bytes) at which wakeup events should be generated when data is
> > written to the aux buffer in non-snapshot mode. On hardware that cannot
> > generate an interrupt when the aux_head reaches an arbitrary wakeup index
> 
> Curious: how do you support non-snapshot trace collection on such
> hardware?

The watermark is constrained to lie on a page boundary, so as long as the
buffer is at least a page (which it is!), we end up rounding up to the next
page boundary, with lots of fun and games to avoid going past the head.

> > (such as ARM SPE), the aux_head sampled from handle->head in
> > perf_aux_output_{skip,end} may in fact be past the wakeup index. This
> 
> I think this is also true of hw where the interrupt is not
> precise. Thanks for looking at this.

Yes, it all looks like "skid" to userspace.

> > can lead to wakeup slowly falling behind the head. For example, consider
> > the case where hardware can only generate an interrupt on a page-boundary
> > and the aux buffer is initialised as follows:
> >
> >   // Buffer size is 2 * PAGE_SIZE
> >   rb->aux_head = rb->aux_wakeup = 0
> >   rb->aux_watermark = PAGE_SIZE / 2
> >
> > following the first perf_aux_output_begin call, the handle is
> > initialised with:
> >
> >   handle->head = 0
> >   handle->size = 2 * PAGE_SIZE
> >   handle->wakeup = PAGE_SIZE / 2
> >
> > and the hardware will be programmed to generate an interrupt at
> > PAGE_SIZE.
> >
> > When the interrupt is raised, the hardware head will be at PAGE_SIZE,
> > so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
> > into the following state:
> >
> >   rb->aux_head = PAGE_SIZE
> >   rb->aux_wakeup = PAGE_SIZE / 2
> >   rb->aux_watermark = PAGE_SIZE / 2
> >
> > and then the next call to perf_aux_output_begin will result in:
> >
> >   handle->head = handle->wakeup = PAGE_SIZE
> >
> > for which the semantics are unclear and, for a smaller aux_watermark
> > (e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
> > this point.
> >
> > This patch fixes the problem by rounding down the aux_head (as sampled
> > from the handle) to the nearest aux_watermark boundary when updating
> > rb->aux_wakeup, therefore taking into account any overruns by the
> > hardware.
> 
> Let's add a small comment to the @aux_wakeup field definition? Other
> than that,

Good thinking; I'll do that.

> Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>

Thanks!

Will

[PATCH 2/2] perf/aux: Ensure aux_wakeup represents most recent wakeup index

[Will Deacon]

Hi Alexander,

Thanks for having a look at these.

On Mon, Jul 31, 2017 at 01:02:16PM +0300, Alexander Shishkin wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > The aux_watermark member of struct ring_buffer represents the period (in
> > terms of bytes) at which wakeup events should be generated when data is
> > written to the aux buffer in non-snapshot mode. On hardware that cannot
> > generate an interrupt when the aux_head reaches an arbitrary wakeup index
> 
> Curious: how do you support non-snapshot trace collection on such
> hardware?

The watermark is constrained to lie on a page boundary, so as long as the
buffer is at least a page (which it is!), we end up rounding up to the next
page boundary, with lots of fun and games to avoid going past the head.

> > (such as ARM SPE), the aux_head sampled from handle->head in
> > perf_aux_output_{skip,end} may in fact be past the wakeup index. This
> 
> I think this is also true of hw where the interrupt is not
> precise. Thanks for looking at this.

Yes, it all looks like "skid" to userspace.

> > can lead to wakeup slowly falling behind the head. For example, consider
> > the case where hardware can only generate an interrupt on a page-boundary
> > and the aux buffer is initialised as follows:
> >
> >   // Buffer size is 2 * PAGE_SIZE
> >   rb->aux_head = rb->aux_wakeup = 0
> >   rb->aux_watermark = PAGE_SIZE / 2
> >
> > following the first perf_aux_output_begin call, the handle is
> > initialised with:
> >
> >   handle->head = 0
> >   handle->size = 2 * PAGE_SIZE
> >   handle->wakeup = PAGE_SIZE / 2
> >
> > and the hardware will be programmed to generate an interrupt at
> > PAGE_SIZE.
> >
> > When the interrupt is raised, the hardware head will be at PAGE_SIZE,
> > so calling perf_aux_output_end(handle, PAGE_SIZE) puts the ring buffer
> > into the following state:
> >
> >   rb->aux_head = PAGE_SIZE
> >   rb->aux_wakeup = PAGE_SIZE / 2
> >   rb->aux_watermark = PAGE_SIZE / 2
> >
> > and then the next call to perf_aux_output_begin will result in:
> >
> >   handle->head = handle->wakeup = PAGE_SIZE
> >
> > for which the semantics are unclear and, for a smaller aux_watermark
> > (e.g. PAGE_SIZE / 4), then the wakeup would in fact be behind head at
> > this point.
> >
> > This patch fixes the problem by rounding down the aux_head (as sampled
> > from the handle) to the nearest aux_watermark boundary when updating
> > rb->aux_wakeup, therefore taking into account any overruns by the
> > hardware.
> 
> Let's add a small comment to the @aux_wakeup field definition? Other
> than that,

Good thinking; I'll do that.

> Acked-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>

Thanks!

Will

Re: [PATCH 1/2] perf/aux: Make aux_{head,wakeup} ring_buffer members long

[Will Deacon]

On Mon, Jul 31, 2017 at 01:28:01PM +0300, Alexander Shishkin wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > The aux_head and aux_wakeup members of struct ring_buffer are defined
> > using the local_t type, despite the fact that they are only accessed via
> > the perf_aux_output_* functions, which cannot race with each other for a
> > given ring buffer.
> >
> > This patch changes the type of the members to long, so we can avoid
> > using the local_* API where it isn't needed.
> 
> Thanks for digging this up! Some minor nits below.
> 
> > @@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >  		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
> >  
> >  		aux_head = handle->head;
> > -		local_set(&rb->aux_head, aux_head);
> > +		rb->aux_head = aux_head;
> >  	} else {
> >  		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
> >  
> > -		aux_head = local_read(&rb->aux_head);
> > -		local_add(size, &rb->aux_head);
> > +		aux_head = rb->aux_head;
> > +		rb->aux_head += size;
> >  	}
> >  
> >  	if (size || handle->aux_flags) {
> > @@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >  		                     handle->aux_flags);
> >  	}
> >  
> > -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> > +	aux_head = rb->user_page->aux_head = rb->aux_head;
> >  
> > -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> > +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> 
> Can't we just do away with aux_head here:
> 
>   rb->user_page->aux_head = rb->aux_head;
>   if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) { ...
> 
> ?
> 
> > @@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
> >  	if (size > handle->size)
> >  		return -ENOSPC;
> >  
> > -	local_add(size, &rb->aux_head);
> > +	rb->aux_head += size;
> >  
> > -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> > -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> > +	aux_head = rb->user_page->aux_head = rb->aux_head;
> > +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> >  		perf_output_wakeup(handle);
> > -		local_add(rb->aux_watermark, &rb->aux_wakeup);
> > -		handle->wakeup = local_read(&rb->aux_wakeup) +
> > -				 rb->aux_watermark;
> > +		rb->aux_wakeup += rb->aux_watermark;
> > +		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
> >  	}
> >  
> >  	handle->head = aux_head;
> 
> And here I think we don't need aux_head at all.

Agreed on both counts. Will fix for v2.

Will

[PATCH 1/2] perf/aux: Make aux_{head, wakeup} ring_buffer members long

[Will Deacon]

On Mon, Jul 31, 2017 at 01:28:01PM +0300, Alexander Shishkin wrote:
> Will Deacon <will.deacon@arm.com> writes:
> 
> > The aux_head and aux_wakeup members of struct ring_buffer are defined
> > using the local_t type, despite the fact that they are only accessed via
> > the perf_aux_output_* functions, which cannot race with each other for a
> > given ring buffer.
> >
> > This patch changes the type of the members to long, so we can avoid
> > using the local_* API where it isn't needed.
> 
> Thanks for digging this up! Some minor nits below.
> 
> > @@ -434,12 +434,12 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >  		handle->aux_flags |= PERF_AUX_FLAG_OVERWRITE;
> >  
> >  		aux_head = handle->head;
> > -		local_set(&rb->aux_head, aux_head);
> > +		rb->aux_head = aux_head;
> >  	} else {
> >  		handle->aux_flags &= ~PERF_AUX_FLAG_OVERWRITE;
> >  
> > -		aux_head = local_read(&rb->aux_head);
> > -		local_add(size, &rb->aux_head);
> > +		aux_head = rb->aux_head;
> > +		rb->aux_head += size;
> >  	}
> >  
> >  	if (size || handle->aux_flags) {
> > @@ -451,11 +451,11 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
> >  		                     handle->aux_flags);
> >  	}
> >  
> > -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> > +	aux_head = rb->user_page->aux_head = rb->aux_head;
> >  
> > -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> > +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> 
> Can't we just do away with aux_head here:
> 
>   rb->user_page->aux_head = rb->aux_head;
>   if (rb->aux_head - rb->aux_wakeup >= rb->aux_watermark) { ...
> 
> ?
> 
> > @@ -485,14 +485,13 @@ int perf_aux_output_skip(struct perf_output_handle *handle, unsigned long size)
> >  	if (size > handle->size)
> >  		return -ENOSPC;
> >  
> > -	local_add(size, &rb->aux_head);
> > +	rb->aux_head += size;
> >  
> > -	aux_head = rb->user_page->aux_head = local_read(&rb->aux_head);
> > -	if (aux_head - local_read(&rb->aux_wakeup) >= rb->aux_watermark) {
> > +	aux_head = rb->user_page->aux_head = rb->aux_head;
> > +	if (aux_head - rb->aux_wakeup >= rb->aux_watermark) {
> >  		perf_output_wakeup(handle);
> > -		local_add(rb->aux_watermark, &rb->aux_wakeup);
> > -		handle->wakeup = local_read(&rb->aux_wakeup) +
> > -				 rb->aux_watermark;
> > +		rb->aux_wakeup += rb->aux_watermark;
> > +		handle->wakeup = rb->aux_wakeup + rb->aux_watermark;
> >  	}
> >  
> >  	handle->head = aux_head;
> 
> And here I think we don't need aux_head at all.

Agreed on both counts. Will fix for v2.

Will