| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAP-5=fV0GBtx_0qHPemO_AsLyf_3XAxcC9Q2eG-=rMpuTsMmzg@mail.gmail.com>
Date: Mon, 23 Jan 2023 20:48:39 -0800
From: Ian Rogers <irogers@...gle.com>
To: John Garry <john.g.garry@...cle.com>
Cc: Will Deacon <will@...nel.org>, James Clark <james.clark@....com>,
Mike Leach <mike.leach@...aro.org>,
Leo Yan <leo.yan@...aro.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Adrian Hunter <adrian.hunter@...el.com>,
Kan Liang <kan.liang@...ux.intel.com>,
Kim Phillips <kim.phillips@....com>,
Florian Fischer <florian.fischer@...q.space>,
Ravi Bangoria <ravi.bangoria@....com>,
Xing Zhengjun <zhengjun.xing@...ux.intel.com>,
Rob Herring <robh@...nel.org>,
Kang Minchul <tegongkang@...il.com>,
linux-arm-kernel@...ts.infradead.org,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Sandipan Das <sandipan.das@....com>,
Jing Zhang <renyu.zj@...ux.alibaba.com>,
linuxppc-dev@...ts.ozlabs.org, Kajol Jain <kjain@...ux.ibm.com>,
Stephane Eranian <eranian@...gle.com>,
Perry Taylor <perry.taylor@...el.com>,
Caleb Biggers <caleb.biggers@...el.com>
Subject: Re: [PATCH v2 7/9] perf pmu-events: Introduce pmu_metrics_table
On Mon, Jan 23, 2023 at 7:36 AM John Garry <john.g.garry@...cle.com> wrote:
>
> 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@...gle.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.
v3 will rebase and fix.
> > }
> >
> > 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
The main reason to do it is to reduce the scope of pm to just be the
loop body. There's some discussion relating to this to do with the
move to C11:
https://lwn.net/Articles/885941/
> > ; 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.
Agreed. I think we should generate the empty case and then event if
there is just 1 iteration, we know that the code is that way because
of the auto-generation.
> > +
> > + 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?
Done.
> > + 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?
It was a trade-off with patch size. Will try to improve in v3.
Thanks,
Ian
> > +
> > 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;
> > }
>
>
>
Powered by blists - more mailing lists