Re: [PATCH v3 5/6] coresight: perf: Update buffer on AUX pause

[Suzuki K Poulose <suzuki.poulose@arm.com>]

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@arm.com>
> ---
>   .../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


> +	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,