[PATCH RFC 0/9] perf tool: sys event metric support re-write

[PATCH RFC 0/9] perf tool: sys event metric support re-write

[John Garry]

The current sys event metric support has some issues, like:
- It is broken that we only match a metric based on PMU compat, but not
  Unit as well, as reported by Jing Zhang <renyu.zj@linux.alibaba.com>
- No real self-test support
- Not able to use resolvable metrics
- Need to specify event PMU Unit and Compat for metric, which should not
  be necessary

This series changes sys event metric support to match metrics based on
evaluating each term in the metric expression and then ensuring it
matches an event from the same associated pmu_sys_events table.

Why an RFC?
- Even though main motivation here was to improve self-test support, that
  has proved quite tricky and nothing has been added yet.
  My desire is to test the feature that we match metrics for a specific
  SoC when PMUs with matching HW identifier are present. So I would hope
  to add sys metrics for many SoCs in ../pmu-events/arch/test/
- I still need to suppress logs from metricgroup_sys_metric_supported()
  indirect calls to functions like parse_events_multi_pmu_add(),
  generating logs like
  "smmuv3_pmcg.wr_sent_sp -> smmuv3_pmcg_50/event=0x86/" - we should only
  see those logs for when really adding the metric in calling add_metric()

Based on 82fe2e45cdb0 (acme/tmp.perf/core, acme/tmp.perf-tools-next, acme/perf/core, acme/perf-tools-next) perf pmus: Check if we can encode the PMU number in perf_event_attr.type

John Garry (9):
  perf metrics: Delete metricgroup_add_iter_data.table
  perf metrics: Don't iter sys metrics if we already found a CPU match
  perf metrics: Pass cpu and sys tables to metricgroup__add_metric()
  perf jevents: Add sys_events_find_events_table()
  perf pmu: Refactor pmu_add_sys_aliases_iter_fn()
  perf metrics: Add metricgroup_sys_metric_supported()
  perf metrics: Test metric match in metricgroup__sys_event_iter()
  perf metrics: Stop metricgroup__add_metric_sys_event_iter if already
    matched
  perf vendor events arm64: Remove unnecessary metric Unit and Compat
    specifiers

 .../arm64/freescale/imx8mm/sys/metrics.json   |   4 -
 .../arm64/freescale/imx8mn/sys/metrics.json   |   4 -
 .../arm64/freescale/imx8mq/sys/metrics.json   |   4 -
 .../arm64/hisilicon/hip09/sys/uncore-cpa.json |   4 -
 tools/perf/pmu-events/empty-pmu-events.c      |   6 +
 tools/perf/pmu-events/jevents.py              |  11 ++
 tools/perf/pmu-events/pmu-events.h            |   3 +
 tools/perf/tests/expand-cgroup.c              |   2 +-
 tools/perf/tests/parse-metric.c               |   2 +-
 tools/perf/tests/pmu-events.c                 |  29 ++-
 tools/perf/util/metricgroup.c                 | 182 +++++++++++++++---
 tools/perf/util/metricgroup.h                 |   3 +-
 tools/perf/util/pmu.c                         |  20 +-
 tools/perf/util/pmu.h                         |   2 +
 14 files changed, 220 insertions(+), 56 deletions(-)

-- 
2.35.3

[PATCH RFC 1/9] perf metrics: Delete metricgroup_add_iter_data.table

[John Garry]

Member metricgroup_add_iter_data.table is only used in
metricgroup__add_metric_sys_event_iter() as the @table arg to the
add_metric() call there.

However we only use the @table arg in add_metric() for resolving metrics,
which is currently not relevant to sys event metrics. As such, don't
bother passing this @table arg and use iter table instead, which is more
sane.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/metricgroup.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index a6a5ed44a679..4389ccd29fe7 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -798,7 +798,6 @@ struct metricgroup_add_iter_data {
 	bool system_wide;
 	struct metric *root_metric;
 	const struct visited_metric *visited;
-	const struct pmu_metrics_table *table;
 };
 
 static bool metricgroup__find_metric(const char *pmu,
@@ -1112,7 +1111,7 @@ static int add_metric(struct list_head *metric_list,
 }
 
 static int metricgroup__add_metric_sys_event_iter(const struct pmu_metric *pm,
-					const struct pmu_metrics_table *table __maybe_unused,
+					const struct pmu_metrics_table *table,
 					void *data)
 {
 	struct metricgroup_add_iter_data *d = data;
@@ -1123,7 +1122,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_metric *pm,
 
 	ret = add_metric(d->metric_list, pm, d->modifier, d->metric_no_group,
 			 d->metric_no_threshold, d->user_requested_cpu_list,
-			 d->system_wide, d->root_metric, d->visited, d->table);
+			 d->system_wide, d->root_metric, d->visited, table);
 	if (ret)
 		goto out;
 
@@ -1275,7 +1274,6 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 				.system_wide = system_wide,
 				.has_match = &has_match,
 				.ret = &ret,
-				.table = table,
 			},
 		};
 
-- 
2.35.3

Re: [PATCH RFC 1/9] perf metrics: Delete metricgroup_add_iter_data.table

[Ian Rogers]

On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote:
>
> Member metricgroup_add_iter_data.table is only used in
> metricgroup__add_metric_sys_event_iter() as the @table arg to the
> add_metric() call there.
>
> However we only use the @table arg in add_metric() for resolving metrics,
> which is currently not relevant to sys event metrics. As such, don't
> bother passing this @table arg and use iter table instead, which is more
> sane.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Acked-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/util/metricgroup.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index a6a5ed44a679..4389ccd29fe7 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -798,7 +798,6 @@ struct metricgroup_add_iter_data {
>         bool system_wide;
>         struct metric *root_metric;
>         const struct visited_metric *visited;
> -       const struct pmu_metrics_table *table;
>  };
>
>  static bool metricgroup__find_metric(const char *pmu,
> @@ -1112,7 +1111,7 @@ static int add_metric(struct list_head *metric_list,
>  }
>
>  static int metricgroup__add_metric_sys_event_iter(const struct pmu_metric *pm,
> -                                       const struct pmu_metrics_table *table __maybe_unused,
> +                                       const struct pmu_metrics_table *table,
>                                         void *data)
>  {
>         struct metricgroup_add_iter_data *d = data;
> @@ -1123,7 +1122,7 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_metric *pm,
>
>         ret = add_metric(d->metric_list, pm, d->modifier, d->metric_no_group,
>                          d->metric_no_threshold, d->user_requested_cpu_list,
> -                        d->system_wide, d->root_metric, d->visited, d->table);
> +                        d->system_wide, d->root_metric, d->visited, table);
>         if (ret)
>                 goto out;
>
> @@ -1275,7 +1274,6 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>                                 .system_wide = system_wide,
>                                 .has_match = &has_match,
>                                 .ret = &ret,
> -                               .table = table,
>                         },
>                 };
>
> --
> 2.35.3
>

[PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match

[John Garry]

In metricgroup__add_metric() we still iter the sys metrics if we already
found a match from the CPU table, which is pretty pointless, so don't
bother.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/metricgroup.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 4389ccd29fe7..8d2ac2513530 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1261,6 +1261,12 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 
 		has_match = data.has_match;
 	}
+
+	if (has_match) {
+		ret = 0;
+		goto out;
+	}
+
 	{
 		struct metricgroup_iter_data data = {
 			.fn = metricgroup__add_metric_sys_event_iter,
@@ -1279,6 +1285,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 
 		pmu_for_each_sys_metric(metricgroup__sys_event_iter, &data);
 	}
+
 	/* End of pmu events. */
 	if (!has_match)
 		ret = -EINVAL;
-- 
2.35.3

Re: [PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match

[Ian Rogers]

On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote:
>
> In metricgroup__add_metric() we still iter the sys metrics if we already
> found a match from the CPU table, which is pretty pointless, so don't
> bother.
>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  tools/perf/util/metricgroup.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 4389ccd29fe7..8d2ac2513530 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1261,6 +1261,12 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>
>                 has_match = data.has_match;
>         }
> +
> +       if (has_match) {
> +               ret = 0;
> +               goto out;
> +       }
> +

I think this can just be:

if (!has_match)

However, I'm not sure I agree with the intent of the change. We may
have a metric like IPC and want it to apply to all types of CPU, GPU,
etc. If we short-cut here then that won't be possible.

Thanks,
Ian
>         {
>                 struct metricgroup_iter_data data = {
>                         .fn = metricgroup__add_metric_sys_event_iter,
> @@ -1279,6 +1285,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>
>                 pmu_for_each_sys_metric(metricgroup__sys_event_iter, &data);
>         }
> +
>         /* End of pmu events. */
>         if (!has_match)
>                 ret = -EINVAL;
> --
> 2.35.3
>

Re: [PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match

[John Garry]

On 30/06/2023 18:41, Ian Rogers wrote:
> On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.com>  wrote:
>> In metricgroup__add_metric() we still iter the sys metrics if we already
>> found a match from the CPU table, which is pretty pointless, so don't
>> bother.
>>
>> Signed-off-by: John Garry<john.g.garry@oracle.com>
>> ---
>>   tools/perf/util/metricgroup.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 4389ccd29fe7..8d2ac2513530 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -1261,6 +1261,12 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>>
>>                  has_match = data.has_match;
>>          }

Hi Ian,

>> +
>> +       if (has_match) {
>> +               ret = 0;
>> +               goto out;
>> +       }
>> +
> I think this can just be:
> 
> if (!has_match)

But ret has no initial value

> 
> However, I'm not sure I agree with the intent of the change. We may
> have a metric like IPC and want it to apply to all types of CPU, GPU,
> etc. If we short-cut here then that won't be possible.

A few points to make on this:
- Currently we don't have any same-named metrics like this, so not much 
use in supporting it in the code (yet).
- Even if we had some same-named metrics, I am not sure if it even works 
properly. Do we have any uncore PMU metrics which have same name as CPU 
metrics?
- Further to the previous point, do we really want same-named metrics 
for different PMUs in the future? I think event / metric names need to 
be chosen carefully to avoid clash for other PMUs or keywords. For your 
example, if I did ask for IPC metric, I'd like to be able to just know 
I'm getting IPC metric for CPUs or some other PMUs, but not both.

Thanks,
John

> 

Re: [PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match

[Ian Rogers]

On Mon, Jul 3, 2023 at 6:09 AM John Garry <john.g.garry@oracle.com> wrote:
>
> On 30/06/2023 18:41, Ian Rogers wrote:
> > On Wed, Jun 28, 2023 at 3:30 AM John Garry<john.g.garry@oracle.com>  wrote:
> >> In metricgroup__add_metric() we still iter the sys metrics if we already
> >> found a match from the CPU table, which is pretty pointless, so don't
> >> bother.
> >>
> >> Signed-off-by: John Garry<john.g.garry@oracle.com>
> >> ---
> >>   tools/perf/util/metricgroup.c | 7 +++++++
> >>   1 file changed, 7 insertions(+)
> >>
> >> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> >> index 4389ccd29fe7..8d2ac2513530 100644
> >> --- a/tools/perf/util/metricgroup.c
> >> +++ b/tools/perf/util/metricgroup.c
> >> @@ -1261,6 +1261,12 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
> >>
> >>                  has_match = data.has_match;
> >>          }
>
> Hi Ian,
>
> >> +
> >> +       if (has_match) {
> >> +               ret = 0;
> >> +               goto out;
> >> +       }
> >> +
> > I think this can just be:
> >
> > if (!has_match)
>
> But ret has no initial value
>
> >
> > However, I'm not sure I agree with the intent of the change. We may
> > have a metric like IPC and want it to apply to all types of CPU, GPU,
> > etc. If we short-cut here then that won't be possible.
>
> A few points to make on this:
> - Currently we don't have any same-named metrics like this, so not much
> use in supporting it in the code (yet).

We have same named metrics for heterogeneous CPU PMUs:
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#n304
cpu_atom
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#n1125
cpu_core

> - Even if we had some same-named metrics, I am not sure if it even works
> properly. Do we have any uncore PMU metrics which have same name as CPU
> metrics?

So I was thinking IPC was a generic concept that would apply to a
co-processor on a network card, a GPU, etc.

> - Further to the previous point, do we really want same-named metrics
> for different PMUs in the future? I think event / metric names need to
> be chosen carefully to avoid clash for other PMUs or keywords. For your
> example, if I did ask for IPC metric, I'd like to be able to just know
> I'm getting IPC metric for CPUs or some other PMUs, but not both.

At the moment if you request an event without a PMU, say instructions
retired, we will attempt to open the event on every PMU - legacy
events (PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE) only try the core
PMUs. It would seem consistent if metrics tried to open on every PMU
like most events.

Thanks,
Ian

> Thanks,
> John
>
> >

Re: [PATCH RFC 2/9] perf metrics: Don't iter sys metrics if we already found a CPU match

[John Garry]

On 12/07/2023 06:40, Ian Rogers wrote:
>> A few points to make on this:
>> - Currently we don't have any same-named metrics like this, so not much
>> use in supporting it in the code (yet).
> We have same named metrics for heterogeneous CPU PMUs:
> 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*n304__;Iw!!ACWV5N9M2RV99hQ!MyvM7oyC6FgOVgDn2-Ot_TJNh4TF_VM9SlIVwv2AOTkJGdmDJ2NYf5WXh-yLcG1dRxLKdXWZVTzsoOo5yDk$  
> cpu_atom
> 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*n1125__;Iw!!ACWV5N9M2RV99hQ!MyvM7oyC6FgOVgDn2-Ot_TJNh4TF_VM9SlIVwv2AOTkJGdmDJ2NYf5WXh-yLcG1dRxLKdXWZVTzsL1dVM3Q$  
> cpu_core
> 

I meant that we have no same-named events for sys PMUs compared to 
uncore/CPU PMUs.

>> - Even if we had some same-named metrics, I am not sure if it even works
>> properly. Do we have any uncore PMU metrics which have same name as CPU
>> metrics?
> So I was thinking IPC was a generic concept that would apply to a
> co-processor on a network card, a GPU, etc.
> 
>> - Further to the previous point, do we really want same-named metrics
>> for different PMUs in the future? I think event / metric names need to
>> be chosen carefully to avoid clash for other PMUs or keywords. For your
>> example, if I did ask for IPC metric, I'd like to be able to just know
>> I'm getting IPC metric for CPUs or some other PMUs, but not both.
> At the moment if you request an event without a PMU, say instructions
> retired, we will attempt to open the event on every PMU - legacy
> events (PERF_TYPE_HARDWARE, PERF_TYPE_HW_CACHE) only try the core
> PMUs. It would seem consistent if metrics tried to open on every PMU
> like most events.

OK, fine. I can drop this change if you prefer. But, to reiterate my 
main point, I still think that there is not much point in looking for 
metrics which currently would not exist.

Thanks,
John

[PATCH RFC 3/9] perf metrics: Pass cpu and sys tables to metricgroup__add_metric()

[John Garry]

The current @table arg may be a CPU metric table or a sys metric table.
There is no point in searching the CPU metric table for a sys metric, and
vice versa, so pass separate pointers

When sys metric table is passed, this would mean that we are in self-test
mode. In this mode, the host system cannot match events in the metric
expression as the associated PMUs may not be present. As such, just try
add the metric, see metricgroup__add_metric_sys_event_iter().

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/tests/expand-cgroup.c |  2 +-
 tools/perf/tests/parse-metric.c  |  2 +-
 tools/perf/tests/pmu-events.c    | 29 +++++++++++++++-----
 tools/perf/util/metricgroup.c    | 45 +++++++++++++++++++++++---------
 tools/perf/util/metricgroup.h    |  3 ++-
 5 files changed, 59 insertions(+), 22 deletions(-)

diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
index 9c1a1f18db75..50e128ddb474 100644
--- a/tools/perf/tests/expand-cgroup.c
+++ b/tools/perf/tests/expand-cgroup.c
@@ -187,7 +187,7 @@ static int expand_metric_events(void)
 
 	rblist__init(&metric_events);
 	pme_test = find_core_metrics_table("testarch", "testcpu");
-	ret = metricgroup__parse_groups_test(evlist, pme_test, metric_str, &metric_events);
+	ret = metricgroup__parse_groups_test(evlist, pme_test, NULL, metric_str, &metric_events);
 	if (ret < 0) {
 		pr_debug("failed to parse '%s' metric\n", metric_str);
 		goto out;
diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
index 2c28fb50dc24..e146f1193294 100644
--- a/tools/perf/tests/parse-metric.c
+++ b/tools/perf/tests/parse-metric.c
@@ -95,7 +95,7 @@ static int __compute_metric(const char *name, struct value *vals,
 
 	/* Parse the metric into metric_events list. */
 	pme_test = find_core_metrics_table("testarch", "testcpu");
-	err = metricgroup__parse_groups_test(evlist, pme_test, name,
+	err = metricgroup__parse_groups_test(evlist, pme_test, NULL, name,
 					     &metric_events);
 	if (err)
 		goto out;
diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
index 64383fc34ef1..de571fd11cd7 100644
--- a/tools/perf/tests/pmu-events.c
+++ b/tools/perf/tests/pmu-events.c
@@ -798,9 +798,9 @@ struct metric {
 	struct metric_ref metric_ref;
 };
 
-static int test__parsing_callback(const struct pmu_metric *pm,
+static int _test__parsing_callback(const struct pmu_metric *pm,
 				  const struct pmu_metrics_table *table,
-				  void *data)
+				  void *data, bool is_cpu_table)
 {
 	int *failures = data;
 	int k;
@@ -811,6 +811,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
 		.nr_entries = 0,
 	};
 	int err = 0;
+	const struct pmu_metrics_table *cpu_table = is_cpu_table ? table : NULL;
+	const struct pmu_metrics_table *sys_table = is_cpu_table ? NULL : table;
 
 	if (!pm->metric_expr)
 		return 0;
@@ -834,7 +836,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
 
 	perf_evlist__set_maps(&evlist->core, cpus, NULL);
 
-	err = metricgroup__parse_groups_test(evlist, table, pm->metric_name, &metric_events);
+	err = metricgroup__parse_groups_test(evlist, cpu_table, sys_table,
+					     pm->metric_name, &metric_events);
 	if (err) {
 		if (!strcmp(pm->metric_name, "M1") || !strcmp(pm->metric_name, "M2") ||
 		    !strcmp(pm->metric_name, "M3")) {
@@ -890,13 +893,27 @@ static int test__parsing_callback(const struct pmu_metric *pm,
 	return err;
 }
 
-static int test__parsing(struct test_suite *test __maybe_unused,
+static int test__parsing_callback_cpu(const struct pmu_metric *pm,
+				  const struct pmu_metrics_table *table,
+				  void *data)
+{
+	return _test__parsing_callback(pm, table, data, true);
+}
+
+static int test__parsing_callback_sys(const struct pmu_metric *pm,
+				  const struct pmu_metrics_table *table,
+				  void *data)
+{
+	return _test__parsing_callback(pm, table, data, false);
+}
+
+static __maybe_unused int test__parsing(struct test_suite *test __maybe_unused,
 			 int subtest __maybe_unused)
 {
 	int failures = 0;
 
-	pmu_for_each_core_metric(test__parsing_callback, &failures);
-	pmu_for_each_sys_metric(test__parsing_callback, &failures);
+	pmu_for_each_core_metric(test__parsing_callback_cpu, &failures);
+	pmu_for_each_sys_metric(test__parsing_callback_sys, &failures);
 
 	return failures == 0 ? TEST_OK : TEST_FAIL;
 }
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 8d2ac2513530..520436fbe99d 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1232,13 +1232,14 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 				   const char *user_requested_cpu_list,
 				   bool system_wide,
 				   struct list_head *metric_list,
-				   const struct pmu_metrics_table *table)
+				   const struct pmu_metrics_table *cpu_table,
+				   const struct pmu_metrics_table *sys_table)
 {
 	LIST_HEAD(list);
 	int ret;
 	bool has_match = false;
 
-	{
+	if (cpu_table) {
 		struct metricgroup__add_metric_data data = {
 			.list = &list,
 			.pmu = pmu,
@@ -1254,7 +1255,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 		 * Iterate over all metrics seeing if metric matches either the
 		 * name or group. When it does add the metric to the list.
 		 */
-		ret = pmu_metrics_table_for_each_metric(table, metricgroup__add_metric_callback,
+		ret = pmu_metrics_table_for_each_metric(cpu_table, metricgroup__add_metric_callback,
 						       &data);
 		if (ret)
 			goto out;
@@ -1267,7 +1268,21 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 		goto out;
 	}
 
-	{
+	if (sys_table) {
+		struct metricgroup_add_iter_data data = {
+			.metric_list = &list,
+			.pmu = pmu,
+			.metric_name = metric_name,
+			.modifier = modifier,
+			.metric_no_group = metric_no_group,
+			.user_requested_cpu_list = user_requested_cpu_list,
+			.system_wide = system_wide,
+			.has_match = &has_match,
+			.ret = &ret,
+		};
+		pmu_metrics_table_for_each_metric(sys_table,
+			metricgroup__add_metric_sys_event_iter, &data);
+	} else {
 		struct metricgroup_iter_data data = {
 			.fn = metricgroup__add_metric_sys_event_iter,
 			.data = (void *) &(struct metricgroup_add_iter_data) {
@@ -1320,7 +1335,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
 					bool metric_no_threshold,
 					const char *user_requested_cpu_list,
 					bool system_wide, struct list_head *metric_list,
-					const struct pmu_metrics_table *table)
+					const struct pmu_metrics_table *cpu_table,
+					const struct pmu_metrics_table *sys_table)
 {
 	char *list_itr, *list_copy, *metric_name, *modifier;
 	int ret, count = 0;
@@ -1338,7 +1354,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
 		ret = metricgroup__add_metric(pmu, metric_name, modifier,
 					      metric_no_group, metric_no_threshold,
 					      user_requested_cpu_list,
-					      system_wide, metric_list, table);
+					      system_wide, metric_list, cpu_table,
+					      sys_table);
 		if (ret == -EINVAL)
 			pr_err("Cannot find metric or group `%s'\n", metric_name);
 
@@ -1534,7 +1551,8 @@ static int parse_groups(struct evlist *perf_evlist,
 			bool system_wide,
 			struct perf_pmu *fake_pmu,
 			struct rblist *metric_events_list,
-			const struct pmu_metrics_table *table)
+			const struct pmu_metrics_table *cpu_table,
+			const struct pmu_metrics_table *sys_table)
 {
 	struct evlist *combined_evlist = NULL;
 	LIST_HEAD(metric_list);
@@ -1547,7 +1565,7 @@ static int parse_groups(struct evlist *perf_evlist,
 		metricgroup__rblist_init(metric_events_list);
 	ret = metricgroup__add_metric_list(pmu, str, metric_no_group, metric_no_threshold,
 					   user_requested_cpu_list,
-					   system_wide, &metric_list, table);
+					   system_wide, &metric_list, cpu_table, sys_table);
 	if (ret)
 		goto out;
 
@@ -1697,18 +1715,19 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
 			      bool system_wide,
 			      struct rblist *metric_events)
 {
-	const struct pmu_metrics_table *table = pmu_metrics_table__find();
+	const struct pmu_metrics_table *cpu_table = pmu_metrics_table__find();
 
-	if (!table)
+	if (!cpu_table)
 		return -EINVAL;
 
 	return parse_groups(perf_evlist, pmu, str, metric_no_group, metric_no_merge,
 			    metric_no_threshold, user_requested_cpu_list, system_wide,
-			    /*fake_pmu=*/NULL, metric_events, table);
+			    /*fake_pmu=*/NULL, metric_events, cpu_table, NULL);
 }
 
 int metricgroup__parse_groups_test(struct evlist *evlist,
-				   const struct pmu_metrics_table *table,
+				   const struct pmu_metrics_table *cpu_table,
+				   const struct pmu_metrics_table *sys_table,
 				   const char *str,
 				   struct rblist *metric_events)
 {
@@ -1718,7 +1737,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
 			    /*metric_no_threshold=*/false,
 			    /*user_requested_cpu_list=*/NULL,
 			    /*system_wide=*/false,
-			    &perf_pmu__fake, metric_events, table);
+			    &perf_pmu__fake, metric_events, cpu_table, sys_table);
 }
 
 struct metricgroup__has_metric_data {
diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
index d5325c6ec8e1..b5f0d598eaec 100644
--- a/tools/perf/util/metricgroup.h
+++ b/tools/perf/util/metricgroup.h
@@ -79,7 +79,8 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
 			      bool system_wide,
 			      struct rblist *metric_events);
 int metricgroup__parse_groups_test(struct evlist *evlist,
-				   const struct pmu_metrics_table *table,
+				   const struct pmu_metrics_table *cpu_table,
+				   const struct pmu_metrics_table *sys_table,
 				   const char *str,
 				   struct rblist *metric_events);
 
-- 
2.35.3

Re: [PATCH RFC 3/9] perf metrics: Pass cpu and sys tables to metricgroup__add_metric()

[Ian Rogers]

On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote:
>
> The current @table arg may be a CPU metric table or a sys metric table.
> There is no point in searching the CPU metric table for a sys metric, and
> vice versa, so pass separate pointers
>
> When sys metric table is passed, this would mean that we are in self-test
> mode. In this mode, the host system cannot match events in the metric
> expression as the associated PMUs may not be present. As such, just try
> add the metric, see metricgroup__add_metric_sys_event_iter().

Thanks John, I'm not opposed to this change. My understanding is it
will give greater testing coverage. As previously mentioned I'd like
longer term we have a sysfs like abstraction for the json events. For
CPUs this could be like:

<cpuid>/cpu/events/inst_retired.any
<cpuid>/cpu/events/inst_retired.any.scale
<cpuid>/cpu/events/inst_retired.any.unit
<cpuid>/cpu/events/inst_retired.any.desc
...
<cpuid>/cpu/metrics/ipc
<cpuid>/cpu/metrics/ipc.scale
<cpuid>/cpu/metrics/ipc.unit
...
<cpuid>/uncore_imc_free_running_0/events/unc_mc0_rdcas_count_freerun
...

Where <cpuid> comes from mapfile.csv. I'd like to union the in memory
json event generated sysfs, with the kernel sysfs. There needs to be
some kind of wildcard mechanism for all the uncore counters. Such a
union-ing could allow on an disk sysfs, and this could be a route for
testing.

For sys metrics I guess we'd so something like:

sys/hisi_sicl/events/cpa_cycles
sys/hisi_sicl/events/cpa_cycles.desc
...
sys/cpa/events/cpa_cycles
sys/cpa/cpa_cycles.desc
...

or perhaps have some kind of wildcard matching syntax:

sys/(hisi_sicl|cpa)/events/cpa_cycles
sys/(hisi_sicl|cpa)/events/cpa_cycles.desc
...

So ultimately I can imagine the distinction of sys and cpu are going
to become less, and we just test properties of PMUs. The ideas of
tables should be hidden, but we could have a boolean on a PMU to say
whether it is a sys or cpu type.

> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  tools/perf/tests/expand-cgroup.c |  2 +-
>  tools/perf/tests/parse-metric.c  |  2 +-
>  tools/perf/tests/pmu-events.c    | 29 +++++++++++++++-----
>  tools/perf/util/metricgroup.c    | 45 +++++++++++++++++++++++---------
>  tools/perf/util/metricgroup.h    |  3 ++-
>  5 files changed, 59 insertions(+), 22 deletions(-)
>
> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
> index 9c1a1f18db75..50e128ddb474 100644
> --- a/tools/perf/tests/expand-cgroup.c
> +++ b/tools/perf/tests/expand-cgroup.c
> @@ -187,7 +187,7 @@ static int expand_metric_events(void)
>
>         rblist__init(&metric_events);
>         pme_test = find_core_metrics_table("testarch", "testcpu");
> -       ret = metricgroup__parse_groups_test(evlist, pme_test, metric_str, &metric_events);
> +       ret = metricgroup__parse_groups_test(evlist, pme_test, NULL, metric_str, &metric_events);

nit: Here and below. Could we name the argument here, so:
ret = metricgroup__parse_groups_test(evlist, pme_test,
/*sys_table=*/NULL, metric_str, &metric_events);
for clarity it would be nice if pme_test were cpu_table.

Thanks,
Ian


>         if (ret < 0) {
>                 pr_debug("failed to parse '%s' metric\n", metric_str);
>                 goto out;
> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
> index 2c28fb50dc24..e146f1193294 100644
> --- a/tools/perf/tests/parse-metric.c
> +++ b/tools/perf/tests/parse-metric.c
> @@ -95,7 +95,7 @@ static int __compute_metric(const char *name, struct value *vals,
>
>         /* Parse the metric into metric_events list. */
>         pme_test = find_core_metrics_table("testarch", "testcpu");
> -       err = metricgroup__parse_groups_test(evlist, pme_test, name,
> +       err = metricgroup__parse_groups_test(evlist, pme_test, NULL, name,
>                                              &metric_events);
>         if (err)
>                 goto out;
> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
> index 64383fc34ef1..de571fd11cd7 100644
> --- a/tools/perf/tests/pmu-events.c
> +++ b/tools/perf/tests/pmu-events.c
> @@ -798,9 +798,9 @@ struct metric {
>         struct metric_ref metric_ref;
>  };
>
> -static int test__parsing_callback(const struct pmu_metric *pm,
> +static int _test__parsing_callback(const struct pmu_metric *pm,
>                                   const struct pmu_metrics_table *table,
> -                                 void *data)
> +                                 void *data, bool is_cpu_table)
>  {
>         int *failures = data;
>         int k;
> @@ -811,6 +811,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>                 .nr_entries = 0,
>         };
>         int err = 0;
> +       const struct pmu_metrics_table *cpu_table = is_cpu_table ? table : NULL;
> +       const struct pmu_metrics_table *sys_table = is_cpu_table ? NULL : table;
>
>         if (!pm->metric_expr)
>                 return 0;
> @@ -834,7 +836,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>
>         perf_evlist__set_maps(&evlist->core, cpus, NULL);
>
> -       err = metricgroup__parse_groups_test(evlist, table, pm->metric_name, &metric_events);
> +       err = metricgroup__parse_groups_test(evlist, cpu_table, sys_table,
> +                                            pm->metric_name, &metric_events);
>         if (err) {
>                 if (!strcmp(pm->metric_name, "M1") || !strcmp(pm->metric_name, "M2") ||
>                     !strcmp(pm->metric_name, "M3")) {
> @@ -890,13 +893,27 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>         return err;
>  }
>
> -static int test__parsing(struct test_suite *test __maybe_unused,
> +static int test__parsing_callback_cpu(const struct pmu_metric *pm,
> +                                 const struct pmu_metrics_table *table,
> +                                 void *data)
> +{
> +       return _test__parsing_callback(pm, table, data, true);
> +}
> +
> +static int test__parsing_callback_sys(const struct pmu_metric *pm,
> +                                 const struct pmu_metrics_table *table,
> +                                 void *data)
> +{
> +       return _test__parsing_callback(pm, table, data, false);
> +}
> +
> +static __maybe_unused int test__parsing(struct test_suite *test __maybe_unused,
>                          int subtest __maybe_unused)
>  {
>         int failures = 0;
>
> -       pmu_for_each_core_metric(test__parsing_callback, &failures);
> -       pmu_for_each_sys_metric(test__parsing_callback, &failures);
> +       pmu_for_each_core_metric(test__parsing_callback_cpu, &failures);
> +       pmu_for_each_sys_metric(test__parsing_callback_sys, &failures);
>
>         return failures == 0 ? TEST_OK : TEST_FAIL;
>  }
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 8d2ac2513530..520436fbe99d 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -1232,13 +1232,14 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>                                    const char *user_requested_cpu_list,
>                                    bool system_wide,
>                                    struct list_head *metric_list,
> -                                  const struct pmu_metrics_table *table)
> +                                  const struct pmu_metrics_table *cpu_table,
> +                                  const struct pmu_metrics_table *sys_table)
>  {
>         LIST_HEAD(list);
>         int ret;
>         bool has_match = false;
>
> -       {
> +       if (cpu_table) {
>                 struct metricgroup__add_metric_data data = {
>                         .list = &list,
>                         .pmu = pmu,
> @@ -1254,7 +1255,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>                  * Iterate over all metrics seeing if metric matches either the
>                  * name or group. When it does add the metric to the list.
>                  */
> -               ret = pmu_metrics_table_for_each_metric(table, metricgroup__add_metric_callback,
> +               ret = pmu_metrics_table_for_each_metric(cpu_table, metricgroup__add_metric_callback,
>                                                        &data);
>                 if (ret)
>                         goto out;
> @@ -1267,7 +1268,21 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>                 goto out;
>         }
>
> -       {
> +       if (sys_table) {
> +               struct metricgroup_add_iter_data data = {
> +                       .metric_list = &list,
> +                       .pmu = pmu,
> +                       .metric_name = metric_name,
> +                       .modifier = modifier,
> +                       .metric_no_group = metric_no_group,
> +                       .user_requested_cpu_list = user_requested_cpu_list,
> +                       .system_wide = system_wide,
> +                       .has_match = &has_match,
> +                       .ret = &ret,
> +               };
> +               pmu_metrics_table_for_each_metric(sys_table,
> +                       metricgroup__add_metric_sys_event_iter, &data);
> +       } else {
>                 struct metricgroup_iter_data data = {
>                         .fn = metricgroup__add_metric_sys_event_iter,
>                         .data = (void *) &(struct metricgroup_add_iter_data) {
> @@ -1320,7 +1335,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
>                                         bool metric_no_threshold,
>                                         const char *user_requested_cpu_list,
>                                         bool system_wide, struct list_head *metric_list,
> -                                       const struct pmu_metrics_table *table)
> +                                       const struct pmu_metrics_table *cpu_table,
> +                                       const struct pmu_metrics_table *sys_table)
>  {
>         char *list_itr, *list_copy, *metric_name, *modifier;
>         int ret, count = 0;
> @@ -1338,7 +1354,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
>                 ret = metricgroup__add_metric(pmu, metric_name, modifier,
>                                               metric_no_group, metric_no_threshold,
>                                               user_requested_cpu_list,
> -                                             system_wide, metric_list, table);
> +                                             system_wide, metric_list, cpu_table,
> +                                             sys_table);
>                 if (ret == -EINVAL)
>                         pr_err("Cannot find metric or group `%s'\n", metric_name);
>
> @@ -1534,7 +1551,8 @@ static int parse_groups(struct evlist *perf_evlist,
>                         bool system_wide,
>                         struct perf_pmu *fake_pmu,
>                         struct rblist *metric_events_list,
> -                       const struct pmu_metrics_table *table)
> +                       const struct pmu_metrics_table *cpu_table,
> +                       const struct pmu_metrics_table *sys_table)
>  {
>         struct evlist *combined_evlist = NULL;
>         LIST_HEAD(metric_list);
> @@ -1547,7 +1565,7 @@ static int parse_groups(struct evlist *perf_evlist,
>                 metricgroup__rblist_init(metric_events_list);
>         ret = metricgroup__add_metric_list(pmu, str, metric_no_group, metric_no_threshold,
>                                            user_requested_cpu_list,
> -                                          system_wide, &metric_list, table);
> +                                          system_wide, &metric_list, cpu_table, sys_table);
>         if (ret)
>                 goto out;
>
> @@ -1697,18 +1715,19 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
>                               bool system_wide,
>                               struct rblist *metric_events)
>  {
> -       const struct pmu_metrics_table *table = pmu_metrics_table__find();
> +       const struct pmu_metrics_table *cpu_table = pmu_metrics_table__find();
>
> -       if (!table)
> +       if (!cpu_table)
>                 return -EINVAL;
>
>         return parse_groups(perf_evlist, pmu, str, metric_no_group, metric_no_merge,
>                             metric_no_threshold, user_requested_cpu_list, system_wide,
> -                           /*fake_pmu=*/NULL, metric_events, table);
> +                           /*fake_pmu=*/NULL, metric_events, cpu_table, NULL);
>  }
>
>  int metricgroup__parse_groups_test(struct evlist *evlist,
> -                                  const struct pmu_metrics_table *table,
> +                                  const struct pmu_metrics_table *cpu_table,
> +                                  const struct pmu_metrics_table *sys_table,
>                                    const char *str,
>                                    struct rblist *metric_events)
>  {
> @@ -1718,7 +1737,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
>                             /*metric_no_threshold=*/false,
>                             /*user_requested_cpu_list=*/NULL,
>                             /*system_wide=*/false,
> -                           &perf_pmu__fake, metric_events, table);
> +                           &perf_pmu__fake, metric_events, cpu_table, sys_table);
>  }
>
>  struct metricgroup__has_metric_data {
> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
> index d5325c6ec8e1..b5f0d598eaec 100644
> --- a/tools/perf/util/metricgroup.h
> +++ b/tools/perf/util/metricgroup.h
> @@ -79,7 +79,8 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
>                               bool system_wide,
>                               struct rblist *metric_events);
>  int metricgroup__parse_groups_test(struct evlist *evlist,
> -                                  const struct pmu_metrics_table *table,
> +                                  const struct pmu_metrics_table *cpu_table,
> +                                  const struct pmu_metrics_table *sys_table,
>                                    const char *str,
>                                    struct rblist *metric_events);
>
> --
> 2.35.3
>

Re: [PATCH RFC 3/9] perf metrics: Pass cpu and sys tables to metricgroup__add_metric()

[John Garry]

On 30/06/2023 19:39, Ian Rogers wrote:
> On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote:
>>
>> The current @table arg may be a CPU metric table or a sys metric table.
>> There is no point in searching the CPU metric table for a sys metric, and
>> vice versa, so pass separate pointers
>>
>> When sys metric table is passed, this would mean that we are in self-test
>> mode. In this mode, the host system cannot match events in the metric
>> expression as the associated PMUs may not be present. As such, just try
>> add the metric, see metricgroup__add_metric_sys_event_iter().
> 
> Thanks John, I'm not opposed to this change. My understanding is it
> will give greater testing coverage. As previously mentioned I'd like
> longer term we have a sysfs like abstraction for the json events. For
> CPUs this could be like:
> 
> <cpuid>/cpu/events/inst_retired.any
> <cpuid>/cpu/events/inst_retired.any.scale
> <cpuid>/cpu/events/inst_retired.any.unit
> <cpuid>/cpu/events/inst_retired.any.desc
> ...
> <cpuid>/cpu/metrics/ipc
> <cpuid>/cpu/metrics/ipc.scale
> <cpuid>/cpu/metrics/ipc.unit
> ...
> <cpuid>/uncore_imc_free_running_0/events/unc_mc0_rdcas_count_freerun
> ...
> 
> Where <cpuid> comes from mapfile.csv. I'd like to union the in memory
> json event generated sysfs, with the kernel sysfs. There needs to be
> some kind of wildcard mechanism for all the uncore counters. Such a
> union-ing could allow on an disk sysfs, and this could be a route for
> testing.
> 
> For sys metrics I guess we'd so something like:
> 
> sys/hisi_sicl/events/cpa_cycles
> sys/hisi_sicl/events/cpa_cycles.desc
> ...
> sys/cpa/events/cpa_cycles
> sys/cpa/cpa_cycles.desc
> ...
> 
> or perhaps have some kind of wildcard matching syntax:
> 
> sys/(hisi_sicl|cpa)/events/cpa_cycles
> sys/(hisi_sicl|cpa)/events/cpa_cycles.desc
> ...
> 
> So ultimately I can imagine the distinction of sys and cpu are going
> to become less, and we just test properties of PMUs. The ideas of
> tables should be hidden, but we could have a boolean on a PMU to say
> whether it is a sys or cpu type.

Hi Ian,

I am not too hung up on my change in this patch really. It was more a 
prep change for better test coverage, but the test coverage was not 
added yet.

Ideas on testing would be helpful, but that can be once the changes in 
patches 4-6 are agreed.

Thanks,
John

> 
>> Signed-off-by: John Garry <john.g.garry@oracle.com>
>> ---
>>   tools/perf/tests/expand-cgroup.c |  2 +-
>>   tools/perf/tests/parse-metric.c  |  2 +-
>>   tools/perf/tests/pmu-events.c    | 29 +++++++++++++++-----
>>   tools/perf/util/metricgroup.c    | 45 +++++++++++++++++++++++---------
>>   tools/perf/util/metricgroup.h    |  3 ++-
>>   5 files changed, 59 insertions(+), 22 deletions(-)
>>
>> diff --git a/tools/perf/tests/expand-cgroup.c b/tools/perf/tests/expand-cgroup.c
>> index 9c1a1f18db75..50e128ddb474 100644
>> --- a/tools/perf/tests/expand-cgroup.c
>> +++ b/tools/perf/tests/expand-cgroup.c
>> @@ -187,7 +187,7 @@ static int expand_metric_events(void)
>>
>>          rblist__init(&metric_events);
>>          pme_test = find_core_metrics_table("testarch", "testcpu");
>> -       ret = metricgroup__parse_groups_test(evlist, pme_test, metric_str, &metric_events);
>> +       ret = metricgroup__parse_groups_test(evlist, pme_test, NULL, metric_str, &metric_events);
> 
> nit: Here and below. Could we name the argument here, so:
> ret = metricgroup__parse_groups_test(evlist, pme_test,
> /*sys_table=*/NULL, metric_str, &metric_events);
> for clarity it would be nice if pme_test were cpu_table.
> 
> Thanks,
> Ian
> 
> 
>>          if (ret < 0) {
>>                  pr_debug("failed to parse '%s' metric\n", metric_str);
>>                  goto out;
>> diff --git a/tools/perf/tests/parse-metric.c b/tools/perf/tests/parse-metric.c
>> index 2c28fb50dc24..e146f1193294 100644
>> --- a/tools/perf/tests/parse-metric.c
>> +++ b/tools/perf/tests/parse-metric.c
>> @@ -95,7 +95,7 @@ static int __compute_metric(const char *name, struct value *vals,
>>
>>          /* Parse the metric into metric_events list. */
>>          pme_test = find_core_metrics_table("testarch", "testcpu");
>> -       err = metricgroup__parse_groups_test(evlist, pme_test, name,
>> +       err = metricgroup__parse_groups_test(evlist, pme_test, NULL, name,
>>                                               &metric_events);
>>          if (err)
>>                  goto out;
>> diff --git a/tools/perf/tests/pmu-events.c b/tools/perf/tests/pmu-events.c
>> index 64383fc34ef1..de571fd11cd7 100644
>> --- a/tools/perf/tests/pmu-events.c
>> +++ b/tools/perf/tests/pmu-events.c
>> @@ -798,9 +798,9 @@ struct metric {
>>          struct metric_ref metric_ref;
>>   };
>>
>> -static int test__parsing_callback(const struct pmu_metric *pm,
>> +static int _test__parsing_callback(const struct pmu_metric *pm,
>>                                    const struct pmu_metrics_table *table,
>> -                                 void *data)
>> +                                 void *data, bool is_cpu_table)
>>   {
>>          int *failures = data;
>>          int k;
>> @@ -811,6 +811,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>>                  .nr_entries = 0,
>>          };
>>          int err = 0;
>> +       const struct pmu_metrics_table *cpu_table = is_cpu_table ? table : NULL;
>> +       const struct pmu_metrics_table *sys_table = is_cpu_table ? NULL : table;
>>
>>          if (!pm->metric_expr)
>>                  return 0;
>> @@ -834,7 +836,8 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>>
>>          perf_evlist__set_maps(&evlist->core, cpus, NULL);
>>
>> -       err = metricgroup__parse_groups_test(evlist, table, pm->metric_name, &metric_events);
>> +       err = metricgroup__parse_groups_test(evlist, cpu_table, sys_table,
>> +                                            pm->metric_name, &metric_events);
>>          if (err) {
>>                  if (!strcmp(pm->metric_name, "M1") || !strcmp(pm->metric_name, "M2") ||
>>                      !strcmp(pm->metric_name, "M3")) {
>> @@ -890,13 +893,27 @@ static int test__parsing_callback(const struct pmu_metric *pm,
>>          return err;
>>   }
>>
>> -static int test__parsing(struct test_suite *test __maybe_unused,
>> +static int test__parsing_callback_cpu(const struct pmu_metric *pm,
>> +                                 const struct pmu_metrics_table *table,
>> +                                 void *data)
>> +{
>> +       return _test__parsing_callback(pm, table, data, true);
>> +}
>> +
>> +static int test__parsing_callback_sys(const struct pmu_metric *pm,
>> +                                 const struct pmu_metrics_table *table,
>> +                                 void *data)
>> +{
>> +       return _test__parsing_callback(pm, table, data, false);
>> +}
>> +
>> +static __maybe_unused int test__parsing(struct test_suite *test __maybe_unused,
>>                           int subtest __maybe_unused)
>>   {
>>          int failures = 0;
>>
>> -       pmu_for_each_core_metric(test__parsing_callback, &failures);
>> -       pmu_for_each_sys_metric(test__parsing_callback, &failures);
>> +       pmu_for_each_core_metric(test__parsing_callback_cpu, &failures);
>> +       pmu_for_each_sys_metric(test__parsing_callback_sys, &failures);
>>
>>          return failures == 0 ? TEST_OK : TEST_FAIL;
>>   }
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 8d2ac2513530..520436fbe99d 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -1232,13 +1232,14 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>>                                     const char *user_requested_cpu_list,
>>                                     bool system_wide,
>>                                     struct list_head *metric_list,
>> -                                  const struct pmu_metrics_table *table)
>> +                                  const struct pmu_metrics_table *cpu_table,
>> +                                  const struct pmu_metrics_table *sys_table)
>>   {
>>          LIST_HEAD(list);
>>          int ret;
>>          bool has_match = false;
>>
>> -       {
>> +       if (cpu_table) {
>>                  struct metricgroup__add_metric_data data = {
>>                          .list = &list,
>>                          .pmu = pmu,
>> @@ -1254,7 +1255,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>>                   * Iterate over all metrics seeing if metric matches either the
>>                   * name or group. When it does add the metric to the list.
>>                   */
>> -               ret = pmu_metrics_table_for_each_metric(table, metricgroup__add_metric_callback,
>> +               ret = pmu_metrics_table_for_each_metric(cpu_table, metricgroup__add_metric_callback,
>>                                                         &data);
>>                  if (ret)
>>                          goto out;
>> @@ -1267,7 +1268,21 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
>>                  goto out;
>>          }
>>
>> -       {
>> +       if (sys_table) {
>> +               struct metricgroup_add_iter_data data = {
>> +                       .metric_list = &list,
>> +                       .pmu = pmu,
>> +                       .metric_name = metric_name,
>> +                       .modifier = modifier,
>> +                       .metric_no_group = metric_no_group,
>> +                       .user_requested_cpu_list = user_requested_cpu_list,
>> +                       .system_wide = system_wide,
>> +                       .has_match = &has_match,
>> +                       .ret = &ret,
>> +               };
>> +               pmu_metrics_table_for_each_metric(sys_table,
>> +                       metricgroup__add_metric_sys_event_iter, &data);
>> +       } else {
>>                  struct metricgroup_iter_data data = {
>>                          .fn = metricgroup__add_metric_sys_event_iter,
>>                          .data = (void *) &(struct metricgroup_add_iter_data) {
>> @@ -1320,7 +1335,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
>>                                          bool metric_no_threshold,
>>                                          const char *user_requested_cpu_list,
>>                                          bool system_wide, struct list_head *metric_list,
>> -                                       const struct pmu_metrics_table *table)
>> +                                       const struct pmu_metrics_table *cpu_table,
>> +                                       const struct pmu_metrics_table *sys_table)
>>   {
>>          char *list_itr, *list_copy, *metric_name, *modifier;
>>          int ret, count = 0;
>> @@ -1338,7 +1354,8 @@ static int metricgroup__add_metric_list(const char *pmu, const char *list,
>>                  ret = metricgroup__add_metric(pmu, metric_name, modifier,
>>                                                metric_no_group, metric_no_threshold,
>>                                                user_requested_cpu_list,
>> -                                             system_wide, metric_list, table);
>> +                                             system_wide, metric_list, cpu_table,
>> +                                             sys_table);
>>                  if (ret == -EINVAL)
>>                          pr_err("Cannot find metric or group `%s'\n", metric_name);
>>
>> @@ -1534,7 +1551,8 @@ static int parse_groups(struct evlist *perf_evlist,
>>                          bool system_wide,
>>                          struct perf_pmu *fake_pmu,
>>                          struct rblist *metric_events_list,
>> -                       const struct pmu_metrics_table *table)
>> +                       const struct pmu_metrics_table *cpu_table,
>> +                       const struct pmu_metrics_table *sys_table)
>>   {
>>          struct evlist *combined_evlist = NULL;
>>          LIST_HEAD(metric_list);
>> @@ -1547,7 +1565,7 @@ static int parse_groups(struct evlist *perf_evlist,
>>                  metricgroup__rblist_init(metric_events_list);
>>          ret = metricgroup__add_metric_list(pmu, str, metric_no_group, metric_no_threshold,
>>                                             user_requested_cpu_list,
>> -                                          system_wide, &metric_list, table);
>> +                                          system_wide, &metric_list, cpu_table, sys_table);
>>          if (ret)
>>                  goto out;
>>
>> @@ -1697,18 +1715,19 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
>>                                bool system_wide,
>>                                struct rblist *metric_events)
>>   {
>> -       const struct pmu_metrics_table *table = pmu_metrics_table__find();
>> +       const struct pmu_metrics_table *cpu_table = pmu_metrics_table__find();
>>
>> -       if (!table)
>> +       if (!cpu_table)
>>                  return -EINVAL;
>>
>>          return parse_groups(perf_evlist, pmu, str, metric_no_group, metric_no_merge,
>>                              metric_no_threshold, user_requested_cpu_list, system_wide,
>> -                           /*fake_pmu=*/NULL, metric_events, table);
>> +                           /*fake_pmu=*/NULL, metric_events, cpu_table, NULL);
>>   }
>>
>>   int metricgroup__parse_groups_test(struct evlist *evlist,
>> -                                  const struct pmu_metrics_table *table,
>> +                                  const struct pmu_metrics_table *cpu_table,
>> +                                  const struct pmu_metrics_table *sys_table,
>>                                     const char *str,
>>                                     struct rblist *metric_events)
>>   {
>> @@ -1718,7 +1737,7 @@ int metricgroup__parse_groups_test(struct evlist *evlist,
>>                              /*metric_no_threshold=*/false,
>>                              /*user_requested_cpu_list=*/NULL,
>>                              /*system_wide=*/false,
>> -                           &perf_pmu__fake, metric_events, table);
>> +                           &perf_pmu__fake, metric_events, cpu_table, sys_table);
>>   }
>>
>>   struct metricgroup__has_metric_data {
>> diff --git a/tools/perf/util/metricgroup.h b/tools/perf/util/metricgroup.h
>> index d5325c6ec8e1..b5f0d598eaec 100644
>> --- a/tools/perf/util/metricgroup.h
>> +++ b/tools/perf/util/metricgroup.h
>> @@ -79,7 +79,8 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
>>                                bool system_wide,
>>                                struct rblist *metric_events);
>>   int metricgroup__parse_groups_test(struct evlist *evlist,
>> -                                  const struct pmu_metrics_table *table,
>> +                                  const struct pmu_metrics_table *cpu_table,
>> +                                  const struct pmu_metrics_table *sys_table,
>>                                     const char *str,
>>                                     struct rblist *metric_events);
>>
>> --
>> 2.35.3
>>

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

[John Garry]

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

[Ian Rogers]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

[John Garry]

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

[Ian Rogers]

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

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

[Arnaldo Carvalho de Melo]

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

[PATCH RFC 5/9] perf pmu: Refactor pmu_add_sys_aliases_iter_fn()

[John Garry]

Refactor pmu_add_sys_aliases_iter_fn() into sub-function
pmu_event_match_pmu(), which is a function which allows us to match a pmu
against a "unit" and "compat" values.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/pmu.c | 20 +++++++++++++++-----
 tools/perf/util/pmu.h |  2 ++
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 6142e4710a2f..ae1a2a97965c 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -801,6 +801,20 @@ struct pmu_sys_event_iter_data {
 	struct perf_pmu *pmu;
 };
 
+bool pmu_event_match_pmu(const char *event_unit, const char *event_compat, struct perf_pmu *pmu)
+{
+	if (!event_unit || !event_compat)
+		return false;
+
+	if (strcmp(pmu->id, event_compat))
+		return false;
+
+	if (!pmu_uncore_alias_match(event_unit, pmu->name))
+		return false;
+
+	return true;
+}
+
 static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
 				       const struct pmu_events_table *table __maybe_unused,
 				       void *data)
@@ -808,11 +822,7 @@ static int pmu_add_sys_aliases_iter_fn(const struct pmu_event *pe,
 	struct pmu_sys_event_iter_data *idata = data;
 	struct perf_pmu *pmu = idata->pmu;
 
-	if (!pe->compat || !pe->pmu)
-		return 0;
-
-	if (!strcmp(pmu->id, pe->compat) &&
-	    pmu_uncore_alias_match(pe->pmu, pmu->name)) {
+	if (pmu_event_match_pmu(pe->pmu, pe->compat, pmu)) {
 		__perf_pmu__new_alias(idata->head, -1,
 				      (char *)pe->name,
 				      (char *)pe->desc,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 8807a624e918..8cccbc9b2548 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -288,4 +288,6 @@ int perf_pmu__pathname_fd(int dirfd, const char *pmu_name, const char *filename,
 struct perf_pmu *perf_pmu__lookup(struct list_head *pmus, int dirfd, const char *lookup_name);
 void perf_pmu__delete(struct perf_pmu *pmu);
 
+bool pmu_event_match_pmu(const char *event_name, const char *event_compat, struct perf_pmu *pmu);
+
 #endif /* __PMU_H */
-- 
2.35.3

[PATCH RFC 6/9] perf metrics: Add metricgroup_sys_metric_supported()

[John Garry]

Add a function to match a sys metric for the host.

Currently we do the following the match a metric:
- iter through all sys metrics for all sys event tables
- for each iter, iter through all PMUs
- if the PMU Unit and id match the metric Unit and compat, then the metric
  matches for the host

We should not be required to match a metric Unit and compat, unless the
metric explicitly specifies target PMU, like:
 "MetricExpr": "imx8_ddr0@axid\\-read\\,axi_mask\\=0xffff\\,axi_id\\=0x0000@",

Rather, we should be match against events aliases present from the events
in the same pmu_sys_events structure. Those events added will be gated on
matching Unit and Compat specifiers.

The steps in metricgroup_match_sys_pm() are as follows:
a. Find event table associated with metric table
b. Call add_metric() for the metric, which resolves referenced metrics and
   generates a list of type struct metric
c. Iter through metric list and parse ids, to find a list of evsel's
d. For each evsel, match event against known events in event table
e. If we can't match an evsel as the metric explicitly specifies target PMU,
   just match evsel PMU against metric Unit and Compat.

Step d. is required as in parse_ids() -> parse_events_add_pmu(), we might
have matched metric for other system which has the same name, i.e. when
we find matching event and PMU, we must ensure it matches compat for event
from metric's associated event table.

Function metricgroup__sys_event_iter() will now call
metricgroup_sys_metric_supported() to match the metric.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/metricgroup.c | 115 +++++++++++++++++++++++++++++++---
 1 file changed, 105 insertions(+), 10 deletions(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 520436fbe99d..6be410363099 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -486,23 +486,18 @@ struct metricgroup_iter_data {
 	void *data;
 };
 
+static bool metricgroup_sys_metric_supported(const struct pmu_metric *pm,
+			const struct pmu_metrics_table *table);
+
 static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
 				       const struct pmu_metrics_table *table,
 				       void *data)
 {
 	struct metricgroup_iter_data *d = data;
-	struct perf_pmu *pmu = NULL;
-
-	if (!pm->metric_expr || !pm->compat)
-		return 0;
-
-	while ((pmu = perf_pmus__scan(pmu))) {
-
-		if (!pmu->id || strcmp(pmu->id, pm->compat))
-			continue;
 
+	if (metricgroup_sys_metric_supported(pm, table))
 		return d->fn(pm, table, d->data);
-	}
+
 	return 0;
 }
 
@@ -1705,6 +1700,106 @@ static int parse_groups(struct evlist *perf_evlist,
 	return ret;
 }
 
+struct metricgroup__test_event {
+	struct pmu_event *found_event;
+	struct perf_pmu *pmu;
+	char *evsel_name;
+};
+
+static int metricgroup__test_event_iter(const struct pmu_event *pe,
+		const struct pmu_events_table *table __maybe_unused,
+		void *vdata)
+{
+	struct metricgroup__test_event *data = vdata;
+	struct pmu_event *found_event = data->found_event;
+	char *evsel_name = data->evsel_name;
+	struct perf_pmu *pmu = data->pmu;
+
+	if (strcasecmp(pe->name, evsel_name))
+		return 0;
+
+	if (!strcmp(pmu->id, pe->compat)) {
+		/* Copy, as pe is only valid in the iter */
+		memcpy(found_event, pe, sizeof(*pe));
+		/* Stop iter'ing */
+		return 1;
+	}
+
+	return 0;
+}
+
+static bool metricgroup_sys_metric_supported(const struct pmu_metric *pm,
+				const struct pmu_metrics_table *table)
+{
+	const struct pmu_events_table *events_table;
+	bool tool_events[PERF_TOOL_MAX] = {false};
+	struct metric *m = NULL;
+	bool ret = false;
+	int err;
+	LIST_HEAD(list);
+
+	events_table = sys_events_find_events_table(table);
+	if (!events_table)
+		return false;
+
+	err = add_metric(&list /* d->metric_list */, pm,
+			 NULL /* modifier */, false /* metric_no_group */,
+			 false /* metric_no_threshold */, false /* user_requested_cpu_list */,
+			 false /*system wide */, NULL /* root metric */,
+			 NULL /* visited */, table);
+	if (ret)
+		return false;
+
+	/*
+	 * We have a list of metrics. Now generate an evlist per metric, and
+	 * match each evsel. We match evsel by findings its corresponding PMU
+	 * and then ensuring the we can find it in either a. the event list
+	 * associated with the metric or b. if it is a metric referencing an
+	 * explicit PMU. In both cases we match the pmu->id against the event
+	 * compat.
+	 */
+	list_for_each_entry(m, &list, nd) {
+		struct evsel *evsel;
+
+		err = parse_ids(false, NULL, m->pctx, m->modifier,
+				m->group_events, tool_events, &m->evlist);
+		if (err)
+			return false;
+
+		evlist__for_each_entry(m->evlist, evsel) {
+			struct perf_pmu *pmu = perf_pmus__find(evsel->pmu_name);
+			struct pmu_event found_event = {};
+			struct metricgroup__test_event data = {
+				.evsel_name = evsel->name,
+				.found_event = &found_event,
+				.pmu = pmu,
+			};
+
+			if (!pmu)
+				return false;
+
+			pmu_events_table_for_each_event(events_table,
+					metricgroup__test_event_iter,
+					&data);
+			if (found_event.name)
+				continue;
+
+			/*
+			 * We dould not find alias event, so maybe our evsel
+			 * is for a specific PMU.
+			 */
+			if (strchr(evsel->name, '/')) {
+				if (pmu_event_match_pmu(pm->pmu, pm->compat, pmu))
+					continue;
+			}
+
+			return false;
+		}
+	}
+
+	return true;
+}
+
 int metricgroup__parse_groups(struct evlist *perf_evlist,
 			      const char *pmu,
 			      const char *str,
-- 
2.35.3

[PATCH RFC 7/9] perf metrics: Test metric match in metricgroup__sys_event_iter()

[John Garry]

When we add a metric in metricgroup__add_metric(), we first iter through
all metrics in each table from pmu_sys_event_tables tables and call
metricgroup_sys_metric_supported() to test whether we support that metric.
The second step is to call metricgroup__add_metric_sys_event_iter() ->
match_pm_metric() to check if this is actually the metric we're looking
for. It would be better before calling metricgroup_sys_metric_supported()
at all to first test whether we're interested in that metric.

Add metricgroup_iter_data.metric_name, which will be set when we're
looking for a specific metric in the iter, and check for that in
metricgroup__sys_event_iter(). In a way this duplicates the
metricgroup__add_metric_sys_event_iter() -> match_pm_metric() check, but
that is needed for other cases.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/metricgroup.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 6be410363099..111ad4e3eb6b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -483,6 +483,7 @@ static int metricgroup__add_to_mep_groups(const struct pmu_metric *pm,
 
 struct metricgroup_iter_data {
 	pmu_metric_iter_fn fn;
+	const char *metric_name;
 	void *data;
 };
 
@@ -495,6 +496,10 @@ static int metricgroup__sys_event_iter(const struct pmu_metric *pm,
 {
 	struct metricgroup_iter_data *d = data;
 
+	/* We may be only interested in a specific metric */
+	if (d->metric_name && strcasecmp(d->metric_name, pm->metric_name))
+		return 0;
+
 	if (metricgroup_sys_metric_supported(pm, table))
 		return d->fn(pm, table, d->data);
 
@@ -1291,6 +1296,7 @@ static int metricgroup__add_metric(const char *pmu, const char *metric_name, con
 				.has_match = &has_match,
 				.ret = &ret,
 			},
+			.metric_name = metric_name,
 		};
 
 		pmu_for_each_sys_metric(metricgroup__sys_event_iter, &data);
-- 
2.35.3

[PATCH RFC 8/9] perf metrics: Stop metricgroup__add_metric_sys_event_iter if already matched

[John Garry]

Functions like pmu_for_each_sys_event() may be inefficient, as we only
stop itering for an error. Often when itering, we may want to stop early
as we already found what we're looking for.

Return 1 in metricgroup__add_metric_sys_event_iter() when we want to stop.
Nobody checks the error code from callers anyway - those are
pmu_metrics_table_for_each_metric() ->
metricgroup__add_metric_sys_event_iter() and pmu_for_each_sys_metric() ->
metricgroup__sys_event_iter() -> metricgroup__add_metric_sys_event_iter().

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 tools/perf/util/metricgroup.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 111ad4e3eb6b..c045b111db84 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -1130,7 +1130,12 @@ static int metricgroup__add_metric_sys_event_iter(const struct pmu_metric *pm,
 
 out:
 	*(d->ret) = ret;
-	return ret;
+	/*
+	 * Return 1 as we don't want to iter any more, as either:
+	 * a. We found a match
+	 * b. We tried to add a metric, which errored
+	 */
+	return 1;
 }
 
 /**
-- 
2.35.3

[PATCH RFC 9/9] perf vendor events arm64: Remove unnecessary metric Unit and Compat specifiers

[John Garry]

It is now not necessary to specify Unit and Compat for metrics which
reference events for the same system, so remove them.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 .../pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json   | 4 ----
 .../pmu-events/arch/arm64/freescale/imx8mn/sys/metrics.json   | 4 ----
 .../pmu-events/arch/arm64/freescale/imx8mq/sys/metrics.json   | 4 ----
 .../pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json | 4 ----
 4 files changed, 16 deletions(-)

diff --git a/tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json
index f416fa052337..574aeb964efd 100644
--- a/tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json
+++ b/tools/perf/pmu-events/arch/arm64/freescale/imx8mm/sys/metrics.json
@@ -4,15 +4,11 @@
 	    "MetricName": "imx8mm_ddr_read.all",
 	    "MetricExpr": "imx8mm_ddr.read_cycles * 4 * 4",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MM"
    },
    {
 	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
 	    "MetricName": "imx8mm_ddr_write.all",
 	    "MetricExpr": "imx8mm_ddr.write_cycles * 4 * 4",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MM"
    }
 ]
diff --git a/tools/perf/pmu-events/arch/arm64/freescale/imx8mn/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/freescale/imx8mn/sys/metrics.json
index 2bbba4d8ea5b..d81e7d562b65 100644
--- a/tools/perf/pmu-events/arch/arm64/freescale/imx8mn/sys/metrics.json
+++ b/tools/perf/pmu-events/arch/arm64/freescale/imx8mn/sys/metrics.json
@@ -4,15 +4,11 @@
 	    "MetricName": "imx8mn_ddr_read.all",
 	    "MetricExpr": "imx8mn_ddr.read_cycles * 4 * 2",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MN"
    },
    {
 	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
 	    "MetricName": "imx8mn_ddr_write.all",
 	    "MetricExpr": "imx8mn_ddr.write_cycles * 4 * 2",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MN"
    }
 ]
diff --git a/tools/perf/pmu-events/arch/arm64/freescale/imx8mq/sys/metrics.json b/tools/perf/pmu-events/arch/arm64/freescale/imx8mq/sys/metrics.json
index 862c98171e0d..d2a71b02ab59 100644
--- a/tools/perf/pmu-events/arch/arm64/freescale/imx8mq/sys/metrics.json
+++ b/tools/perf/pmu-events/arch/arm64/freescale/imx8mq/sys/metrics.json
@@ -4,15 +4,11 @@
 	    "MetricName": "imx8mq_ddr_read.all",
 	    "MetricExpr": "imx8mq_ddr.read_cycles * 4 * 4",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MQ"
    },
    {
 	    "BriefDescription": "bytes all masters write to ddr based on write-cycles event",
 	    "MetricName": "imx8mq_ddr_write.all",
 	    "MetricExpr": "imx8mq_ddr.write_cycles * 4 * 4",
 	    "ScaleUnit": "9.765625e-4KB",
-	    "Unit": "imx8_ddr",
-	    "Compat": "i.MX8MQ"
     }
 ]
diff --git a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
index 7bcddec8a84f..c2ff0d85c354 100644
--- a/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
+++ b/tools/perf/pmu-events/arch/arm64/hisilicon/hip09/sys/uncore-cpa.json
@@ -67,15 +67,11 @@
 		"BriefDescription": "Average bandwidth of CPA Port 1",
 		"MetricGroup": "CPA",
 		"MetricName": "cpa_p1_avg_bw",
-		"Compat": "0x00000030",
-		"Unit": "hisi_sicl,cpa"
 	},
 	{
 		"MetricExpr": "(cpa_p0_wr_dat * 64 + cpa_p0_rd_dat_64b * 64 + cpa_p0_rd_dat_32b * 32) / cpa_cycles",
 		"BriefDescription": "Average bandwidth of CPA Port 0",
 		"MetricGroup": "CPA",
 		"MetricName": "cpa_p0_avg_bw",
-		"Compat": "0x00000030",
-		"Unit": "hisi_sicl,cpa"
 	}
 ]
-- 
2.35.3

Re: [PATCH RFC 0/9] perf tool: sys event metric support re-write

[Namhyung Kim]

Hello,

On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@oracle.com> wrote:
>
> The current sys event metric support has some issues, like:
> - It is broken that we only match a metric based on PMU compat, but not
>   Unit as well, as reported by Jing Zhang <renyu.zj@linux.alibaba.com>
> - No real self-test support
> - Not able to use resolvable metrics
> - Need to specify event PMU Unit and Compat for metric, which should not
>   be necessary
>
> This series changes sys event metric support to match metrics based on
> evaluating each term in the metric expression and then ensuring it
> matches an event from the same associated pmu_sys_events table.
>
> Why an RFC?
> - Even though main motivation here was to improve self-test support, that
>   has proved quite tricky and nothing has been added yet.
>   My desire is to test the feature that we match metrics for a specific
>   SoC when PMUs with matching HW identifier are present. So I would hope
>   to add sys metrics for many SoCs in ../pmu-events/arch/test/
> - I still need to suppress logs from metricgroup_sys_metric_supported()
>   indirect calls to functions like parse_events_multi_pmu_add(),
>   generating logs like
>   "smmuv3_pmcg.wr_sent_sp -> smmuv3_pmcg_50/event=0x86/" - we should only
>   see those logs for when really adding the metric in calling add_metric()
>
> Based on 82fe2e45cdb0 (acme/tmp.perf/core, acme/tmp.perf-tools-next, acme/perf/core, acme/perf-tools-next) perf pmus: Check if we can encode the PMU number in perf_event_attr.type

We moved to new repos from acme to perf/perf-tools and perf/perf-tools-next.
You'd better rebase the series onto perf-tools-next (branch name is the same).

Thanks,
Namhyung


>
> John Garry (9):
>   perf metrics: Delete metricgroup_add_iter_data.table
>   perf metrics: Don't iter sys metrics if we already found a CPU match
>   perf metrics: Pass cpu and sys tables to metricgroup__add_metric()
>   perf jevents: Add sys_events_find_events_table()
>   perf pmu: Refactor pmu_add_sys_aliases_iter_fn()
>   perf metrics: Add metricgroup_sys_metric_supported()
>   perf metrics: Test metric match in metricgroup__sys_event_iter()
>   perf metrics: Stop metricgroup__add_metric_sys_event_iter if already
>     matched
>   perf vendor events arm64: Remove unnecessary metric Unit and Compat
>     specifiers
>
>  .../arm64/freescale/imx8mm/sys/metrics.json   |   4 -
>  .../arm64/freescale/imx8mn/sys/metrics.json   |   4 -
>  .../arm64/freescale/imx8mq/sys/metrics.json   |   4 -
>  .../arm64/hisilicon/hip09/sys/uncore-cpa.json |   4 -
>  tools/perf/pmu-events/empty-pmu-events.c      |   6 +
>  tools/perf/pmu-events/jevents.py              |  11 ++
>  tools/perf/pmu-events/pmu-events.h            |   3 +
>  tools/perf/tests/expand-cgroup.c              |   2 +-
>  tools/perf/tests/parse-metric.c               |   2 +-
>  tools/perf/tests/pmu-events.c                 |  29 ++-
>  tools/perf/util/metricgroup.c                 | 182 +++++++++++++++---
>  tools/perf/util/metricgroup.h                 |   3 +-
>  tools/perf/util/pmu.c                         |  20 +-
>  tools/perf/util/pmu.h                         |   2 +
>  14 files changed, 220 insertions(+), 56 deletions(-)
>
> --
> 2.35.3
>

Re: [PATCH RFC 0/9] perf tool: sys event metric support re-write

[John Garry]

>>
>> Based on 82fe2e45cdb0 (acme/tmp.perf/core, acme/tmp.perf-tools-next, acme/perf/core, acme/perf-tools-next) perf pmus: Check if we can encode the PMU number in perf_event_attr.type
> 
> We moved to new repos from acme to perf/perf-tools and perf/perf-tools-next.
> You'd better rebase the series onto perf-tools-next (branch name is the same).

Is that in the MAINTAINERS file? I could not see it.

And I was hoping that Ian could first have a look, since this is just an 
RFC.

Cheers,
John

Re: [PATCH RFC 0/9] perf tool: sys event metric support re-write

[Namhyung Kim]

On Fri, Jun 30, 2023 at 2:35 AM John Garry <john.g.garry@oracle.com> wrote:
>
>
> >>
> >> Based on 82fe2e45cdb0 (acme/tmp.perf/core, acme/tmp.perf-tools-next, acme/perf/core, acme/perf-tools-next) perf pmus: Check if we can encode the PMU number in perf_event_attr.type
> >
> > We moved to new repos from acme to perf/perf-tools and perf/perf-tools-next.
> > You'd better rebase the series onto perf-tools-next (branch name is the same).
>
> Is that in the MAINTAINERS file? I could not see it.

No it's not.  But it seems acme/perf is not there either.
Probably we need to add one and split the tooling part.

>
> And I was hoping that Ian could first have a look, since this is just an
> RFC.

Ok, makes sense.

Thanks,
Namhyung