On Thu, Sep 10, 2020 at 12:26 PM Ian Rogers <irog...@google.com> 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 > namhy...@kernel.org. 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....@linux.intel.com> > Signed-off-by: Ian Rogers <irog...@google.com> > --- > 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.. > 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 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 >