Re: [PATCH v2 3/3] perf metricgroup: Fix uncore metric expressions

[Ian Rogers]

On Wed, Sep 9, 2020 at 10:51 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irogers@xxxxxxxxxx> wrote:
> >
> > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> > and uncore_imc/case_count_write/. These events open 6 events per socket
> > with pmu names of uncore_imc_[0-5]. The current metric setup code in
> > find_evsel_group assumes one ID will map to 1 event to be recorded in
> > metric_events. For events with multiple matches, the first event is
> > recorded in metric_events (avoiding matching >1 event with the same
> > name) and the evlist_used updated so that duplicate events aren't
> > removed when the evlist has unused events removed.
> >
> > Before this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >              41.14 MiB  uncore_imc/cas_count_read/
> >      1,002,614,251 ns   duration_time
> >
> >        1.002614251 seconds time elapsed
> >
> > After this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> >  Performance counter stats for 'system wide':
> >
> >             157.47 MiB  uncore_imc/cas_count_read/ #     0.00 DRAM_BW_Use
> >             126.97 MiB  uncore_imc/cas_count_write/
> >      1,003,019,728 ns   duration_time
> >
> > v2. avoids iterating over the whole evlist as suggested by
> >     namhyung@xxxxxxxxxx. It also fixes the metric_leader computation
> >     that was broken in the same commits.
> >
> > Erroneous duplication introduced in:
> > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> >
> > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > Reported-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ian Rogers <irogers@xxxxxxxxxx>
> > ---
> >  tools/perf/util/metricgroup.c | 45 ++++++++++++++++++++++++++++++++---
> >  1 file changed, 42 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 662f4e8777d5..79080de9217d 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -206,6 +206,18 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >                                 sizeof(struct evsel *) * idnum);
> >                         current_leader = ev->leader;
> >                 }
> > +               /*
> > +                * Check for duplicate events with the same name. For example,
> > +                * uncore_imc/cas_count_read/ will turn into 6 events per socket
> > +                * on skylakex. Only the first such event is placed in
> > +                * metric_events.
> > +                */
> > +               for (i = 0; i < matched_events; i++) {
> > +                       if (!strcmp(metric_events[i]->name, ev->name))
> > +                               break;
> > +               }
> > +               if (i != matched_events)
> > +                       continue;
>
> We have the same logic in the below.  Maybe it'd better to factor out..

You're right, actually with this loop the below is redundant.

>
> >                 if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >                         if (has_constraint) {
> >                                 /*
> > @@ -245,9 +257,36 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >         metric_events[idnum] = NULL;
> >
> >         for (i = 0; i < idnum; i++) {
> > -               ev = metric_events[i];
> > -               ev->metric_leader = ev;
> > -               set_bit(ev->idx, evlist_used);
> > +               /* Don't free used events. */
> > +               set_bit(metric_events[i]->idx, evlist_used);
> > +               /*
> > +                * The metric leader points to the identically named event in
> > +                * metric_events.
> > +                */
> > +               metric_events[i]->metric_leader = metric_events[i];
> > +               /*
> > +                * Mark two events with identical names in the same group (or
> > +                * globally) as being in use as uncore events may be duplicated
> > +                * for each pmu. Set the metric leader to be the event that
> > +                * appears in metric_events.
> > +                */
>
> I thought this again, and it's not guaranteed that the metric leader is
> a group leader so below won't work IMHO.  Instead we should iterate
> evlist always, but started from the metric leader with the
> evlist__for_each_entry_continue.

Thanks for pointing out evlist__for_each_entry_continue! For the
constraint case it avoids iterating a bunch of list elements. For the
sibling group case we do know that all the metric's events are
siblings with the exception of duration time that is pulled out
specially and not at risk of being aliased.

Thanks,
Ian

> Thanks
> Namhyung
>
>
> > +               if (!has_constraint) {
> > +                       for_each_group_evsel(ev, metric_events[i]->leader) {
> > +                               if (ev != metric_events[i] &&
> > +                                   !strcmp(metric_events[i]->name, ev->name)) {
> > +                                       set_bit(ev->idx, evlist_used);
> > +                                       ev->metric_leader = metric_events[i];
> > +                               }
> > +                       }
> > +               } else {
> > +                       evlist__for_each_entry(perf_evlist, ev) {
> > +                               if (ev != metric_events[i] &&
> > +                                   !strcmp(metric_events[i]->name, ev->name)) {
> > +                                       set_bit(ev->idx, evlist_used);
> > +                                       ev->metric_leader = metric_events[i];
> > +                               }
> > +                       }
> > +               }
> >         }
> >
> >         return metric_events[0];
> > --
> > 2.28.0.526.ge36021eeef-goog
> >