Messages in this thread | | | Date | Mon, 23 Jan 2023 15:35:52 +0000 | Subject | Re: [PATCH v2 7/9] perf pmu-events: Introduce pmu_metrics_table | From | John Garry <> |
| |
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@google.com> > --- > 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; > }
| |