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

[John Garry <john.g.garry@oracle.com>]

>>>> 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"?

  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.

Thanks,
John