diff mbox series

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

Message ID 20230628102949.2598096-5-john.g.garry@oracle.com (mailing list archive)
State New, archived
Headers show
Series perf tool: sys event metric support re-write | expand

Commit Message

John Garry June 28, 2023, 10:29 a.m. UTC
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.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 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(+)

Comments

Ian Rogers June 30, 2023, 7 p.m. UTC | #1
On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.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.

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@oracle.com>
> ---
>  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
>
Ian Rogers June 30, 2023, 8:16 p.m. UTC | #2
On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.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.
>
> 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.

So looking at the later patches, they are making it so the PMU doesn't
need to be specified, 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. 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://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

Thanks,
Ian

> Thanks,
> Ian
>
>
> > Signed-off-by: John Garry <john.g.garry@oracle.com>
> > ---
> >  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
> >
John Garry July 3, 2023, 3:15 p.m. UTC | #3
On 30/06/2023 21:16, Ian Rogers wrote:
> On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers<irogers@google.com>  wrote:
>> On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.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@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
Ian Rogers July 12, 2023, 6:05 a.m. UTC | #4
On Mon, Jul 3, 2023 at 8:15 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 30/06/2023 21:16, Ian Rogers wrote:
> > On Fri, Jun 30, 2023 at 12:00 PM Ian Rogers<irogers@google.com>  wrote:
> >> On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.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@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?

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
>
John Garry July 12, 2023, 10:55 a.m. UTC | #5
>>>> 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 
Ian Rogers July 12, 2023, 5:52 p.m. UTC | #6
On Wed, Jul 12, 2023 at 3:55 AM John Garry <john.g.garry@oracle.com> 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 
John Garry July 13, 2023, 3:06 p.m. UTC | #7
On 12/07/2023 18:52, Ian Rogers wrote:
>> 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 
Ian Rogers July 13, 2023, 9:35 p.m. UTC | #8
On Thu, Jul 13, 2023 at 8:07 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 12/07/2023 18:52, Ian Rogers wrote:
> >> 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 
John Garry July 14, 2023, 11:58 a.m. UTC | #9
On 13/07/2023 22:35, Ian Rogers wrote:
>>
>> {
>>                  "MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 2",
>>                  "MetricName": "metric_baz",
>> },
>> {
>>                  "EventCode": "0x84",
>>                  "EventName": "event_foo ",
>>                  "Compat": "0x00000040",
>>                  "Unit": "pmu_unit"
>> },
>> {
>>                  "EventCode": "0x85",
>>                  "EventName": "event_bar ",
>>                  "Compat": "0x00000040",
>>                  "Unit": "pmu_unit"
>> },
>>
>> And we have a pmu with name and hw id matching "pmu_unit" and
>> "0x00000040" present, that we select metric metric_baz for soc_b
> Not sure I'm fully understanding.With the sysfs layout we'd have to
> have a way of supporting CPUIDs, we could have a mapfile.csv style
> approach or perhaps encode the CPUID into the path. It is complex as
> CPUIDs are wildcards in the tool.

I am not sure why you mention CPUIDs. sys events and their metrics are 
matched only on Unit and Compat.

Furthermore, my solution here is based what we have today, and would not 
be based on this sysfs solution which you mention.

> 
>>> 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.
>> Do you have an RFC or something for this "sysfs style layout"? I think
>> that it would be easier for me to understand your idea by seeing the SW.
> When I get a chance 
Ian Rogers July 14, 2023, 3:55 p.m. UTC | #10
On Fri, Jul 14, 2023 at 4:58 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 13/07/2023 22:35, Ian Rogers wrote:
> >>
> >> {
> >>                  "MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 2",
> >>                  "MetricName": "metric_baz",
> >> },
> >> {
> >>                  "EventCode": "0x84",
> >>                  "EventName": "event_foo ",
> >>                  "Compat": "0x00000040",
> >>                  "Unit": "pmu_unit"
> >> },
> >> {
> >>                  "EventCode": "0x85",
> >>                  "EventName": "event_bar ",
> >>                  "Compat": "0x00000040",
> >>                  "Unit": "pmu_unit"
> >> },
> >>
> >> And we have a pmu with name and hw id matching "pmu_unit" and
> >> "0x00000040" present, that we select metric metric_baz for soc_b
> > Not sure I'm fully understanding.With the sysfs layout we'd have to
> > have a way of supporting CPUIDs, we could have a mapfile.csv style
> > approach or perhaps encode the CPUID into the path. It is complex as
> > CPUIDs are wildcards in the tool.
>
> I am not sure why you mention CPUIDs. sys events and their metrics are
> matched only on Unit and Compat.
>
> Furthermore, my solution here is based what we have today, and would not
> be based on this sysfs solution which you mention.
>
> >
> >>> 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.
> >> Do you have an RFC or something for this "sysfs style layout"? I think
> >> that it would be easier for me to understand your idea by seeing the SW.
> > When I get a chance 
John Garry July 17, 2023, 7:41 a.m. UTC | #11
On 14/07/2023 16:55, Ian Rogers wrote:
> In this
> series my main concern was in the changes of the event lookup and
> having implied PMUs. You mentioned doing these changes so I was
> waiting for a v2.

OK, fine, I can look to do this now.

BTW, which git repo/branch do you guys use for dev? I thought that it 
would be acme git, but Namhyung says "We moved to new repos from acme to 
perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?

Thanks,
John
Ian Rogers July 17, 2023, 9:39 p.m. UTC | #12
On Mon, Jul 17, 2023 at 12:41 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 14/07/2023 16:55, Ian Rogers wrote:
> > In this
> > series my main concern was in the changes of the event lookup and
> > having implied PMUs. You mentioned doing these changes so I was
> > waiting for a v2.
>
> OK, fine, I can look to do this now.
>
> BTW, which git repo/branch do you guys use for dev? I thought that it
> would be acme git, but Namhyung says "We moved to new repos from acme to
> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?

Current development is here now:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next

Thanks,
Ian

> Thanks,
> John
John Garry July 18, 2023, 9:32 a.m. UTC | #13
On 17/07/2023 22:39, Ian Rogers wrote:
> On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com>  wrote:
>> On 14/07/2023 16:55, Ian Rogers wrote:
>>> In this
>>> series my main concern was in the changes of the event lookup and
>>> having implied PMUs. You mentioned doing these changes so I was
>>> waiting for a v2.
>> OK, fine, I can look to do this now.

I was thinking about this a little further. So you suggest that the 
metric expression contains PMU name per term, like 
"cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work 
for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY? 
Would we use the "Unit" expression for the metric name, like 
"@hisi_sicl,cpa@event_foo"?

>>
>> BTW, which git repo/branch do you guys use for dev? I thought that it
>> would be acme git, but Namhyung says "We moved to new repos from acme to
>> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?
> Current development is here now:
> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$  

Can that be added to the MAINTAINERS file? I suppose it is ok under 
"PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed 
under that same entry would be pretty obvious in purpose.

Cheers,
John
Ian Rogers July 19, 2023, 3:25 p.m. UTC | #14
On Tue, Jul 18, 2023 at 2:32 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 17/07/2023 22:39, Ian Rogers wrote:
> > On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com>  wrote:
> >> On 14/07/2023 16:55, Ian Rogers wrote:
> >>> In this
> >>> series my main concern was in the changes of the event lookup and
> >>> having implied PMUs. You mentioned doing these changes so I was
> >>> waiting for a v2.
> >> OK, fine, I can look to do this now.
>
> I was thinking about this a little further. So you suggest that the
> metric expression contains PMU name per term, like
> "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work
> for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY?
> Would we use the "Unit" expression for the metric name, like
> "@hisi_sicl,cpa@event_foo"?

How does this work for events? The "@hisi_sicl,cpa@event_foo" looks
strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl
looks like an event name.

>
> >>
> >> BTW, which git repo/branch do you guys use for dev? I thought that it
> >> would be acme git, but Namhyung says "We moved to new repos from acme to
> >> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?
> > Current development is here now:
> > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$
>
> Can that be added to the MAINTAINERS file? I suppose it is ok under
> "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed
> under that same entry would be pretty obvious in purpose.

Arnaldo could you take a look at doing this?

Thanks,
Ian

> Cheers,
> John
>
John Garry July 19, 2023, 3:36 p.m. UTC | #15
On 19/07/2023 16:25, Ian Rogers wrote:
>> I was thinking about this a little further. So you suggest that the
>> metric expression contains PMU name per term, like
>> "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work
>> for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY?
>> Would we use the "Unit" expression for the metric name, like
>> "@hisi_sicl,cpa@event_foo"?
> How does this work for events? The "@hisi_sicl,cpa@event_foo" looks
> strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl
> looks like an event name.

Yeah, that was a typo from me - like you say, it would be 
hisi_sicl,cpa@event_foo@

So is that what you would be suggesting then, such that we specify the 
PMU in the metric terms? It does look a bit odd :)

> 
>>>> BTW, which git repo/branch do you guys use for dev? I thought that it
>>>> would be acme git, but Namhyung says "We moved to new repos from acme to
>>>> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?
>>> Current development is here now:
>>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$
>> Can that be added to the MAINTAINERS file? I suppose it is ok under
>> "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed
>> under that same entry would be pretty obvious in purpose.
> Arnaldo could you take a look at doing this?

Thanks,
John
Ian Rogers July 19, 2023, 3:49 p.m. UTC | #16
On Wed, Jul 19, 2023 at 8:37 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 19/07/2023 16:25, Ian Rogers wrote:
> >> I was thinking about this a little further. So you suggest that the
> >> metric expression contains PMU name per term, like
> >> "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work
> >> for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY?
> >> Would we use the "Unit" expression for the metric name, like
> >> "@hisi_sicl,cpa@event_foo"?
> > How does this work for events? The "@hisi_sicl,cpa@event_foo" looks
> > strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl
> > looks like an event name.
>
> Yeah, that was a typo from me - like you say, it would be
> hisi_sicl,cpa@event_foo@
>
> So is that what you would be suggesting then, such that we specify the
> PMU in the metric terms? It does look a bit odd :)

I guess there could be some kind of regular expression syntax:
(hisi_scl|cpa)@event_foo

We have the "has_event" function now in the metric expressions:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/expr.c?h=perf-tools-next#n480

So it could be:
hisi_scl@event_foo@ if has_event(hisi_scl@event_foo@) else cpa@event_foo@

Which has the advantage of working today, but the disadvantage of being verbose.

Thanks,
Ian


> >
> >>>> BTW, which git repo/branch do you guys use for dev? I thought that it
> >>>> would be acme git, but Namhyung says "We moved to new repos from acme to
> >>>> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?
> >>> Current development is here now:
> >>> https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$
> >> Can that be added to the MAINTAINERS file? I suppose it is ok under
> >> "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed
> >> under that same entry would be pretty obvious in purpose.
> > Arnaldo could you take a look at doing this?
>
> Thanks,
> John
Arnaldo Carvalho de Melo July 19, 2023, 8:07 p.m. UTC | #17
Em Wed, Jul 19, 2023 at 08:25:31AM -0700, Ian Rogers escreveu:
> On Tue, Jul 18, 2023 at 2:32 AM John Garry <john.g.garry@oracle.com> wrote:
> >
> > On 17/07/2023 22:39, Ian Rogers wrote:
> > > On Mon, Jul 17, 2023 at 12:41 AM John Garry<john.g.garry@oracle.com>  wrote:
> > >> On 14/07/2023 16:55, Ian Rogers wrote:
> > >>> In this
> > >>> series my main concern was in the changes of the event lookup and
> > >>> having implied PMUs. You mentioned doing these changes so I was
> > >>> waiting for a v2.
> > >> OK, fine, I can look to do this now.
> >
> > I was thinking about this a little further. So you suggest that the
> > metric expression contains PMU name per term, like
> > "cpu_atom@instructions@ / cpu_atom@cycles@" - how would/could this work
> > for PMUs with more complex naming, like the form hisi_siclXXX_cpa_YYY?
> > Would we use the "Unit" expression for the metric name, like
> > "@hisi_sicl,cpa@event_foo"?
> 
> How does this work for events? The "@hisi_sicl,cpa@event_foo" looks
> strange, shouldn't it be "hisi_sicl,cpa@event_foo@" but then hisi_sicl
> looks like an event name.
> 
> >
> > >>
> > >> BTW, which git repo/branch do you guys use for dev? I thought that it
> > >> would be acme git, but Namhyung says "We moved to new repos from acme to
> > >> perf/perf-tools and perf/perf-tools-next" - where is repo "perf"?
> > > Current development is here now:
> > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/log/?h=perf-tools-next__;!!ACWV5N9M2RV99hQ!OQDHOClSjd6nVZhmgzrK3RwzXuQpP54QhqyIKpITa_MFD4PLdS7yPYSnvInFja9nrFx9Sd-UnlsJ6XUqAh4$
> >
> > Can that be added to the MAINTAINERS file? I suppose it is ok under
> > "PERFORMANCE EVENTS SUBSYTEM", since the two would-be git repos listed
> > under that same entry would be pretty obvious in purpose.
> 
> Arnaldo could you take a look at doing this?

Sure, just added this:

[acme@quaco perf-tools-next]$ git show
commit 0146244875046fad472a39ffd61ec4f91719b62a (HEAD -> perf-tools-next)
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Wed Jul 19 16:53:01 2023 -0300

    MAINTAINERS: Add git information for perf-tools and perf-tools-next trees/branches

    Now the perf tools development is done on these trees/branches:

      git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git perf-tools
      git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git perf-tools-next

    For a while I'll continue mirroring what is these to the same branches
    in my git tree.

    Suggested-by: John Garry <john.g.garry@oracle.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Ian Rogers <irogers@google.com>
    Cc: Jiri Olsa <jolsa@kernel.org>
    Cc: Namhyung Kim <namhyung@kernel.org>
    Link: https://lore.kernel.org/lkml/CAP-5=fVGOP6-k=BTRd_bn=N0HVy+1ShpdW5rk5ND0ZGhm_fQkg@mail.gmail.com
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/MAINTAINERS b/MAINTAINERS
index aee340630ecaea38..e351cfc7cd41c570 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16629,6 +16629,8 @@ L:      linux-kernel@vger.kernel.org
 S:     Supported
 W:     https://perf.wiki.kernel.org/
 T:     git git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git perf/core
+T:     git git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools.git perf-tools
+T:     git git://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git perf-tools-next
 F:     arch/*/events/*
 F:     arch/*/events/*/*
 F:     arch/*/include/asm/perf_event.h
[acme@quaco perf-tools-next]$
diff mbox series

Patch

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,