Re: [PATCH v11 5/9] perf cs-etm: Use thread-stack for last branch entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Branch stack corruption and trace history loss in CPU-wide and idle-thread traces due to `thread_stack` sharing and global flushing.
--

commit 2e2bc54f5f904af76d7d22b2bf5c9fcfc4cc32e8
Author: Leo Yan <leo.yan@arm.com>

perf cs-etm: Use thread-stack for last branch entries

This commit replaces the private last branch circular array with the common
thread_stack helpers. It records taken branches with thread_stack__event()
and synthesizes PERF_SAMPLE_BRANCH_STACK data with thread_stack__br_sample().

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> @@ -1637,6 +1550,57 @@ static inline u64 cs_etm__resolve_sample_time(struct cs_etm_queue *etmq,
> +static void cs_etm__add_stack_event(struct cs_etm_queue *etmq,
> +				    struct cs_etm_traceid_queue *tidq)
> +{
> +	struct cs_etm_auxtrace *etm = etmq->etm;
> +	u64 from, to;
> +	int size;
> +
> +	if (!etm->synth_opts.branches && !etm->synth_opts.instructions)
> +		return;
> +
> +	if (!cs_etm__packet_has_taken_branch(tidq->prev_packet))
> +		return;
> +
> +	if (etmq->etm->synth_opts.last_branch) {
> +		from = cs_etm__last_executed_instr(tidq->prev_packet);
> +		to = cs_etm__first_executed_instr(tidq->packet);
> +
> +		size = cs_etm__instr_size(etmq, tidq, tidq->prev_packet, from);
> +
> +		/* Enable callchain so thread stack entry can be allocated */
> +		thread_stack__event(tidq->frontend_thread, tidq->prev_packet->cpu,
> +				    tidq->prev_packet->flags, from, to, size,
> +				    etmq->buffer->buffer_nr + 1, false,
> +				    tidq->br_stack_sz, 0);

[Severity: High]
In CPU-wide trace scenarios where queue->tid is -1,
cs_etm__init_traceid_queue() assigns the same dummy thread (PID -1, TID -1)
to tidq->frontend_thread across all queues.

Since thread_stack__per_cpu() returns false for PID -1, will all CPUs end up
sharing a single thread_stack?

If they share a single stack, interleaving cross-CPU decoding will pass
different etmq->buffer->buffer_nr + 1 trace numbers into
thread_stack__event(). Can this cause the stack to be continuously flushed
due to the mismatched trace numbers, leading to branch trace history loss?

[ ... ]

> @@ -2046,7 +1992,7 @@ static int cs_etm__flush(struct cs_etm_queue *etmq,
>  
>  	/* Reset last branches after flush the trace */
>  	if (etm->synth_opts.last_branch)
> -		cs_etm__reset_last_branch_rb(tidq);
> +		thread_stack__flush(tidq->frontend_thread);

[Severity: High]
For threads that do get per-CPU stacks, such as the idle thread (PID 0,
TID 0), thread_stack__flush() unconditionally iterates over and flushes the
stacks for all CPUs associated with that thread.

Does calling thread_stack__flush() here inadvertently wipe the branch trace
history for all CPUs on the idle thread, rather than performing a
queue-local reset for just the one CPU being processed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-b4-arm_cs_callchain_support_v1-v11-0-846abbf600d7@arm.com?part=5