Messages in this thread | | | Date | Mon, 3 Jul 2023 16:20:34 +0100 | Subject | Re: [PATCH RFC 3/9] perf metrics: Pass cpu and sys tables to metricgroup__add_metric() | From | 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 >>
| |