Re: [PATCH v2 7/9] perf pmu-events: Introduce pmu_metrics_table
From: John Garry
Date: Mon Jan 23 2023 - 10:36:54 EST
On 21/12/2022 22:34, Ian Rogers wrote:
Add a metrics table that is just a cast from pmu_events_table. This
changes the APIs so that event and metric usage of the underlying
table is different. Later changes will separate the tables.
This introduction fixes a NO_JEVENTS=1 regression on:
68: Parse and process metrics : Ok
70: Event expansion for cgroups : Ok
caused by the necessary test metrics not being found.
I have just checked some of this code so far...
Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
---
tools/perf/arch/arm64/util/pmu.c | 23 ++++++++++-
tools/perf/pmu-events/empty-pmu-events.c | 52 ++++++++++++++++++++----
tools/perf/pmu-events/jevents.py | 24 ++++++++---
tools/perf/pmu-events/pmu-events.h | 10 +++--
tools/perf/tests/expand-cgroup.c | 4 +-
tools/perf/tests/parse-metric.c | 4 +-
tools/perf/tests/pmu-events.c | 5 ++-
tools/perf/util/metricgroup.c | 50 +++++++++++------------
tools/perf/util/metricgroup.h | 2 +-
tools/perf/util/pmu.c | 9 +++-
tools/perf/util/pmu.h | 1 +
11 files changed, 133 insertions(+), 51 deletions(-)
diff --git a/tools/perf/arch/arm64/util/pmu.c b/tools/perf/arch/arm64/util/pmu.c
index 477e513972a4..f8ae479a06db 100644
--- a/tools/perf/arch/arm64/util/pmu.c
+++ b/tools/perf/arch/arm64/util/pmu.c
@@ -19,7 +19,28 @@ const struct pmu_events_table *pmu_events_table__find(void)
if (pmu->cpus->nr != cpu__max_cpu().cpu)
return NULL;
- return perf_pmu__find_table(pmu);
+ return perf_pmu__find_events_table(pmu);
+ }
+
+ return NULL;
+}
+
+const struct pmu_metrics_table *pmu_metrics_table__find(void)
+{
+ struct perf_pmu *pmu = NULL;
+
+ while ((pmu = perf_pmu__scan(pmu))) {
+ if (!is_pmu_core(pmu->name))
+ continue;
+
+ /*
+ * The cpumap should cover all CPUs. Otherwise, some CPUs may
+ * not support some events or have different event IDs.
+ */
+ if (pmu->cpus->nr != cpu__max_cpu().cpu)
+ return NULL;
+
+ return perf_pmu__find_metrics_table(pmu);
I think that this code will be conflicting with the recent arm64 metric
support. And now it seems even more scope for factoring out code.
}
return NULL;
diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c
index 5572a4d1eddb..d50f60a571dd 100644
--- a/tools/perf/pmu-events/empty-pmu-events.c
+++ b/tools/perf/pmu-events/empty-pmu-events.c
@@ -278,14 +278,12 @@ int pmu_events_table_for_each_event(const struct pmu_events_table *table, pmu_ev
return 0;
}
-int pmu_events_table_for_each_metric(const struct pmu_events_table *etable, pmu_metric_iter_fn fn,
- void *data)
+int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu_metric_iter_fn fn,
+ void *data)
{
- struct pmu_metrics_table *table = (struct pmu_metrics_table *)etable;
-
for (const struct pmu_metric *pm = &table->entries[0]
nit on coding style: do we normally declare local variables like this?
It condenses the code but makes a bit less readable, IMHO
; pm->metric_group || pm->metric_name;
pm++) {
- int ret = fn(pm, etable, data);
+ int ret = fn(pm, table, data);
if (ret)
return ret;
@@ -293,7 +291,7 @@ int pmu_events_table_for_each_metric(const struct pmu_events_table *etable, pmu_
return 0;
}
-const struct pmu_events_table *perf_pmu__find_table(struct perf_pmu *pmu)
+const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
{
const struct pmu_events_table *table = NULL;
char *cpuid = perf_pmu__getcpuid(pmu);
@@ -321,6 +319,34 @@ const struct pmu_events_table *perf_pmu__find_table(struct perf_pmu *pmu)
return table;
}
+const struct pmu_metrics_table *perf_pmu__find_metrics_table(struct perf_pmu *pmu)
+{
+ const struct pmu_metrics_table *table = NULL;
+ char *cpuid = perf_pmu__getcpuid(pmu);
+ int i;
+
+ /* on some platforms which uses cpus map, cpuid can be NULL for
+ * PMUs other than CORE PMUs.
+ */
+ if (!cpuid)
+ return NULL;
+
+ i = 0;
+ for (;;) {
+ const struct pmu_events_map *map = &pmu_events_map[i++];
To me, this is all strange code. Again this is a comment on the current
code: Consider pmu_for_each_sys_event() as an example, we have a while
loop for each member of pmu_sys_event_tables[]. But pmu_sys_event_tables
is hardcoded for a single member, so why loop? It seems the same for all
these "for each" helper in the "empty" events c file.
+
+ if (!map->cpuid)
+ break;
+
+ if (!strcmp_cpuid_str(map->cpuid, cpuid)) {
+ table = &map->metric_table;
+ break;
+ }
+ }
+ free(cpuid);
+ return table;
+}
+
const struct pmu_events_table *find_core_events_table(const char *arch, const char *cpuid)
{
for (const struct pmu_events_map *tables = &pmu_events_map[0];
@@ -332,6 +358,17 @@ const struct pmu_events_table *find_core_events_table(const char *arch, const ch
return NULL;
}
+const struct pmu_metrics_table *find_core_metrics_table(const char *arch, const char *cpuid)
+{
+ for (const struct pmu_events_map *tables = &pmu_events_map[0];
+ tables->arch;
+ tables++) {
combine with previous line?
+ if (!strcmp(tables->arch, arch) && !strcmp_cpuid_str(tables->cpuid, cpuid))
+ return &tables->metric_table;
+ }
+ return NULL;
+}
+
int pmu_for_each_core_event(pmu_event_iter_fn fn, void *data)
{
for (const struct pmu_events_map *tables = &pmu_events_map[0];
@@ -350,8 +387,7 @@ int pmu_for_each_core_metric(pmu_metric_iter_fn fn, void *data)
for (const struct pmu_events_map *tables = &pmu_events_map[0];
tables->arch;
tables++) {
- int ret = pmu_events_table_for_each_metric(
- (const struct pmu_events_table *)&tables->metric_table, fn, data);
+ int ret = pmu_metrics_table_for_each_metric(&tables->metric_table, fn, data);
if (ret)
return ret;
diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
index 7b9714b25d0a..be2cf8a8779c 100755
--- a/tools/perf/pmu-events/jevents.py
+++ b/tools/perf/pmu-events/jevents.py
@@ -609,17 +609,19 @@ int pmu_events_table_for_each_event(const struct pmu_events_table *table,
return 0;
}
-int pmu_events_table_for_each_metric(const struct pmu_events_table *table,
+int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *mtable,
pmu_metric_iter_fn fn,
void *data)
{
+ struct pmu_events_table *table = (struct pmu_events_table *)mtable;
As I may have hinted before, can we avoid casts like this, even if
transient?
+
for (size_t i = 0; i < table->length; i++) {
struct pmu_metric pm;
int ret;
decompress_metric(table->entries[i].offset, &pm);
if (pm.metric_name) {
- ret = fn(&pm, table, data);
+ ret = fn(&pm, mtable, data);
if (ret)
return ret;
}