On Tue, 1 Apr 2025 at 13:52, Suzuki K Poulose <suzuki.poulose@xxxxxxx> wrote:
>
> On 11/03/2025 17:04, Leo Yan wrote:
> > Due to sinks like ETR and ETB don't support interrupt handling, the
> > hardware trace data might be lost for continuous running tasks.
> >
> > This commit takes advantage of the AUX pause for updating trace buffer
> > to mitigate the trace data losing issue.
> >
> > The per CPU sink has its own interrupt handling. Thus, there will be a
> > race condition between the updating buffer in NMI and sink's interrupt
> > handler. To avoid the race condition, this commit disallows updating
> > buffer on AUX pause for the per CPU sink. Currently, this is only
> > applied for TRBE.
> >
> > Signed-off-by: Leo Yan <leo.yan@xxxxxxx>
> > ---
> > .../hwtracing/coresight/coresight-etm-perf.c | 43 ++++++++++++++++++-
> > 1 file changed, 41 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > index 2dcf1809cb7f..f1551c08ecb2 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm-perf.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c
> > @@ -574,14 +574,53 @@ static void etm_event_start(struct perf_event *event, int flags)
> > return;
> > }
> >
> > -static void etm_event_pause(struct coresight_device *csdev,
> > +static void etm_event_pause(struct perf_event *event,
> > + struct coresight_device *csdev,
> > struct etm_ctxt *ctxt)
> > {
> > + int cpu = smp_processor_id();
> > + struct coresight_device *sink;
> > + struct perf_output_handle *handle = &ctxt->handle;
> > + struct coresight_path *path;
> > + unsigned long size;
> > +
> > if (!ctxt->event_data)
> > return;
> >
> > /* Stop tracer */
> > coresight_pause_source(csdev);
> > +
> > + path = etm_event_cpu_path(ctxt->event_data, cpu);
> > + sink = coresight_get_sink(path);
> > + if (WARN_ON_ONCE(!sink))
> > + return;
> > +
> > + /*
> > + * The per CPU sink has own interrupt handling, it might have
> > + * race condition with updating buffer on AUX trace pause if
> > + * it is invoked from NMI. To avoid the race condition,
> > + * disallows updating buffer for the per CPU sink case.
> > + */
> > + if (coresight_is_percpu_sink(sink))
> > + return;
> > +
> > + if (WARN_ON_ONCE(handle->event != event))
> > + return;
> > +
> > + if (!sink_ops(sink)->update_buffer)
> > + return;
> > +
> > + size = sink_ops(sink)->update_buffer(sink, handle,
> > + ctxt->event_data->snk_config);
>
> I believe we keep the sink disabled/stopped in update_buffer. We need to
> turn it back ON after the "buffer update". May be we could use a flag
> to update_buffer() to indicate this is "pause" triggered update.
>
> Cheers
> Suzuki
>
The sink is stopped to read data, but also important is the
enable/disable refcount. The only time that "update_buffer" will read
is if the sink has a refcount == 1. These are ultimately controlled by
enable/disable path - which will not occur during pause/resume
operations.
Regards
Mike
>
> > + if (READ_ONCE(handle->event)) {
> > + if (!size)
> > + return;
> > +
> > + perf_aux_output_end(handle, size);
> > + perf_aux_output_begin(handle, event);
> > + } else {
> > + WARN_ON_ONCE(size);
> > + }
> > }
> >
> > static void etm_event_stop(struct perf_event *event, int mode)
> > @@ -595,7 +634,7 @@ static void etm_event_stop(struct perf_event *event, int mode)
> > struct coresight_path *path;
> >
> > if (mode & PERF_EF_PAUSE)
> > - return etm_event_pause(csdev, ctxt);
> > + return etm_event_pause(event, csdev, ctxt);
> >
> > /*
> > * If we still have access to the event_data via handle,
>
--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK
