Re: [PATCH v1 05/22] perf metricgroup: Add care to picking the evsel for displaying a metric

[Ian Rogers]

On Mon, Nov 3, 2025 at 8:52 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Fri, Oct 24, 2025 at 10:58:40AM -0700, Ian Rogers wrote:
> > Rather than using the first evsel in the matched events, try to find
> > the least shared non-tool evsel. The aim is to pick the first evsel
> > that typifies the metric within the list of metrics.
> >
> > This addresses an issue where Default metric group metrics may lose
> > their counter value due to how the stat displaying hides counters for
> > default event/metric output.
>
> Do you have a command line example to show impact of this change?

You can just run a Topdown metricgroup on Intel to see differences,
but they are minor. The main impact is on the Default legacy metrics
as those have a counter then a metric, but without this change you get
everything grouped on the cpu-clock event and the formatting gets
broken. As --metric-only is popular when looking at a group of events
and the Default legacy metrics are added in subsequent changes it
didn't seem right to include the output (it either shows broken things
keeping to be somewhat broken or output from later patches).

Thanks,
Ian

> Thanks,
> Namhyung
>
> >
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/metricgroup.c | 48 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 48936e517803..76092ee26761 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -1323,6 +1323,51 @@ static int parse_ids(bool metric_no_merge, bool fake_pmu,
> >       return ret;
> >  }
> >
> > +/* How many times will a given evsel be used in a set of metrics? */
> > +static int count_uses(struct list_head *metric_list, struct evsel *evsel)
> > +{
> > +     const char *metric_id = evsel__metric_id(evsel);
> > +     struct metric *m;
> > +     int uses = 0;
> > +
> > +     list_for_each_entry(m, metric_list, nd) {
> > +             if (hashmap__find(m->pctx->ids, metric_id, NULL))
> > +                     uses++;
> > +     }
> > +     return uses;
> > +}
> > +
> > +/*
> > + * Select the evsel that stat-display will use to trigger shadow/metric
> > + * printing. Pick the least shared non-tool evsel, encouraging metrics to be
> > + * with a hardware counter that is specific to them.
> > + */
> > +static struct evsel *pick_display_evsel(struct list_head *metric_list,
> > +                                     struct evsel **metric_events)
> > +{
> > +     struct evsel *selected = metric_events[0];
> > +     size_t selected_uses;
> > +     bool selected_is_tool;
> > +
> > +     if (!selected)
> > +             return NULL;
> > +
> > +     selected_uses = count_uses(metric_list, selected);
> > +     selected_is_tool = evsel__is_tool(selected);
> > +     for (int i = 1; metric_events[i]; i++) {
> > +             struct evsel *candidate = metric_events[i];
> > +             size_t candidate_uses = count_uses(metric_list, candidate);
> > +
> > +             if ((selected_is_tool && !evsel__is_tool(candidate)) ||
> > +                 (candidate_uses < selected_uses)) {
> > +                     selected = candidate;
> > +                     selected_uses = candidate_uses;
> > +                     selected_is_tool = evsel__is_tool(selected);
> > +             }
> > +     }
> > +     return selected;
> > +}
> > +
> >  static int parse_groups(struct evlist *perf_evlist,
> >                       const char *pmu, const char *str,
> >                       bool metric_no_group,
> > @@ -1430,7 +1475,8 @@ static int parse_groups(struct evlist *perf_evlist,
> >                       goto out;
> >               }
> >
> > -             me = metricgroup__lookup(&perf_evlist->metric_events, metric_events[0],
> > +             me = metricgroup__lookup(&perf_evlist->metric_events,
> > +                                      pick_display_evsel(&metric_list, metric_events),
> >                                        /*create=*/true);
> >
> >               expr = malloc(sizeof(struct metric_expr));
> > --
> > 2.51.1.821.gb6fe4d2222-goog
> >