Re: [PATCH v11 6/9] perf cs-etm: Flush thread stacks after decoder reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] cs_etm__flush_all_stack() explicitly skips flushing thread stacks when the trace PID format is CS_ETM_PIDFMT_NONE.
- [Medium] Globally flushing all thread stacks on a per-queue decoder reset indiscriminately destroys valid callchains for unrelated threads on other CPUs.
--

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

perf cs-etm: Flush thread stacks after decoder reset

Perf resets the CoreSight decoder when moving to a new AUX trace buffer,
causing global trace discontinuity. This patch flushes all host (and
guest) thread stacks after a decoder reset to prevent stale call/return
histories from being attached to threads that are decoded later.

> diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c
> index ba9ffb7bdb4bd..996317687ce7b 100644
> --- a/tools/perf/util/cs-etm.c
> +++ b/tools/perf/util/cs-etm.c

[ ... ]

> +static int cs_etm__flush_stack_cb(struct thread *thread,
> +				  void *data __maybe_unused)
> +{
> +	thread_stack__flush(thread);
> +	return 0;
> +}
> +
> +static void cs_etm__flush_machine_stack(struct cs_etm_queue *etmq, pid_t pid)
> +{
> +	struct machine *machine;
> +
> +	machine = machines__find(&etmq->etm->session->machines, pid);
> +	if (machine)
> +		machine__for_each_thread(machine, cs_etm__flush_stack_cb, NULL);

[Severity: Medium]
Does this globally flush the stacks for all threads in the machine, even
those executing on other CPUs?

Since the decoder reset triggered in cs_etm__get_data_block() happens
per-queue (per-CPU) when it hits a buffer discontinuity, wiping the stack
for all threads indiscriminately might destroy the valid, continuous branch
stacks of threads executing on unrelated CPUs.

Should this flush be limited only to the threads actively traced by the
queue undergoing the reset?

> +}
> +
> +static void cs_etm__flush_all_stack(struct cs_etm_queue *etmq)
> +{
> +	enum cs_etm_pid_fmt pid_fmt = cs_etm__get_pid_fmt(etmq);
> +
> +	if (!etmq->etm->synth_opts.last_branch)
> +		return;
> +
> +	switch (pid_fmt) {
> +	case CS_ETM_PIDFMT_CTXTID2:
> +		/* Clear the guest stack if virtualization is supported */
> +		cs_etm__flush_machine_stack(etmq, DEFAULT_GUEST_KERNEL_ID);
> +		fallthrough;
> +	case CS_ETM_PIDFMT_CTXTID:
> +		cs_etm__flush_machine_stack(etmq, HOST_KERNEL_ID);
> +		break;
> +	case CS_ETM_PIDFMT_NONE:
> +	default:
> +		break;

[Severity: Medium]
When tracing without hardware context IDs or in per-thread mode, pid_fmt
evaluates to CS_ETM_PIDFMT_NONE.

By explicitly breaking here, doesn't this skip the flush entirely for these
traces? It seems this would leave the stale call and return histories intact
for those workloads, skipping the exact fix this patch intends to apply.

> +
> +	}
> +}

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