linux-kernel - Re: [PATCH] perf: Allow suppressing AUX records

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 31 Mar 2018 11:35:46 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Arnaldo Carvalho de Melo <acme@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
        Will Deacon <will.deacon@....com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Markus Metzger <markus.t.metzger@...el.com>
Subject: Re: [PATCH] perf: Allow suppressing AUX records


* Peter Zijlstra <peterz@...radead.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@...ux.intel.com>
> > Cc: Markus Metzger <markus.t.metzger@...el.com>
> > Cc: Adrian Hunter <adrian.hunter@...el.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