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

[Ian Rogers]

On Wed, Jul 12, 2023 at 3:55 AM John Garry <john.g.garry@xxxxxxxxxx> wrote:
>
>
> >>>> MetricExpr: "cpu_core@instructions@ / cpu_core@cycles@",
> >> I did not know that it was possible to state that an event is for a
> >> specific PMU type in this fashion - is this feature new? Does it work
> >> only for known terms, like cycles and instructions?
> > It has been in metrics a long time (I didn't choose that @ was the /
> > replacement 😄 ). It should work for all events.
> >
>
> Good to know.
>
> >>>> 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 this works ok for IPC and CPU PMUs as we want the same event for many
> >> PMU types and naturally it would have the same name.
> >>
> >> I am still not sure that sys event metrics need to specify a PMU.
> > There was a similar thought for hybrid metrics. The PMU could be
> > implied from the PMU of the metric. I think there can be confusion
> > from an implied PMU, for example the cycles event without a PMU will
> > open two events on a hybrid CPU. If we imply the PMU then it can mean
> > just 1 PMU, but if the PMU doesn't have the event presumably it means
> > the multiple PMU behavior.
> >
> > In parse-events there is existing logic to wildcard events but to
> > ignore those that don't match a given PMU. This is used to support the
> > --cputype option in builtin-stat.c, there is a similar option for
> > builtin-list.c. We can use this so that events in a metric only match
> > the PMU of the metric. Currently there are core metrics but whose
> > events are all uncore like:
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/pmu-events/arch/x86/alderlake/adl-metrics.json?h=perf-tools-next*n1802__;Iw!!ACWV5N9M2RV99hQ!MjhanGd4AVsAl6d8weFktNHgeOptrgeBDyooXlpeW-J1TQ0e2BwzvqO4BTFEjs_gRzuWTPfnhW_jLx1pIJc$
> >
> > So we'd need to move these metrics to be on the appropriate uncore
> > PMU. Supporting >1 PMU
>
> To be crystal clear, when you say ">1 PMU", do you mean ">1 PMU instance
> of the same type" or ">1 PMU type"?

So I'm meaning that if you have a MetricExpr where the events are from
>1 PMU, for example memory bandwidth coming from uncore PMUs and then
instructions from a core PMU, and you do something like a ratio
between these two.

>   in a metric wouldn't work though as it would
> > appear the event was missing. Having the metric specify the PMU avoids
> > these problems, but is verbose.
>
> The message I'm getting - correct me if I am wrong - is that you would
> still prefer the PMU specified per event in metric expr, right? We don't
> do that exactly for sys PMU metrics today - we specify "Unit" instead, like:
>
> MetricExpr: "sys_pmu_bar_event_foo1 + sys_pmu_bar_event_foo2"
> Compat: "baz"
> Unit:"sys_pmu_bar"
>
> And so you prefer something like the following, right?
> MetricExpr: "sys_pmu_foo@bar1@ + sys_pmu_foo@bar2@"
>
> If so, I think that is ok - I just want to get rid of Unit and Compat.

I think we're agreeing :-) I think Unit may be useful, say on Intel
hybrid I want the tma_fround_bound metric on just cpu_atom. Currently
the use of Unit is messy for metrics, ie uncore metrics are associated
with core PMUs, and what to do with a MetricExpr with >1 PMU. I think
we're learning from trying. I'm just hoping the migration to a sysfs
style layout will still be possible, as I can see lots of upside in
terms of testing, 1 approach, etc.

Thanks,
Ian

> Thanks,
> John