Re: [PATCH v1 9/9] perf arm-spe: Dump metadata with version 2

On 8/28/24 17:20, James Clark wrote:

quoted

+static void arm_spe_print_info(struct arm_spe *spe, __u64 *arr)
  {
+     unsigned int i, cpu, header_size, cpu_num, per_cpu_size;
+
      if (!dump_trace)
              return;

-     fprintf(stdout, arm_spe_info_fmts[ARM_SPE_PMU_TYPE], arr[ARM_SPE_PMU_TYPE]);
+     if (spe->metadata_ver == 1) {
+             cpu_num = 0;
+             header_size = ARM_SPE_AUXTRACE_V1_PRIV_MAX;
+             per_cpu_size = 0;
+     } else if (spe->metadata_ver == 2) {

Assuming future version updates are backwards compatible and only add
new info this should be spe->metadata_ver >= 2, otherwise version bumps
end up causing errors when files get passed around.

I know there are arguments about what should and shouldn't be supported
when opening new files on old perfs, but in this case it's easy to only
add new info to the aux header and leave the old stuff intact.

quoted

+             cpu_num = arr[ARM_SPE_CPU_NUM];
+             header_size = ARM_SPE_AUXTRACE_V2_PRIV_MAX;
+             per_cpu_size = ARM_SPE_AUXTRACE_V2_PRIV_PER_CPU_MAX;

I think for coresight we also save the size of each per-cpu block rather
than use a constant, that way new items can be appended without breaking
readers.

Good point for adding a 'size' field for each per-cpu block.

My understanding is we need to make the metadata format to be self-described.
E.g. the metadata header contains the size for itself, and every per CPU
metadata block also contains a 'size' field.  Based on this, a general code
can be used to processing different metadata versions.
  

That kind of leads to another point that this mechanism is mostly
duplicated from coresight. It saves a main header version, then per-cpu
groups of variable size with named elements. I'm not saying we should
definitely try to share the code, but it's worth keeping in mind.

Agreed. I will refine a bit, for better matching this direction.

Thanks for suggestion.

Leo