Messages in this thread | | | Date | Sat, 31 Mar 2018 11:35:46 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] perf: Allow suppressing AUX records |
| |
* Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Jan 15, 2018 at 05:00:20PM +0200, Alexander Shishkin wrote: > > It has been pointed out to me many times that it is useful to be able > > to switch off AUX records to save the bandwidth for records that actually > > matter, for example, in AUX overwrite mode. > > > > The usefulness of PERF_RECORD_AUX is in some of its flags, like the > > TRUNCATED flag that tells the decoder where exactly gaps in the trace are. > > The OVERWRITE flag, on the other hand will be set on every single record > > in overwrite mode. However, a PERF_RECORD_AUX[flags=OVERWRITE] is > > generated on every target task's sched_out, which over time adds up to > > a lot of useless information. > > > > In case the existing userspace depends on AUX records in the overwrite > > mode, we preserve the original behavior and add an opt-in for the new > > behavior, wherein the 'useless' records get suppressed. > > > > This patch adds an attribute bit to the described effect. > > > > Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com> > > Cc: Markus Metzger <markus.t.metzger@intel.com> > > Cc: Adrian Hunter <adrian.hunter@intel.com> > > --- > > include/uapi/linux/perf_event.h | 3 ++- > > kernel/events/core.c | 5 +++++ > > kernel/events/ring_buffer.c | 13 +++++++++++-- > > 3 files changed, 18 insertions(+), 3 deletions(-) > > > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > > index c77c9a2ebbbb..d7a981130561 100644 > > --- a/include/uapi/linux/perf_event.h > > +++ b/include/uapi/linux/perf_event.h > > @@ -370,7 +370,8 @@ struct perf_event_attr { > > context_switch : 1, /* context switch data */ > > write_backward : 1, /* Write ring buffer from end to beginning */ > > namespaces : 1, /* include namespaces data */ > > - __reserved_1 : 35; > > + suppress_aux : 1, /* don't generate PERF_RECORD_AUX */ > > + __reserved_1 : 34; > > > > union { > > __u32 wakeup_events; /* wakeup every n events */ > > So I'm basically fine with this patch, however I wonder if we really > need this suppress flag and can't just unconditionally drop these > events. > > Ash said that as far as he knows no Intel-PT user actually relies on it; > Will is there anything ARM that is known to rely on them? > > In anycase, tentative ACK on this, unless we wants to be brave and forgo > this flag. > > Ingo, any opinions?
Yeah, I'd suggest we just supress those record, and wait for complaints - let's not complicate the ABI if not necessary?
Thanks,
Ingo
| |