Re: [PATCH V8 02/12] perf/x86/intel/pt: Add support for pause / resume

Peter Zijlstra
2024-07-01T10:47:24Z

On Fri, Jun 28, 2024 at 09:51:01AM +0300, Adrian Hunter wrote:

> @@ -534,7 +537,20 @@ static void pt_config(struct perf_event *event)
> reg |= (event->attr.config & PT_CONFIG_MASK);
>
> event->hw.config = reg;
> +
> + /*
> + * Allow resume before starting so as not to overwrite a value set by a
> + * PMI.
> + */
> + WRITE_ONCE(pt->resume_allowed, 1);
barrier();
> pt_config_start(event);
barrier();
> + /*
> + * Allow pause after starting so its pt_config_stop() doesn't race with
> + * pt_config_start().
> + */
> + WRITE_ONCE(pt->pause_allowed, 1);

IIRC you need those barrier()s, because if the compiler 'helpfully'
inlines the static pt_config_start(), you loose your sequence point and
things can get re-ordered. WRITE_ONCE() only ensures the store is whole
and ordered against other volatile ops, but not against much else.

> }
>
> static void pt_config_stop(struct perf_event *event)
> @@ -1511,6 +1527,7 @@ void intel_pt_interrupt(void)
> buf = perf_aux_output_begin(&pt->handle, event);
> if (!buf) {
> event->hw.state = PERF_HES_STOPPED;
> + pt->resume_allowed = 0;
> return;
> }
>
> @@ -1519,6 +1536,7 @@ void intel_pt_interrupt(void)
> ret = pt_buffer_reset_markers(buf, &pt->handle);
> if (ret) {
> perf_aux_output_end(&pt->handle, 0);
> + pt->resume_allowed = 0;
> return;
> }
>

Above you WRITE_ONCE() on ->resume_allowed, here you do not. Some *SAN
thing or other is bound to get upset about things like that.

> @@ -1573,6 +1591,26 @@ static void pt_event_start(struct perf_event *event, int mode)
> struct pt *pt = this_cpu_ptr(&pt_ctx);
> struct pt_buffer *buf;
>
> + if (mode & PERF_EF_RESUME) {
> + if (READ_ONCE(pt->resume_allowed)) {

At this point I seem to have lost the plot, how do ->resume_allowed and
->aux_paused interact?

> + u64 status;
> +
> + /*
> + * Only if the trace is not active and the error and
> + * stopped bits are clear, is it safe to start, but a
> + * PMI might have just cleared these, so resume_allowed
> + * must be checked again also.
> + */
> + rdmsrl(MSR_IA32_RTIT_STATUS, status);
> + if (!(status & (RTIT_STATUS_TRIGGEREN |
> + RTIT_STATUS_ERROR |
> + RTIT_STATUS_STOPPED)) &&
> + READ_ONCE(pt->resume_allowed))
> + pt_config_start(event);
> + }
> + return;
> + }
> +
> buf = perf_aux_output_begin(&pt->handle, event);
> if (!buf)
> goto fail_stop;
> @@ -1601,6 +1639,16 @@ static void pt_event_stop(struct perf_event *event, int mode)
> {
> struct pt *pt = this_cpu_ptr(&pt_ctx);
>
> + if (mode & PERF_EF_PAUSE) {
> + if (READ_ONCE(pt->pause_allowed))
> + pt_config_stop(event);
> + return;
> + }
> +
> + /* Protect against racing */

No F1 cars allowed? Sure the comment can elucidate the reader as to what
actual race one is concerned about, no?

> + WRITE_ONCE(pt->pause_allowed, 0);
> + WRITE_ONCE(pt->resume_allowed, 0);
> +
> /*
> * Protect against the PMI racing with disabling wrmsr,
> * see comment in intel_pt_interrupt().
> @@ -1659,8 +1707,12 @@ static long pt_event_snapshot_aux(struct perf_event *event,
> /*
> * Here, handle_nmi tells us if the tracing is on
> */
> - if (READ_ONCE(pt->handle_nmi))
> + if (READ_ONCE(pt->handle_nmi)) {
> + /* Protect against racing */
> + WRITE_ONCE(pt->pause_allowed, 0);
> + WRITE_ONCE(pt->resume_allowed, 0);

barrier()?

> pt_config_stop(event);
> + }
>
> pt_read_offset(buf);
> pt_update_head(pt);
> @@ -1677,8 +1729,11 @@ static long pt_event_snapshot_aux(struct perf_event *event,
> * Compiler barrier not needed as we couldn't have been
> * preempted by anything that touches pt->handle_nmi.
> */
> - if (pt->handle_nmi)
> + if (pt->handle_nmi) {
> + WRITE_ONCE(pt->resume_allowed, 1);
> pt_config_start(event);
> + WRITE_ONCE(pt->pause_allowed, 1);

barrier() went missing again?

> + }
>
> return ret;
> }