[PATCH] perf: Fix AUX record suppression

[PATCH] perf: Fix AUX record suppression

[Alexander Shishkin]

Commit 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records") has an
unintended side-effect of also suppressing all AUX records with no flags
and non-zero size, so all the regular records in the full trace mode.
This breaks some use cases for people.

Fix this by restoring "regular" AUX records.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Fixes: 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records")
Reported-by: Ben Gainey <Ben.Gainey@arm.com>
Tested-by: Ben Gainey <Ben.Gainey@arm.com>
CC: stable@vger.kernel.org # v4.20+
---
 kernel/events/ring_buffer.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 678ccec60d8f..626256dc26c1 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -455,24 +455,21 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		rb->aux_head += size;
 	}
 
-	if (size || handle->aux_flags) {
-		/*
-		 * Only send RECORD_AUX if we have something useful to communicate
-		 *
-		 * Note: the OVERWRITE records by themselves are not considered
-		 * useful, as they don't communicate any *new* information,
-		 * aside from the short-lived offset, that becomes history at
-		 * the next event sched-in and therefore isn't useful.
-		 * The userspace that needs to copy out AUX data in overwrite
-		 * mode should know to use user_page::aux_head for the actual
-		 * offset. So, from now on we don't output AUX records that
-		 * have *only* OVERWRITE flag set.
-		 */
-
-		if (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE)
-			perf_event_aux_event(handle->event, aux_head, size,
-			                     handle->aux_flags);
-	}
+	/*
+	 * Only send RECORD_AUX if we have something useful to communicate
+	 *
+	 * Note: the OVERWRITE records by themselves are not considered
+	 * useful, as they don't communicate any *new* information,
+	 * aside from the short-lived offset, that becomes history at
+	 * the next event sched-in and therefore isn't useful.
+	 * The userspace that needs to copy out AUX data in overwrite
+	 * mode should know to use user_page::aux_head for the actual
+	 * offset. So, from now on we don't output AUX records that
+	 * have *only* OVERWRITE flag set.
+	 */
+	if (size || (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE))
+		perf_event_aux_event(handle->event, aux_head, size,
+				     handle->aux_flags);
 
 	rb->user_page->aux_head = rb->aux_head;
 	if (rb_need_aux_wakeup(rb))
-- 
2.20.1

Re: [PATCH] perf: Fix AUX record suppression

[Ben Gainey]

Hi Alex

I was just wondering if this was expected to make 5.1? (I could not see
it in RC5)?

Thanks
Ben



On Fri, 2019-03-29 at 11:13 +0200, Alexander Shishkin wrote:
> Commit 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records") has
> an
> unintended side-effect of also suppressing all AUX records with no
> flags
> and non-zero size, so all the regular records in the full trace mode.
> This breaks some use cases for people.
>
> Fix this by restoring "regular" AUX records.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com
> >
> Fixes: 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records")
> Reported-by: Ben Gainey <Ben.Gainey@arm.com>
> Tested-by: Ben Gainey <Ben.Gainey@arm.com>
> CC: stable@vger.kernel.org # v4.20+
> ---
>  kernel/events/ring_buffer.c | 33 +++++++++++++++------------------
>  1 file changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/kernel/events/ring_buffer.c
> b/kernel/events/ring_buffer.c
> index 678ccec60d8f..626256dc26c1 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -455,24 +455,21 @@ void perf_aux_output_end(struct
> perf_output_handle *handle, unsigned long size)
>  rb->aux_head += size;
>  }
>
> -if (size || handle->aux_flags) {
> -/*
> - * Only send RECORD_AUX if we have something useful to
> communicate
> - *
> - * Note: the OVERWRITE records by themselves are not
> considered
> - * useful, as they don't communicate any *new*
> information,
> - * aside from the short-lived offset, that becomes
> history at
> - * the next event sched-in and therefore isn't useful.
> - * The userspace that needs to copy out AUX data in
> overwrite
> - * mode should know to use user_page::aux_head for the
> actual
> - * offset. So, from now on we don't output AUX records
> that
> - * have *only* OVERWRITE flag set.
> - */
> -
> -if (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE)
> -perf_event_aux_event(handle->event, aux_head,
> size,
> -                     handle->aux_flags);
> -}
> +/*
> + * Only send RECORD_AUX if we have something useful to
> communicate
> + *
> + * Note: the OVERWRITE records by themselves are not considered
> + * useful, as they don't communicate any *new* information,
> + * aside from the short-lived offset, that becomes history at
> + * the next event sched-in and therefore isn't useful.
> + * The userspace that needs to copy out AUX data in overwrite
> + * mode should know to use user_page::aux_head for the actual
> + * offset. So, from now on we don't output AUX records that
> + * have *only* OVERWRITE flag set.
> + */
> +if (size || (handle->aux_flags &
> ~(u64)PERF_AUX_FLAG_OVERWRITE))
> +perf_event_aux_event(handle->event, aux_head, size,
> +     handle->aux_flags);
>
>  rb->user_page->aux_head = rb->aux_head;
>  if (rb_need_aux_wakeup(rb))
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH] perf: Fix AUX record suppression

[Peter Zijlstra]

On Mon, Apr 15, 2019 at 08:04:33AM +0000, Ben Gainey wrote:
> Hi Alex
> 
> I was just wondering if this was expected to make 5.1? (I could not see
> it in RC5)?

Yikes, I forgot about this again, didn't I ?

At least, they appear to still apply, so I've queued them. Not sure
they'll still make this release, but at least they should not go missing
_again_.

Re: [PATCH] perf: Fix AUX record suppression

[Ben Gainey]

Thanks Peter :-) 


On Mon, 2019-04-15 at 11:16 +0200, Peter Zijlstra wrote:
> On Mon, Apr 15, 2019 at 08:04:33AM +0000, Ben Gainey wrote:
> > Hi Alex
> > 
> > I was just wondering if this was expected to make 5.1? (I could not
> > see
> > it in RC5)?
> 
> Yikes, I forgot about this again, didn't I ?
> 
> At least, they appear to still apply, so I've queued them. Not sure
> they'll still make this release, but at least they should not go
> missing
> _again_.

[tip:perf/core] perf/ring_buffer: Fix AUX record suppression

[tip-bot for Alexander Shishkin]

Commit-ID:  339bc4183596e1f68c2c98a03b87aa124107c317
Gitweb:     https://git.kernel.org/tip/339bc4183596e1f68c2c98a03b87aa124107c317
Author:     Alexander Shishkin <alexander.shishkin@linux.intel.com>
AuthorDate: Fri, 29 Mar 2019 11:13:38 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 16 Apr 2019 12:13:57 +0200

perf/ring_buffer: Fix AUX record suppression

The following commit:

  1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records")

has an unintended side-effect of also suppressing all AUX records with no flags
and non-zero size, so all the regular records in the full trace mode.
This breaks some use cases for people.

Fix this by restoring "regular" AUX records.

Reported-by: Ben Gainey <Ben.Gainey@arm.com>
Tested-by: Ben Gainey <Ben.Gainey@arm.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: <stable@vger.kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Vince Weaver <vincent.weaver@maine.edu>
Fixes: 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records")
Link: https://lkml.kernel.org/r/20190329091338.29999-1-alexander.shishkin@linux.intel.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/events/ring_buffer.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
index 2545ac08cc77..5eedb49a65ea 100644
--- a/kernel/events/ring_buffer.c
+++ b/kernel/events/ring_buffer.c
@@ -455,24 +455,21 @@ void perf_aux_output_end(struct perf_output_handle *handle, unsigned long size)
 		rb->aux_head += size;
 	}
 
-	if (size || handle->aux_flags) {
-		/*
-		 * Only send RECORD_AUX if we have something useful to communicate
-		 *
-		 * Note: the OVERWRITE records by themselves are not considered
-		 * useful, as they don't communicate any *new* information,
-		 * aside from the short-lived offset, that becomes history at
-		 * the next event sched-in and therefore isn't useful.
-		 * The userspace that needs to copy out AUX data in overwrite
-		 * mode should know to use user_page::aux_head for the actual
-		 * offset. So, from now on we don't output AUX records that
-		 * have *only* OVERWRITE flag set.
-		 */
-
-		if (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE)
-			perf_event_aux_event(handle->event, aux_head, size,
-			                     handle->aux_flags);
-	}
+	/*
+	 * Only send RECORD_AUX if we have something useful to communicate
+	 *
+	 * Note: the OVERWRITE records by themselves are not considered
+	 * useful, as they don't communicate any *new* information,
+	 * aside from the short-lived offset, that becomes history at
+	 * the next event sched-in and therefore isn't useful.
+	 * The userspace that needs to copy out AUX data in overwrite
+	 * mode should know to use user_page::aux_head for the actual
+	 * offset. So, from now on we don't output AUX records that
+	 * have *only* OVERWRITE flag set.
+	 */
+	if (size || (handle->aux_flags & ~(u64)PERF_AUX_FLAG_OVERWRITE))
+		perf_event_aux_event(handle->event, aux_head, size,
+				     handle->aux_flags);
 
 	rb->user_page->aux_head = rb->aux_head;
 	if (rb_need_aux_wakeup(rb))