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

[Leo Yan <leo.yan@linaro.org>]

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