Message-ID: <CAP-5=fUu6xgVDQT4tq=vmRLDMe3ddMLywP11uOLvKSu8Lc6BjQ@mail.gmail.com>
Date:   Tue, 11 Jul 2023 23:05:22 -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,
        kjain@...ux.ibm.com, kan.liang@...ux.intel.com
Subject: Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()

On Mon, Jul 3, 2023 at 8:15 AM John Garry <john.g.garry@...cle.com> wrote:
>
> On 30/06/2023 21:16, Ian Rogers wrote:
> > 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.
>
> 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@...tructions@ / cpu_atom@...les@",
> >> ...
> >> MetricName: "IPC",
> >> MetricExpr: "cpu_core@...tructions@ / cpu_core@...les@",
>
> 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.

> >> 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://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

So we'd need to move these metrics to be on the appropriate uncore
PMU. Supporting >1 PMU 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.

Thanks,
Ian

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

Powered by blists - more mailing lists