Rather than spreading AUX flag setting in different functions, use
trbe_get_fault_act() as a central place for setting the flag.
Later we will support WRAP mode with continuous trace, so the WRAP
event does not necessarily cause the trace discontinuity, change to
check the stop status instead.
Signed-off-by: Leo Yan <leo.yan@arm.com>
---
drivers/hwtracing/coresight/coresight-trbe.c | 38 +++++++++++++---------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
index 28e2bfa68074f19ccaa4a737d00af577aea818fe..b06885a08e082fd34f68d9588518807b5c47c86e 100644
--- a/drivers/hwtracing/coresight/coresight-trbe.c
+++ b/drivers/hwtracing/coresight/coresight-trbe.c
@@ -265,25 +265,6 @@ static void trbe_reset_local(struct trbe_cpudata *cpudata)
write_sysreg_s(0, SYS_TRBSR_EL1);
}
-static void trbe_report_wrap_event(struct perf_output_handle *handle)
-{
- /*
- * Mark the buffer to indicate that there was a WRAP event by
- * setting the COLLISION flag. This indicates to the user that
- * the TRBE trace collection was stopped without stopping the
- * ETE and thus there might be some amount of trace that was
- * lost between the time the WRAP was detected and the IRQ
- * was consumed by the CPU.
- *
- * Setting the TRUNCATED flag would move the event to STOPPED
- * state unnecessarily, even when there is space left in the
- * ring buffer. Using the COLLISION flag doesn't have this side
- * effect. We only set TRUNCATED flag when there is no space
- * left in the ring buffer.
- */
- perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
-}
-
static void trbe_truncate_event(struct perf_output_handle *handle)
{
struct trbe_buf *buf = etm_perf_sink_config(handle);
@@ -687,6 +668,23 @@ static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *hand
goto out_fatal;
}
+ /*
+ * Mark the buffer to indicate that there was a WRAP event by
+ * setting the COLLISION flag. This indicates to the user that
+ * the TRBE trace collection was stopped without stopping the
+ * ETE and thus there might be some amount of trace that was
+ * lost between the time the WRAP was detected and the IRQ
+ * was consumed by the CPU.
+ *
+ * Setting the TRUNCATED flag would move the event to STOPPED
+ * state unnecessarily, even when there is space left in the
+ * ring buffer. Using the COLLISION flag doesn't have this side
+ * effect. We only set TRUNCATED flag when there is no space
+ * left in the ring buffer.
+ */
+ if (!is_trbe_running(trbsr))
+ perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
+
if (is_trbe_wrap(trbsr))
return TRBE_FAULT_ACT_WRAP;
@@ -878,7 +876,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
goto done;
}
- trbe_report_wrap_event(handle);
wrap = true;
}
@@ -1099,7 +1096,6 @@ static int trbe_handle_overflow(struct perf_output_handle *handle)
if (buf->snapshot)
handle->head += size;
- trbe_report_wrap_event(handle);
perf_aux_output_end(handle, size);
event_data = perf_aux_output_begin(handle, event);
if (!event_data) {
--
2.34.1
On 01/12/2025 11:21 am, Leo Yan wrote:
> Rather than spreading AUX flag setting in different functions, use
> trbe_get_fault_act() as a central place for setting the flag.
>
> Later we will support WRAP mode with continuous trace, so the WRAP
> event does not necessarily cause the trace discontinuity, change to
> check the stop status instead.
>
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-trbe.c | 38 +++++++++++++---------------
> 1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 28e2bfa68074f19ccaa4a737d00af577aea818fe..b06885a08e082fd34f68d9588518807b5c47c86e 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -265,25 +265,6 @@ static void trbe_reset_local(struct trbe_cpudata *cpudata)
> write_sysreg_s(0, SYS_TRBSR_EL1);
> }
>
> -static void trbe_report_wrap_event(struct perf_output_handle *handle)
> -{
> - /*
> - * Mark the buffer to indicate that there was a WRAP event by
> - * setting the COLLISION flag. This indicates to the user that
> - * the TRBE trace collection was stopped without stopping the
> - * ETE and thus there might be some amount of trace that was
> - * lost between the time the WRAP was detected and the IRQ
> - * was consumed by the CPU.
> - *
> - * Setting the TRUNCATED flag would move the event to STOPPED
> - * state unnecessarily, even when there is space left in the
> - * ring buffer. Using the COLLISION flag doesn't have this side
> - * effect. We only set TRUNCATED flag when there is no space
> - * left in the ring buffer.
> - */
> - perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> -}
> -
> static void trbe_truncate_event(struct perf_output_handle *handle)
> {
> struct trbe_buf *buf = etm_perf_sink_config(handle);
> @@ -687,6 +668,23 @@ static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *hand
> goto out_fatal;
> }
>
> + /*
> + * Mark the buffer to indicate that there was a WRAP event by
> + * setting the COLLISION flag. This indicates to the user that
> + * the TRBE trace collection was stopped without stopping the
> + * ETE and thus there might be some amount of trace that was
> + * lost between the time the WRAP was detected and the IRQ
> + * was consumed by the CPU.
> + *
> + * Setting the TRUNCATED flag would move the event to STOPPED
> + * state unnecessarily, even when there is space left in the
> + * ring buffer. Using the COLLISION flag doesn't have this side
> + * effect. We only set TRUNCATED flag when there is no space
> + * left in the ring buffer.
> + */
> + if (!is_trbe_running(trbsr))
> + perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> +
Do we need the complexity of COLLISION (changed to PARTIAL in a later
commit) and TRUNCATED at the same time?
From my understanding, TRUNCATED would be when Perf is too slow and we
want to disable the event for it to catch up, and PARTIAL is when there
was some buffer error so technically we don't need the event to be
disabled, but Perf still needs to be notified if we want to start only
resetting the decoder when a flag is set rather than on all aux records?
There is no happy path where there is a buffer error, so I'm not sure
why we need the complexity of an extra case? There's no harm in
disabling the event on a buffer error. Also it's hard to see what the
exact scenario for PARTIAL is, because we set truncated anyway for fatal
statuses. The comment in the later commit for the PARTIAL change just
says "trace was stopped", but not why.
> if (is_trbe_wrap(trbsr))
> return TRBE_FAULT_ACT_WRAP;
>
> @@ -878,7 +876,6 @@ static unsigned long arm_trbe_update_buffer(struct coresight_device *csdev,
> goto done;
> }
>
> - trbe_report_wrap_event(handle);
> wrap = true;
> }
>
> @@ -1099,7 +1096,6 @@ static int trbe_handle_overflow(struct perf_output_handle *handle)
> if (buf->snapshot)
> handle->head += size;
>
> - trbe_report_wrap_event(handle);
> perf_aux_output_end(handle, size);
> event_data = perf_aux_output_begin(handle, event);
> if (!event_data) {
>
On Tue, Dec 09, 2025 at 01:37:39PM +0000, James Clark wrote: [...] > > + if (!is_trbe_running(trbsr)) > > + perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION); > > + > > Do we need the complexity of COLLISION (changed to PARTIAL in a later > commit) and TRUNCATED at the same time? > From my understanding, TRUNCATED would be when Perf is too slow and we want > to disable the event for it to catch up, and PARTIAL is when there was some > buffer error so technically we don't need the event to be disabled, but Perf > still needs to be notified if we want to start only resetting the decoder > when a flag is set rather than on all aux records? The main difference between the TRUNCATED flag and the COLLISION/PARTIAL flags is that the TRUNCATED flag implicitly asks the event core layer to disable the PMU event. In contrast, the COLLISION and PARTIAL flags are only stored in AUX record and no additional action in the event core layer. Setting the TRUNCATED flag would be a over-killer if the buffer has free space, as it would run the expensive sequence of disabling both ETE and TRBE and then requiring userspace to re-enable them. > There is no happy path where there is a buffer error, so I'm not sure why we > need the complexity of an extra case? There's no harm in disabling the event > on a buffer error. Also it's hard to see what the exact scenario for PARTIAL > is, because we set truncated anyway for fatal statuses. The comment in the > later commit for the PARTIAL change just says "trace was stopped", but not > why. The trace can be stopped on Fill mode (stop on wrap) or Stop on trigger, so it is not (only) about buffer error. In these cases, even though the trace unit is stopped, the buffer may still have space available, tracing can be directly re-enabled in interrupt handler, thus the COLLISION/PARTIAL flags are better choices. Only when the buffer is actually used out (i.e., handle->size == 0), it is appropriate to set TRUNCATED flag, since at that point we must wait for userspace to read out the data. Thanks, Leo
On 10/12/2025 17:43, Leo Yan wrote: > On Tue, Dec 09, 2025 at 01:37:39PM +0000, James Clark wrote: > > [...] > >>> + if (!is_trbe_running(trbsr)) >>> + perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION); >>> + >> >> Do we need the complexity of COLLISION (changed to PARTIAL in a later >> commit) and TRUNCATED at the same time? > >> From my understanding, TRUNCATED would be when Perf is too slow and we want >> to disable the event for it to catch up, and PARTIAL is when there was some >> buffer error so technically we don't need the event to be disabled, but Perf >> still needs to be notified if we want to start only resetting the decoder >> when a flag is set rather than on all aux records? > > The main difference between the TRUNCATED flag and the COLLISION/PARTIAL > flags is that the TRUNCATED flag implicitly asks the event core layer to > disable the PMU event. In contrast, the COLLISION and PARTIAL flags are > only stored in AUX record and no additional action in the event core > layer. > > Setting the TRUNCATED flag would be a over-killer if the buffer has > free space, as it would run the expensive sequence of disabling both ETE > and TRBE and then requiring userspace to re-enable them. > >> There is no happy path where there is a buffer error, so I'm not sure why we >> need the complexity of an extra case? There's no harm in disabling the event >> on a buffer error. Also it's hard to see what the exact scenario for PARTIAL >> is, because we set truncated anyway for fatal statuses. The comment in the >> later commit for the PARTIAL change just says "trace was stopped", but not >> why. > > The trace can be stopped on Fill mode (stop on wrap) or Stop on trigger, so > it is not (only) about buffer error. In these cases, even though the trace > unit is stopped, the buffer may still have space available, tracing can be > directly re-enabled in interrupt handler, thus the COLLISION/PARTIAL flags > are better choices. I suppose this is the bit that I don't understand. If there is space left then why is the stop hit at all? For the other cases, I agree that it's overkill, but my point is that doesn't matter because we don't expect it to be hit in normal cases anyway. So it's worth the simplification of not doing anything different than TRUNCATED. > > Only when the buffer is actually used out (i.e., handle->size == 0), it is > appropriate to set TRUNCATED flag, since at that point we must wait for > userspace to read out the data. > > Thanks, > Leo
On Fri, Dec 12, 2025 at 04:50:52PM +0200, James Clark wrote: [...] > > The trace can be stopped on Fill mode (stop on wrap) or Stop on trigger, so > > it is not (only) about buffer error. In these cases, even though the trace > > unit is stopped, the buffer may still have space available, tracing can be > > directly re-enabled in interrupt handler, thus the COLLISION/PARTIAL flags > > are better choices. > > I suppose this is the bit that I don't understand. If there is space left > then why is the stop hit at all? For instance, the buffer becomes empty in the middle: +----------------------------+ |$$$$$$$| |$$$$$$$| +----------------------------+ ` base `wptr `limit In this case, the trace unit stops when it reaches the limit. Meanwhile, the tool might have already read out the old trace data so we will get some free space, thus no need to set TRUNCATED flag.
On 12/12/2025 17:27, Leo Yan wrote:
> On Fri, Dec 12, 2025 at 04:50:52PM +0200, James Clark wrote:
>
> [...]
>
>>> The trace can be stopped on Fill mode (stop on wrap) or Stop on trigger, so
>>> it is not (only) about buffer error. In these cases, even though the trace
>>> unit is stopped, the buffer may still have space available, tracing can be
>>> directly re-enabled in interrupt handler, thus the COLLISION/PARTIAL flags
>>> are better choices.
>>
>> I suppose this is the bit that I don't understand. If there is space left
>> then why is the stop hit at all?
>
> For instance, the buffer becomes empty in the middle:
>
> +----------------------------+
> |$$$$$$$| |$$$$$$$|
> +----------------------------+
> ` base `wptr `limit
>
> In this case, the trace unit stops when it reaches the limit.
> Meanwhile, the tool might have already read out the old trace data so
> we will get some free space, thus no need to set TRUNCATED flag.
Hmmm I see, I still think that means that Perf is being too slow and
disabling the event wouldn't be that bad.
Also isn't there a very small chance to hit this? I'm wondering if you
ever saw PARTIAL flags in practice?
I think the following all have to be true:
* 3 out of 4 sections were full on the last interrupt
* Perf hadn't managed to finish reading even 1 section in that time
* The last section fills up
* Perf had managed to finish reading 1 section between the time of
the previous interrupt and the hard stop
There would have to be quite a high variability in read speed to make
that possible. Maybe it happens and it's worthwhile optimizing for. The
comments could explain it better if we do decide to keep it.
On 01/12/2025 11:21, Leo Yan wrote:
> Rather than spreading AUX flag setting in different functions, use
> trbe_get_fault_act() as a central place for setting the flag.
>
> Later we will support WRAP mode with continuous trace, so the WRAP
> event does not necessarily cause the trace discontinuity, change to
> check the stop status instead.
>
> Signed-off-by: Leo Yan <leo.yan@arm.com>
> ---
> drivers/hwtracing/coresight/coresight-trbe.c | 38 +++++++++++++---------------
> 1 file changed, 17 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/hwtracing/coresight/coresight-trbe.c b/drivers/hwtracing/coresight/coresight-trbe.c
> index 28e2bfa68074f19ccaa4a737d00af577aea818fe..b06885a08e082fd34f68d9588518807b5c47c86e 100644
> --- a/drivers/hwtracing/coresight/coresight-trbe.c
> +++ b/drivers/hwtracing/coresight/coresight-trbe.c
> @@ -265,25 +265,6 @@ static void trbe_reset_local(struct trbe_cpudata *cpudata)
> write_sysreg_s(0, SYS_TRBSR_EL1);
> }
>
> -static void trbe_report_wrap_event(struct perf_output_handle *handle)
> -{
> - /*
> - * Mark the buffer to indicate that there was a WRAP event by
> - * setting the COLLISION flag. This indicates to the user that
> - * the TRBE trace collection was stopped without stopping the
> - * ETE and thus there might be some amount of trace that was
> - * lost between the time the WRAP was detected and the IRQ
> - * was consumed by the CPU.
> - *
> - * Setting the TRUNCATED flag would move the event to STOPPED
> - * state unnecessarily, even when there is space left in the
> - * ring buffer. Using the COLLISION flag doesn't have this side
> - * effect. We only set TRUNCATED flag when there is no space
> - * left in the ring buffer.
> - */
> - perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> -}
> -
> static void trbe_truncate_event(struct perf_output_handle *handle)
> {
> struct trbe_buf *buf = etm_perf_sink_config(handle);
> @@ -687,6 +668,23 @@ static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *hand
> goto out_fatal;
> }
>
> + /*
> + * Mark the buffer to indicate that there was a WRAP event by
> + * setting the COLLISION flag. This indicates to the user that
> + * the TRBE trace collection was stopped without stopping the
> + * ETE and thus there might be some amount of trace that was
> + * lost between the time the WRAP was detected and the IRQ
> + * was consumed by the CPU.
> + *
> + * Setting the TRUNCATED flag would move the event to STOPPED
> + * state unnecessarily, even when there is space left in the
> + * ring buffer. Using the COLLISION flag doesn't have this side
> + * effect. We only set TRUNCATED flag when there is no space
> + * left in the ring buffer.
> + */
> + if (!is_trbe_running(trbsr))
Is this really required ? There is a WARN_ON(is_trbe_running(trbsr))
above ?
> + perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
Also, setting this would unnecessarily mark COLLISION while stopping the
TRBE buffer normally (when called from arm_trbe_update_buffer), when
there is no WRAP event ?
> +
> if (is_trbe_wrap(trbsr))
> return TRBE_FAULT_ACT_WRAP;
Couldn't this be :
/* Move the big fat comment here */
if (is_trbe_wrap(trbsr)) {
perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
return TRBE_FAULT_ACT_WRAP;
}
Suzuki
On Tue, Dec 02, 2025 at 11:15:47AM +0000, Suzuki K Poulose wrote:
[...]
> > @@ -687,6 +668,23 @@ static enum trbe_fault_action trbe_get_fault_act(struct perf_output_handle *hand
> > goto out_fatal;
> > }
> > + /*
> > + * Mark the buffer to indicate that there was a WRAP event by
> > + * setting the COLLISION flag. This indicates to the user that
> > + * the TRBE trace collection was stopped without stopping the
> > + * ETE and thus there might be some amount of trace that was
> > + * lost between the time the WRAP was detected and the IRQ
> > + * was consumed by the CPU.
> > + *
> > + * Setting the TRUNCATED flag would move the event to STOPPED
> > + * state unnecessarily, even when there is space left in the
> > + * ring buffer. Using the COLLISION flag doesn't have this side
> > + * effect. We only set TRUNCATED flag when there is no space
> > + * left in the ring buffer.
> > + */
> > + if (!is_trbe_running(trbsr))
>
> Is this really required ? There is a WARN_ON(is_trbe_running(trbsr))
> above ?
The WARN_ON() will reports warning for running case, here it sets
COLLISION flag for not running case, which is not conflict.
This works for the Fill mode (stop collection), we always set the
COLLISION flag for it. Once we enable trigger and wrap modes, due to
the trace continues to run, we don't set the flag for these modes.
> > + perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
>
> Also, setting this would unnecessarily mark COLLISION while stopping the
> TRBE buffer normally (when called from arm_trbe_update_buffer), when
> there is no WRAP event ?
Good point. I should refactor arm_trbe_update_buffer(), it firstly
reads out TRBSR, stop trace afterwards, and then call
trbe_get_fault_act().
In this case, it is in running mode so COLLISION will not be set for
normal disable flow.
I will add a new patch for this and place it before this one.
> > +
> > if (is_trbe_wrap(trbsr))
> > return TRBE_FAULT_ACT_WRAP;
>
> Couldn't this be :
>
> /* Move the big fat comment here */
> if (is_trbe_wrap(trbsr)) {
> perf_aux_output_flag(handle, PERF_AUX_FLAG_COLLISION);
> return TRBE_FAULT_ACT_WRAP;
> }
It deliberately de-couples the COLLISION flag from the WRAP event.
In later change, a WRAP event only means buffer wrap around, it does
mean the trace has been stopped. The AUX flag is only set for
stopping case.
Thanks,
Leo
© 2016 - 2026 Red Hat, Inc.