[PATCH v2 04/14] perf cs-etm: Use per-CPU queues for per-thread mode
James Clark
james.clark at linaro.org
Wed Oct 21 02:20:54 PDT 2026
On 26/08/2026 20:06, Leo Yan wrote:
> On Fri, Aug 21, 2026 at 10:49:02AM +0100, James Clark wrote:
>> This fixes two problems, firstly per-thread mode didn't support multiple
>> threads well or at all because we always use queue 0 and assumed it was
>> for a single thread. However, Perf attaches to all child threads of one
>> process or allows multiple threads to be specified on the commandline.
>> This meant that trace before a context packet could use the wrong MMAP
>> for decoding, and threads running concurrently wouldn't be interleaved
>> properly.
>
> If I connect this with patch 03, I understand "a context packet" here as
> the trace synchronization with a timestamp, which is needed to correlate
> with MMAP records.
>
> What I don't quite understand is how switching to multiple queues fixes
> the "trace before a context packet" case. Wouldn't decoding still need
> to wait until the first timestamp/context information is available?
>
Context IDs aren't enabled in per-thread mode, we still use the TID
initially assigned to the queue for mmap lookup. That TID comes from
"etmq->buffer->tid" which is always correct in per-thread mode. Even if
we turned on context IDs they might either be cropped in snapshot mode,
or not exist when we drop the PID_IN_CONTEXTIDR hard requirement.
The commit message is a bit misleading, it should say "trace before a
trace synchronization *timestamp*".
>> The second problem is that we'd put all HW_IDs into queue 0, so if a
>> thread runs on multiple CPUs then it would receive multiple HW_IDs and
>> trigger the assert that unformatted trace should only have 1 decoder
>> per-CPU:
>>
>> $ perf record --per-thread -e cs_etm//u -- taskset --cpu-list 0 \
>> taskset --cpu-list 1 true
>> $ perf script
>>
>> perf: util/cs-etm.c:3381: cs_etm__create_queue_decoders: Assertion `decoders == 1' failed.
>
> Seems the commit log misses to explain how to fix issue.
>
This issue is gone now, V3 will be based on "perf: cs-etm: HW_ID
improvements" which doesn't read HW_IDs at all for unformatted mode.
I'll remove this and the fixes tag.
For information, the fix explanation was:
"The second problem is that we'd put all HW_IDs into queue 0" ...
making that queue have multiple decoders ... Fix it by not special
casing queue 0 in per-thread mode in cs_etm__get_queue(), so each
HW_ID goes into it's correct per-CPU queue and leaves only 1 decoder"
>> Fixes: 9182f04a85b2 ("perf cs-etm: Pass unformatted flag to decoder")
>> Signed-off-by: James Clark <james.clark at linaro.org>
>> ---
>> tools/perf/util/cs-etm.c | 30 +++++++++++++++++++++++++-----
>> 1 file changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
>> index d423f1cb408f..ccf886efff95 100644
>> --- a/tools/perf/util/cs-etm.c
>> +++ b/tools/perf/util/cs-etm.c
>> @@ -295,9 +295,6 @@ static int cs_etm__insert_trace_id_node(struct cs_etm_queue *etmq,
>>
>> static struct cs_etm_queue *cs_etm__get_queue(struct cs_etm_auxtrace *etm, int cpu)
>> {
>> - if (etm->per_thread_decoding)
>> - return etm->queues.queue_array[0].priv;
>> -
>> if (cpu < 0 || cpu >= (int)etm->queues.nr_queues)
>> return NULL;
>>
>> @@ -1268,7 +1265,7 @@ static int cs_etm__setup_queue(struct cs_etm_auxtrace *etm,
>> queue->priv = etmq;
>> etmq->etm = etm;
>> etmq->queue_nr = queue_nr;
>> - queue->cpu = queue_nr; /* Placeholder, may be reset to -1 in per-thread mode */
>> + queue->cpu = queue_nr;
>> etmq->offset = 0;
>> etmq->sink_id = SINK_UNSET;
>>
>> @@ -2160,6 +2157,7 @@ static void cs_etm__flush_all_stack(struct cs_etm_queue *etmq)
>> */
>> static int cs_etm__get_data_block(struct cs_etm_queue *etmq)
>> {
>> + struct cs_etm_auxtrace *etm = etmq->etm;
>> int ret;
>>
>> /* The current block is not finished */
>> @@ -2188,6 +2186,27 @@ static int cs_etm__get_data_block(struct cs_etm_queue *etmq)
>> */
>> cs_etm__flush_all_stack(etmq);
>>
>> + /*
>> + * Per-thread mode still uses a queue for each CPU, but that CPU can run
>> + * different threads. When the TID from the AUX record on a CPU changes,
>> + * re-initialize the thread using the AUX record/buffer fragment TID so
>> + * we can start decoding even if the context ID packet was cropped or
>> + * they're disabled.
>> + */
>> + if (etm->per_thread_decoding) {
>> + struct cs_etm_traceid_queue *tidq = cs_etm__etmq_get_traceid_queue(etmq,
>> + CS_ETM_PER_THREAD_TRACEID);
>> +
>> + if (thread__tid(tidq->decode_thread) != etmq->buffer->tid) {
>> + thread__zput(tidq->frontend_thread);
>> + thread__zput(tidq->decode_thread);
>> + tidq->frontend_thread = machine__findnew_thread(&etm->session->machines.host,
>> + -1, etmq->buffer->tid);
>> + tidq->decode_thread = machine__findnew_thread(&etm->session->machines.host,
>> + -1, etmq->buffer->tid);
>> + }
>> + }
>
> This works if the auxtrace fragment initially represents trace from that
> TID. However, doesn't this introduce an assumption that an AUX fragment
> contains trace from only one thread?
>
> It seems this assumption impacts the kernel patch [1], the tool's
> implementatoin imposes the limitation that driver to cannot support
> multiple threads run concurrently with shared sink.
Seems like we've decided to prohibit multiple per-thread events with
shared sinks, so we can keep the assumption that each fragment only
contains trace from buffer->tid. This is similar to the assumption in
the aux sampling patches where we initialise the thread with the TID of
the sample.
>
> Would it be better to enable contextidr for per-thread mode and rely on
> Context ID packets to update the thread context? We could then reuse
> cs_etm__etmq_update_thread() / cs_etm__etmq_update_decode_context()
> rather than relying on the AUX fragment TID for this purpose.
>
> [1] https://lore.kernel.org/linux-arm-kernel/d833aeed-7376-48af-b772-4fe4ac66471d@linaro.org/
This comment is probably a bit stale now given the latest version of [1]
where a per-thread event can only contain trace from one thread. So the
context packets aren't useful. I thought the plan was to eventually drop
the requirement for PID_IN_CONTEXTIDR and rely on timestamps anyway, so
it doesn't make sense to add it in more places.
More information about the linux-arm-kernel
mailing list