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
