Re: [PATCH] perf cs-etm: Fix missing decoder for per-process trace

[James Clark]

On 19/09/2023 23:45, Besar Wicaksono wrote:
> The decoder creation for raw trace uses metadata from the first CPU.
> On per-process/per-thread traces, the first CPU is CPU0. If CPU0 trace
> is not enabled, its metadata will be marked unused and the decoder is
> not created. Perf report dump skips the decoding part because the
> decoder is missing.
> 

Hi Besar,

It's not just per-process trace, the bug is also in per-cpu mode but it
means that the metadata from CPU 0 is used for every decoder which is
wrong. Although your change also fixes this issue.

> To fix this, use metadata of the CPU associated with sample object.
> 
> Signed-off-by: Besar Wicaksono <bwicaksono@xxxxxxxxxx>
> ---
>  tools/perf/util/cs-etm.c | 130 +++++++++++++++++++++++----------------
>  1 file changed, 77 insertions(+), 53 deletions(-)
> 

[...]

> +	if (!formatted) {
> +		/*
> +		 * There is only one decoder when unformatted. Use metadata of
> +		 * sample AUX cpu.
> +		 */
> +		t_param = t_params;
> +		metadata = get_cpu_data(etm, sample_cpu);
> +		if (!metadata) {
> +			pr_err("CS_ETM: invalid sample CPU: %d\n", sample_cpu);
>  			return -EINVAL;
>  		}

Apart from Mike's comments, this looks ok. Thanks for fixing this it has
been on our list for a while.

One issue with calling get_cpu_data() with the sample CPU ID is that it
won't work with old files that don't have the CPU sample flag set. Mike
added the sample flag fairly recently, and I don't think that was a
breaking change for old files at that time. It should be easy to avoid
that by still returning the metadata from CPU 0 when CPU = -1 (Which
isn't correct but is 99% likely to work).

I checked the Coresight tests and they're all passing, at least on a
system without ETE. If you could make sure they're all passing for you
as well that would be great:

  sudo ./perf test coresight

I think they currently only work from an in source build, if you get
stuck there.

Thanks
James