Re: [PATCH v1 3/7] perf cs-etm: Calculate per CPU metadata array size

Hi Mike,

On Wed, Jan 13, 2021 at 12:00:10AM +0000, Mike Leach wrote:

[...]

...

diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index a2a369e2fbb6..edaec57362f0 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -2558,12 +2558,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -ENOMEM; goto err_free_metadata; }

•                   for (k = 0; k < CS_ETM_PRIV_MAX; k++)
  

•                   for (k = 0; k < CS_ETM_PRIV_MAX; k++) {
                            metadata[j][k] = ptr[i + k];
  
  

•                           if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
  

•                               ptr[i + k + 1] == __perf_cs_etmv4_magic) {
  

•                                   k++;
  

•                                   break;
  

•                           }
  

•                   }
  

•                   /* The traceID is our handle */
                    idx = metadata[j][CS_ETM_ETMTRACEIDR];
  

•                   i += CS_ETM_PRIV_MAX;
  

•                   i += k;
            } else if (ptr[i] == __perf_cs_etmv4_magic) {
                    metadata[j] = zalloc(sizeof(*metadata[j]) *
                                         CS_ETMV4_PRIV_MAX);
  

@@ -2571,12 +2578,19 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -ENOMEM; goto err_free_metadata; }

•                   for (k = 0; k < CS_ETMV4_PRIV_MAX; k++)
  

•                   for (k = 0; k < CS_ETMV4_PRIV_MAX; k++) {
                            metadata[j][k] = ptr[i + k];
  
  

•                           if (ptr[i + k + 1] == __perf_cs_etmv3_magic ||
  

•                               ptr[i + k + 1] == __perf_cs_etmv4_magic) {
  

•                                   k++;
  

•                                   break;
  

•                           }
  

•                   }
  

•                   /* The traceID is our handle */
                    idx = metadata[j][CS_ETMV4_TRCTRACEIDR];
  

•                   i += CS_ETMV4_PRIV_MAX;
  

•                   i += k;
            }
  
            /* Get an RB node for this CPU */
  

That would be a spot fix for the read /copy case, but will not fix the print routine which will still bail out on older versions of the format. (when using perf report --dump).

The "self describing" format I have been looking at will add an NR_PARAMS value to the common block in the CPU metadata parameter list, increment the header version to '1' and update the format writer to use the version 1 format while having the reader understand both v0 and v1 formats.

i..e in cs-etm.h perf I add: /*

• Update the version for new format.
• New version 1 format adds a param count to the per cpu metadata.
• This allows easy adding of new metadata parameters.
• Requires that new params always added after current ones.
• Also allows client reader to handle file versions that are different by
• checking the number of params in the file vs the number expected.

*/ #define CS_HEADER_CURRENT_VERSION 1

/* Beginning of header common to both ETMv3 and V4 */ enum { CS_ETM_MAGIC, CS_ETM_CPU, CS_ETM_NR_PARAMS, /* number of parameters to follow in this block */ };

where in verison 1, NR_PARAMS indicates the total number of params that follow - so adding new parameters can be added to the metadata enums and the tool will automatically adjust, and will handle v0 files, plus older and newer files that have differing numbers of parameters, as long as the parameters are only ever added to the end of the list.

I have been working on a patch for this today, which took a little longer than expected as it was a little more complex than expected (the printing routines in for the --dump command!).

I will post this tomorrow when tested - and if we agree it works it could be rolled into your set - it would make adding the PID parameter easier, and ensure that this new format is available for the upcoming developments.

Thanks for the info. I will look at your patch and see how to fit with it.

Thanks, Leo