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:

[...]

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel