| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <c167a8db-d644-3f65-1bde-4635853cab92@oracle.com>
Date: Mon, 3 Jul 2023 16:20:34 +0100
From: John Garry <john.g.garry@...cle.com>
To: Ian Rogers <irogers@...gle.com>
Cc: acme@...nel.org, namhyung@...nel.org, jolsa@...nel.org,
linux-arm-kernel@...ts.infradead.org,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
renyu.zj@...ux.alibaba.com, shangxiaojing@...wei.com,
kjain@...ux.ibm.com, kan.liang@...ux.intel.com
Subject: Re: [PATCH RFC 3/9] perf metrics: Pass cpu and sys tables to
metricgroup__add_metric()
On 30/06/2023 19:39, Ian Rogers wrote:
> On Wed, Jun 28, 2023 at 3:30 AM John Garry <john.g.garry@...cle.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@...cle.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
>>
Powered by blists - more mailing lists