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

[John Garry <john.g.garry@oracle.com>]

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