Re: [RFC PATCH v5 02/16] perf stat: Add basic functions for the hardware aware grouping

[Ian Rogers]

On Fri, Apr 12, 2024 at 2:08 PM <weilin.wang@xxxxxxxxx> wrote:
>
> From: Weilin Wang <weilin.wang@xxxxxxxxx>
>
> Add the first set of functions for the hardware aware grouping method. Function
> hw_aware_parse_groups() is the entry point of this metric grouping method  It
> does metric grouping on a combined list of events and will create a list of
> grouping strings as final results of the grouping method. These grouping strings
> will be used in the same manner as existing metric grouping process.
>
> This method will fall back to normal grouping when hardware aware grouping
> return with err so that perf stat still executes and returns with correct
> result.
>
> Signed-off-by: Weilin Wang <weilin.wang@xxxxxxxxx>

Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>

> ---
>  tools/perf/util/metricgroup.c | 217 +++++++++++++++++++++++++++++++++-
>  1 file changed, 216 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 11613450725a..8047f03b2b1f 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -159,6 +159,14 @@ struct metric {
>         struct evlist *evlist;
>  };
>
> +/**
> + * Each group is one node in the group string list.
> + */
> +struct metricgroup__group_strs {
> +       struct list_head nd;
> +       struct strbuf grouping_str;
> +};
> +
>  static void metric__watchdog_constraint_hint(const char *name, bool foot)
>  {
>         static bool violate_nmi_constraint;
> @@ -1432,6 +1440,101 @@ static int build_combined_expr_ctx(const struct list_head *metric_list,
>         return ret;
>  }
>
> +/**
> + * hw_aware_build_grouping - Build event groupings by reading counter
> + * requirement of the events and counter available on the system from
> + * pmu-events.
> + * @ctx: the event identifiers parsed from metrics.
> + * @groupings: header to the list of final event grouping.
> + * @modifier: any modifiers added to the events.
> + */
> +static int hw_aware_build_grouping(struct expr_parse_ctx *ctx __maybe_unused,
> +                                 struct list_head *groupings __maybe_unused,
> +                                 const char *modifier __maybe_unused)
> +{
> +       int ret = 0;
> +
> +       pr_debug("This is a placeholder\n");
> +       return ret;
> +}
> +
> +static void group_str_free(struct metricgroup__group_strs *g)
> +{
> +       if (!g)
> +               return;
> +
> +       strbuf_release(&g->grouping_str);
> +       free(g);
> +}
> +
> +static void metricgroup__free_grouping_strs(struct list_head
> +                                          *grouping_strs)
> +{
> +       struct metricgroup__group_strs *g, *tmp;
> +
> +       list_for_each_entry_safe(g, tmp, grouping_strs, nd) {
> +               list_del_init(&g->nd);
> +               group_str_free(g);
> +       }
> +}
> +
> +/**
> + * hw_aware_parse_ids - Build the event string for the ids and parse them
> + * creating an evlist. The encoded metric_ids are decoded. Events are placed
> + * into groups based on event counter requirements and counter availabilities of
> + * the system.
> + * @metric_no_merge: is metric sharing explicitly disabled.
> + * @fake_pmu: used when testing metrics not supported by the current CPU.
> + * @ids: the event identifiers parsed from a metric.
> + * @modifier: any modifiers added to the events.
> + * @out_evlist: the created list of events.
> + */
> +static int hw_aware_parse_ids(struct perf_pmu *fake_pmu,
> +                            struct expr_parse_ctx *ids, const char *modifier,
> +                            struct evlist **out_evlist)
> +{
> +       struct parse_events_error parse_error;
> +       struct evlist *parsed_evlist;
> +       LIST_HEAD(groupings);
> +       struct metricgroup__group_strs *group;
> +       int ret;
> +
> +       *out_evlist = NULL;
> +       ret = hw_aware_build_grouping(ids, &groupings, modifier);
> +       if (ret) {
> +               metricgroup__free_grouping_strs(&groupings);
> +               return ret;
> +       }
> +
> +       parsed_evlist = evlist__new();
> +       if (!parsed_evlist) {
> +               ret = -ENOMEM;
> +               goto err_out;
> +       }
> +       list_for_each_entry(group, &groupings, nd) {
> +               struct strbuf *events = &group->grouping_str;
> +
> +               pr_debug("Parsing metric events '%s'\n", events->buf);
> +               parse_events_error__init(&parse_error);
> +               ret = __parse_events(parsed_evlist, events->buf, /*pmu_filter=*/NULL,
> +                                   &parse_error, fake_pmu, /*warn_if_reordered=*/false);
> +               if (ret) {
> +                       parse_events_error__print(&parse_error, events->buf);
> +                       goto err_out;
> +               }
> +               ret = decode_all_metric_ids(parsed_evlist, modifier);
> +               if (ret)
> +                       goto err_out;
> +       }
> +       *out_evlist = parsed_evlist;
> +       parsed_evlist = NULL;
> +err_out:
> +       parse_events_error__exit(&parse_error);
> +       evlist__delete(parsed_evlist);
> +       metricgroup__free_grouping_strs(&groupings);
> +       return ret;
> +}
> +
>  /**
>   * parse_ids - Build the event string for the ids and parse them creating an
>   *             evlist. The encoded metric_ids are decoded.
> @@ -1520,6 +1623,113 @@ static int parse_ids(bool metric_no_merge, struct perf_pmu *fake_pmu,
>         return ret;
>  }
>
> +static int hw_aware_parse_groups(struct evlist *perf_evlist,
> +                               const char *pmu, const char *str,
> +                               bool metric_no_threshold,
> +                               const char *user_requested_cpu_list,
> +                               bool system_wide,
> +                               struct perf_pmu *fake_pmu,
> +                               struct rblist *metric_events_list,
> +                               const struct pmu_metrics_table *table)
> +{
> +       struct evlist *combined_evlist = NULL;
> +       LIST_HEAD(metric_list);
> +       struct metric *m;
> +       int ret;
> +       bool metric_no_group = false;
> +       bool metric_no_merge = false;
> +
> +       if (metric_events_list->nr_entries == 0)
> +               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);
> +       if (ret)
> +               goto out;
> +
> +       /* Sort metrics from largest to smallest. */
> +       list_sort(NULL, &metric_list, metric_list_cmp);
> +
> +       if (!metric_no_merge) {
> +               struct expr_parse_ctx *combined = NULL;
> +
> +               ret = build_combined_expr_ctx(&metric_list, &combined);
> +
> +               if (!ret && combined && hashmap__size(combined->ids)) {
> +                       ret = hw_aware_parse_ids(fake_pmu, combined,
> +                                               /*modifier=*/NULL,
> +                                               &combined_evlist);
> +               }
> +
> +               if (combined)
> +                       expr__ctx_free(combined);
> +               if (ret)
> +                       goto out;
> +       }
> +
> +       list_for_each_entry(m, &metric_list, nd) {
> +               struct metric_expr *expr;
> +               struct metric_event *me;
> +               struct evsel **metric_events;
> +
> +               ret = setup_metric_events(fake_pmu ? "all" : m->pmu, m->pctx->ids,
> +                                        combined_evlist, &metric_events);
> +               if (ret) {
> +                       pr_debug("Cannot resolve IDs for %s: %s\n",
> +                               m->metric_name, m->metric_expr);
> +                       goto out;
> +               }
> +
> +               me = metricgroup__lookup(metric_events_list, metric_events[0], true);
> +
> +               expr = malloc(sizeof(struct metric_expr));
> +               if (!expr) {
> +                       ret = -ENOMEM;
> +                       free(metric_events);
> +                       goto out;
> +               }
> +
> +               expr->metric_refs = m->metric_refs;
> +               m->metric_refs = NULL;
> +               expr->metric_expr = m->metric_expr;
> +               if (m->modifier) {
> +                       char *tmp;
> +
> +                       if (asprintf(&tmp, "%s:%s", m->metric_name, m->modifier) < 0)
> +                               expr->metric_name = NULL;
> +                       else
> +                               expr->metric_name = tmp;
> +               } else {
> +                       expr->metric_name = strdup(m->metric_name);
> +               }
> +
> +               if (!expr->metric_name) {
> +                       ret = -ENOMEM;
> +                       free(metric_events);
> +                       goto out;
> +               }
> +               expr->metric_threshold = m->metric_threshold;
> +               expr->metric_unit = m->metric_unit;
> +               expr->metric_events = metric_events;
> +               expr->runtime = m->pctx->sctx.runtime;
> +               list_add(&expr->nd, &me->head);
> +       }
> +
> +       if (combined_evlist) {
> +               evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
> +               evlist__delete(combined_evlist);
> +       }
> +
> +       list_for_each_entry(m, &metric_list, nd) {
> +               if (m->evlist)
> +                       evlist__splice_list_tail(perf_evlist, &m->evlist->core.entries);
> +       }
> +
> +out:
> +       metricgroup__free_metrics(&metric_list);
> +       return ret;
> +}
> +
>  static int parse_groups(struct evlist *perf_evlist,
>                         const char *pmu, const char *str,
>                         bool metric_no_group,
> @@ -1698,10 +1908,15 @@ int metricgroup__parse_groups(struct evlist *perf_evlist,
>         if (!table)
>                 return -EINVAL;
>         if (hardware_aware_grouping) {
> +               int ret;

nit: there should be a '\n' between variables and code,
scripts/checkpatch.pl sould catch this.

>                 pr_debug("Use hardware aware grouping instead of traditional metric grouping method\n");
> +               ret = hw_aware_parse_groups(perf_evlist, pmu, str,
> +                           metric_no_threshold, user_requested_cpu_list, system_wide,
> +                           /*fake_pmu=*/NULL, metric_events, table);
> +               if (!ret)
> +                       return 0;
>         }
>
> -

nit: unneeded whitespace change.

Thanks,
Ian

>         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);
> --
> 2.42.0
>