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

[John Garry]

On 30/06/2023 21:16, Ian Rogers wrote:
> On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers<irogers@xxxxxxxxxx>  wrote:
> > 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.

Hi Ian,

> > If an event is specific to a particular PMU, shouldn't the metric name
> > the PMU with the event?

I am working on the basis - which I think is quite sane in case of sys 
events - that event names are unique to a PMU type.

> 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@",

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?

> > 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.

> So looking at the later patches, they are making it so the PMU doesn't
> need to be specified,

Right, as we assume that we use uniquely named events. Having non-unique 
event names seems to create problems.

> 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.

We treat these sys PMUs as standalone, and it makes no sense (currently) 
to have a metric which contains events for multiple PMU types as we 
don't know if the system is created with those PMUs, and, if it is, what 
topology etc.

> 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://urldefense.com/v3/__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__;Iw!!ACWV5N9M2RV99hQ!PE_9BEFVCr25fA9OHzfEDuT-MncA5pnPf5C3eTqYnXGKG9Q2OItrEIiEYz1T366HjAayQmYtZ6N6WxPJBCI$  

Thanks
John