Re: BUG in "perf: Suppress AUX/OVERWRITE records"?

[Ben Gainey]

On Wed, 2019-03-27 at 11:17 +0200, Alexander Shishkin wrote:
> Ben Gainey <Ben.Gainey@xxxxxxx> writes:
>
> > > It was an unintentional side effect that it also
> > > happened to coincide with context switches in the overwrite mode.
> >
> > I'm not using overwrite mode, I'm opening the mmap with PROT_WRITE
> > (i.e. in truncate mode).
>
> Now I get it. Does the below fix the problem for you?

Yes I guess I should have mentioned that in the first place.
Tested, working now as expected; I see regular PERF_RECORD_AUX, that
coincide with the context switch.

Thanks!
Ben

>
> From bf52320cce0e74a2c0d987db7bd571f7687b4f4f Mon Sep 17 00:00:00
> 2001
> From: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Date: Wed, 27 Mar 2019 11:05:53 +0200
> Subject: [PATCH] perf: Fix AUX record suppression
>
> 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 to their former glory.
>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx
> >
> Fixes: 1627314fb54a33e ("perf: Suppress AUX/OVERWRITE records")
> Reported-by: Ben Gainey <Ben.Gainey@xxxxxxx>
> CC: stable@xxxxxxxxxxxxxxx # 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.