linux-kernel - Re: [PATCH V9 02/13] perf/core: Add aux_pause, aux_resume, aux_start

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]

Message-ID: <14cd68b2-eeee-42e3-87a6-c12d3814bd51@intel.com>
Date: Thu, 18 Jul 2024 14:19:03 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Heiko Carstens <hca@...ux.ibm.com>, Thomas Richter <tmricht@...ux.ibm.com>,
 Hendrik Brueckner <brueckner@...ux.ibm.com>,
 Suzuki K Poulose <suzuki.poulose@....com>, Mike Leach
 <mike.leach@...aro.org>, James Clark <james.clark@....com>,
 coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org,
 Yicong Yang <yangyicong@...ilicon.com>,
 Jonathan Cameron <jonathan.cameron@...wei.com>, Will Deacon
 <will@...nel.org>, Arnaldo Carvalho de Melo <acme@...nel.org>,
 Jiri Olsa <jolsa@...nel.org>, Namhyung Kim <namhyung@...nel.org>,
 Ian Rogers <irogers@...gle.com>, Andi Kleen <ak@...ux.intel.com>,
 linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org
Subject: Re: [PATCH V9 02/13] perf/core: Add aux_pause, aux_resume,
 aux_start_paused

On 18/07/24 12:38, Peter Zijlstra wrote:
> On Mon, Jul 15, 2024 at 07:07:01PM +0300, Adrian Hunter wrote:
>> Hardware traces, such as instruction traces, can produce a vast amount of
>> trace data, so being able to reduce tracing to more specific circumstances
>> can be useful.
>>
>> The ability to pause or resume tracing when another event happens, can do
>> that.
>>
>> Add ability for an event to "pause" or "resume" AUX area tracing.
>>
>> Add aux_pause bit to perf_event_attr to indicate that, if the event
>> happens, the associated AUX area tracing should be paused. Ditto
>> aux_resume. Do not allow aux_pause and aux_resume to be set together.
>>
>> Add aux_start_paused bit to perf_event_attr to indicate to an AUX area
>> event that it should start in a "paused" state.
>>
>> Add aux_paused to struct hw_perf_event for AUX area events to keep track of
>> the "paused" state. aux_paused is initialized to aux_start_paused.
>>
>> Add PERF_EF_PAUSE and PERF_EF_RESUME modes for ->stop() and ->start()
>> callbacks. Call as needed, during __perf_event_output(). Add
> 
> Why in __perf_event_output() rather than in __perf_event_overflow().
> Specifically, before bpf_overflow_handler().
> 
> That is, what do we want BPF to be able to do here? To me it seems
> strange that BPF would be able to affect this functionality. You want
> this pause/resume to happen irrespective of how the rest of the event is
> processed, no?

The thought was to have the output match up with pause/resume, but it
doesn't really make much difference.

Putting it before the BPF handler is reasonable.

> 
>> aux_in_pause_resume to struct perf_buffer to prevent races with the NMI
>> handler. Pause/resume in NMI context will miss out if it coincides with
>> another pause/resume.
> 
> I'm struggling here. That variable is only ever used inside that one
> function. So it must be self-recursion. Are you thinking something like:
> 
>   swevent_overflow()
>     ...
>       event_aux_pause()
>         <NMI>
> 	  event_overflow()
> 	    ...
> 	      event_aux_pause()
> 
> ?
> 
> Where two events in the group, one software and one hardware, are both
> trying to control the AUX thing?

Exactly that yes.

>                                  Because I don't think the PT-PMI ever
> gets here.

No it doesn't.  AUX pause/resume is something a non-AUX event in the
group does to the AUX event which is the group leader.

> 
>> To use aux_pause or aux_resume, an event must be in a group with the AUX
>> area event as the group leader.
> 
> 
>> @@ -402,6 +411,15 @@ struct pmu {
>>  	 *
>>  	 * ->start() with PERF_EF_RELOAD will reprogram the counter
>>  	 *  value, must be preceded by a ->stop() with PERF_EF_UPDATE.
>> +	 *
>> +	 * ->stop() with PERF_EF_PAUSE will stop as simply as possible. Will not
>> +	 * overlap another ->stop() with PERF_EF_PAUSE nor ->start() with
>> +	 * PERF_EF_RESUME.
>> +	 *
>> +	 * ->start() with PERF_EF_RESUME will start as simply as possible but
>> +	 * only if the counter is not otherwise stopped. Will not overlap
>> +	 * another ->start() with PERF_EF_RESUME nor ->stop() with
>> +	 * PERF_EF_PAUSE.
>>  	 */
>>  	void (*start)			(struct perf_event *event, int flags);
>>  	void (*stop)			(struct perf_event *event, int flags);
> 
> Notably, they *can* race with ->stop/start without EF_PAUSE/RESUME.

Yes that would be worth adding to the comments.