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 <[email protected]>
- 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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
Hello,
On Wed, Jun 28, 2023 at 3:30 AM John Garry <[email protected]> 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 <[email protected]>
> - 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
>
>>
>> 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
On Wed, Jun 28, 2023 at 3:30 AM John Garry <[email protected]> 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 <[email protected]>
Acked-by: Ian Rogers <[email protected]>
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
>
On Wed, Jun 28, 2023 at 3:30 AM John Garry <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Wed, Jun 28, 2023 at 3:30 AM John Garry <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Fri, Jun 30, 2023 at 2:35 AM John Garry <[email protected]> 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
On 30/06/2023 18:41, Ian Rogers wrote:
> On Wed, Jun 28, 2023 at 3:30 AM John Garry<[email protected]> 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<[email protected]>
>> ---
>> 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
>
On 30/06/2023 19:39, Ian Rogers wrote:
> On Wed, Jun 28, 2023 at 3:30 AM John Garry <[email protected]> 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 <[email protected]>
>> ---
>> 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
>>
On Mon, Jul 3, 2023 at 6:09 AM John Garry <[email protected]> wrote:
>
> On 30/06/2023 18:41, Ian Rogers wrote:
> > On Wed, Jun 28, 2023 at 3:30 AM John Garry<[email protected]> 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<[email protected]>
> >> ---
> >> 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
>
> >
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