Re: [PATCH v2 04/14] perf cs-etm: Use per-CPU queues for per-thread mode

[James Clark <james.clark@linaro.org>]

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@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.