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

John Garry posted 9 patches 2 years, 7 months ago
[PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 7 months ago
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(+)

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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 7 months ago
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
>
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 7 months ago
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
> >
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 7 months ago
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

Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 7 months ago
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
>
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 7 months ago
>>>> 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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 7 months ago
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 😄 ). 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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 7 months ago
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 😄 

Ok, fine. I need to check on implementing support for that.

Then would you have any idea for testing here?

What I do is to ensure that if we 2x tables like following for separate 
SoCs:

soc_a.json


{
		"MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 1",
		"MetricName": "metric_baz",
},
{
		"EventCode": "0x84",
		"EventName": "event_foo ",
		"Compat": "0x00000030",
		"Unit": "pmu_unit"
},
{
		"EventCode": "0x85",
		"EventName": "event_bar ",
		"Compat": "0x00000030",
		"Unit": "pmu_unit"
},



soc_b.json


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

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

Thanks,
John
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 7 months ago
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 😄
>
> Ok, fine. I need to check on implementing support for that.
>
> Then would you have any idea for testing here?
>
> What I do is to ensure that if we 2x tables like following for separate
> SoCs:
>
> soc_a.json
>
>
> {
>                 "MetricExpr": "pmu_unit@event_foo@ * pmu_unit@event_bar@ * 1",
>                 "MetricName": "metric_baz",
> },
> {
>                 "EventCode": "0x84",
>                 "EventName": "event_foo ",
>                 "Compat": "0x00000030",
>                 "Unit": "pmu_unit"
> },
> {
>                 "EventCode": "0x85",
>                 "EventName": "event_bar ",
>                 "Compat": "0x00000030",
>                 "Unit": "pmu_unit"
> },
>
>
>
> soc_b.json
>
>
> {
>                 "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 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 :-) My thought is to first extend jevents.py to
have a second output format, so sysfs style rather than pmu-events.c.
This way we can merge the changes as a jevents.py feature even if we
don't change the perf tool to support it.

Thanks,
Ian

> Thanks,
> John
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 6 months ago
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 😄 My thought is to first extend jevents.py to
> have a second output format, so sysfs style rather than pmu-events.c.
> This way we can merge the changes as a jevents.py feature even if we
> don't change the perf tool to support it.

ok, fine, but I would still like this progress this work now. It has 
been sitting around for some time and was really difficult to rebase 
(since I was not tumbling my baseline).

Thanks,
John
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 6 months ago
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 😄 My thought is to first extend jevents.py to
> > have a second output format, so sysfs style rather than pmu-events.c.
> > This way we can merge the changes as a jevents.py feature even if we
> > don't change the perf tool to support it.
>
> ok, fine, but I would still like this progress this work now. It has
> been sitting around for some time and was really difficult to rebase
> (since I was not tumbling my baseline).

Sure, the sysfs thing is a distraction until we have it. 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.

Thanks,
Ian

> Thanks,
> John
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 6 months ago
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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 6 months ago
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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 6 months ago
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

Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 6 months ago
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
>
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Arnaldo Carvalho de Melo 2 years, 6 months ago
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]$
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by John Garry 2 years, 6 months ago
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
Re: [PATCH RFC 4/9] perf jevents: Add sys_events_find_events_table()
Posted by Ian Rogers 2 years, 6 months ago
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