Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()

[Ian Rogers]

On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@xxxxxxxxxx> 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@instructions@ / cpu_atom@cycles@",
...
MetricName: "IPC",
MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@",

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.

Thanks,
Ian


> Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> ---
>  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
>