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

Leo Yan
2021-01-13T02:28:36Z

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