linux-kernel - Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAP-5=fUs=u3PKYP3mVDdzNB8+=GAHaEXG3SGfWJpMELYYqO_hA@mail.gmail.com>
Date:   Fri, 30 Jun 2023 13:16:52 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     John Garry <john.g.garry@...cle.com>
Cc:     acme@...nel.org, namhyung@...nel.org, jolsa@...nel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
        renyu.zj@...ux.alibaba.com, shangxiaojing@...wei.com,
        zhangshaokun@...ilicon.com, qiangqing.zhang@....com,
        kjain@...ux.ibm.com, kan.liang@...ux.intel.com
Subject: Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()

On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers <irogers@...gle.com> wrote:
>
> On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@...cle.com> wrote:
> >
> > Add a function to get the events table associated with a metric table for
> > struct pmu_sys_events.
> >
> > We could also use something like:
> > struct pmu_sys_events *sys = container_of(metrics, struct pmu_sys_events,
> >                                                 metric_table);
> >
> > to lookup struct pmu_sys_events, but that relies on the user always passing
> > a sys events metric struct pointer, so this way is safer, but slower.
>
> If an event is specific to a particular PMU, shouldn't the metric name
> the PMU with the event? For example:
>
> MetricName: "IPC",
> MetricExpr: "instructions / cycles",
>
> Here instructions and cycles can wildcard match on BIG.little/hybrid
> systems and so we get an IPC metric for each PMU - although, I suspect
> this isn't currently quite working. We can also, and currently, do:
>
> MetricName: "IPC",
> MetricExpr: "cpu_atom@...tructions@ / cpu_atom@...les@",
> ...
> MetricName: "IPC",
> MetricExpr: "cpu_core@...tructions@ / cpu_core@...les@",
>
> The @ is used to avoid parsing confusion with / meaning divide. The
> PMUs for the events are explicitly listed here. We could say the PMU
> is implied but then it gets complex for uncore events, for metrics
> that mix core and uncore events.

So looking at the later patches, they are making it so the PMU doesn't
need to be specified, so I think it is the same issue as here. My
thought was that the PMU would always be required for metrics like
memory bandwidth per million instructions, ie >1 PMU. I know this
makes the metrics longer, I've tried to avoid writing json metrics and
have used Python to write them in my own work:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/metric.py?h=perf-tools-next#n411

Thanks,
Ian

> Thanks,
> Ian
>
>
> > Signed-off-by: John Garry <john.g.garry@...cle.com>
> > ---
> >  tools/perf/pmu-events/empty-pmu-events.c |  6 ++++++
> >  tools/perf/pmu-events/jevents.py         | 11 +++++++++++
> >  tools/perf/pmu-events/pmu-events.h       |  3 +++
> >  3 files changed, 20 insertions(+)
> >
> > diff --git a/tools/perf/pmu-events/empty-pmu-events.c b/tools/perf/pmu-events/empty-pmu-events.c
> > index a630c617e879..ae431b6bdf91 100644
> > --- a/tools/perf/pmu-events/empty-pmu-events.c
> > +++ b/tools/perf/pmu-events/empty-pmu-events.c
> > @@ -290,6 +290,12 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu
> >         return 0;
> >  }
> >
> > +const struct pmu_events_table *
> > +sys_events_find_events_table(__maybe_unused const struct pmu_metrics_table *metrics)
> > +{
> > +       return NULL;
> > +}
> > +
> >  const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
> >  {
> >         const struct pmu_events_table *table = NULL;
> > diff --git a/tools/perf/pmu-events/jevents.py b/tools/perf/pmu-events/jevents.py
> > index 80b569b8634b..947e8b1efa26 100755
> > --- a/tools/perf/pmu-events/jevents.py
> > +++ b/tools/perf/pmu-events/jevents.py
> > @@ -786,6 +786,17 @@ int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table,
> >          return 0;
> >  }
> >
> > +const struct pmu_events_table *
> > +sys_events_find_events_table(const struct pmu_metrics_table *metrics)
> > +{
> > +       for (const struct pmu_sys_events *tables = &pmu_sys_event_tables[0];
> > +             tables->name; tables++) {
> > +               if (&tables->metric_table == metrics)
> > +                       return &tables->event_table;
> > +       }
> > +       return NULL;
> > +}
> > +
> >  const struct pmu_events_table *perf_pmu__find_events_table(struct perf_pmu *pmu)
> >  {
> >          const struct pmu_events_table *table = NULL;
> > diff --git a/tools/perf/pmu-events/pmu-events.h b/tools/perf/pmu-events/pmu-events.h
> > index caf59f23cd64..a3642c08e39d 100644
> > --- a/tools/perf/pmu-events/pmu-events.h
> > +++ b/tools/perf/pmu-events/pmu-events.h
> > @@ -77,6 +77,9 @@ typedef int (*pmu_metric_iter_fn)(const struct pmu_metric *pm,
> >                                   const struct pmu_metrics_table *table,
> >                                   void *data);
> >
> > +const struct pmu_events_table *
> > +sys_events_find_events_table(const struct pmu_metrics_table *metrics);
> > +
> >  int pmu_events_table_for_each_event(const struct pmu_events_table *table, pmu_event_iter_fn fn,
> >                                     void *data);
> >  int pmu_metrics_table_for_each_metric(const struct pmu_metrics_table *table, pmu_metric_iter_fn fn,
> > --
> > 2.35.3
> >