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

[Ian Rogers]

On Wed, Sep 9, 2020 at 1:33 AM Jin, Yao <yao.jin@xxxxxxxxxxxxxxx> wrote:
>
>
>
> On 9/9/2020 4:02 PM, Ian Rogers 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
> >
> > 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 | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index 8831b964288f..d216a161f41c 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;
> >               if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> >                       if (has_constraint) {
> >                               /*
> > @@ -248,6 +260,16 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >               ev = metric_events[i];
> >               ev->metric_leader = ev;
> >               set_bit(ev->idx, evlist_used);
> > +             /*
> > +              * Mark two events with identical names in the same group as
> > +              * being in use as uncore events may be duplicated for each pmu.
> > +              */
> > +             evlist__for_each_entry(perf_evlist, ev) {
> > +                     if (metric_events[i]->leader == ev->leader &&
> > +                         !strcmp(metric_events[i]->name, ev->name)) {
>
> Do we really need this '!strcmp(metric_events[i]->name, ev->name)'?

When there are multiple metrics being computed then we end up parsing
multiple groups of events, something like
"{...}:W,{....}:W,{....}:W...". The code sorts the groups so that
those with the largest number of events appear at the front, it then
matches metrics against the list trying to reuse events at the front
of the list first (the selection must respect the groupings). It's not
a particularly sophisticated algorithm, but it reduces multiplexing
for say top down metrics. We need to remove the unused events/groups
from the evlist to achieve this benefit. If there were some events
like "{instructions,cycles}:W" but we only matched "instructions"
within the group, then we would want to eliminate "cycles" from the
group - the strcmp achieves this, otherwise we'd mark everything in
the group as in use. Now, the "cycles" will likely be marked live by
another metric, that's how we got the group in the first place, but
marking it live here strikes me as a little odd as the event wouldn't
be being used by the metric. A case where this problem could happen is
with --metric-no-group, where all the events multiplex freely with
each other as they aren't grouped. We'd like to eliminate duplicated
events in the evlist, but if we mark them all live this won't happen.

Sorry for the long explanation :-) TL;DR I think it is necessary.

Thanks,
Ian

> Thanks
> Jin Yao
>
> > +                             set_bit(ev->idx, evlist_used);
> > +                     }
> > +             }
> >       }
> >
> >       return metric_events[0];
> >