On Tue, Oct 6, 2020 at 7:22 AM John Garry <john.garry@xxxxxxxxxx> wrote:
>
> On 05/10/2020 19:05, John Garry wrote:
> >> Can you provide a reproduction? Looking on broadwell
> >> this metric doesn't exist.
> >
> > Right, I just added this test metric as my 2x x86 platform has no
> > examples which I can find:
> >
> > diff --git a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > index 8cdc7c13dc2a..fc6d9adf996a 100644
> > --- a/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > +++ b/tools/perf/pmu-events/arch/x86/broadwell/bdw-metrics.json
> > @@ -348,5 +348,11 @@
> > "MetricExpr": "(cstate_pkg@c7\\-residency@ / msr@tsc@) * 100",
> > "MetricGroup": "Power",
> > "MetricName": "C7_Pkg_Residency"
> > + },
> > + {
> > + "BriefDescription": "test metric",
> > + "MetricExpr": "UNC_CBO_XSNP_RESPONSE.MISS_XCORE *
> > UNC_CBO_XSNP_RESPONSE.MISS_EVICTION",
> > + "MetricGroup": "Test",
> > + "MetricName": "test_metric_inc"
> > }
> > ]
> >
>
> It seems that the code in find_evsel_group() does not properly handle
> the scenario of event alias matching different PMUs (as I already said).
>
> So I got it working on top of "perf metricgroup: Fix uncore metric
> expressions" with the following change:
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index d948a7f910cf..6293378c019c 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -213,7 +213,8 @@ static struct evsel *find_evsel_group(struct evlist
> *perf_evlist,
> /* Ignore event if already used and merging is disabled. */
> if (metric_no_merge && test_bit(ev->idx, evlist_used))
> continue;
> - if (!has_constraint && ev->leader != current_leader) {
> + if (!has_constraint && (!current_leader ||
> strcmp(current_leader->name, ev->leader->name))) {
> /*
> * Start of a new group, discard the whole match and
> * start again.
> @@ -279,7 +280,8 @@ static struct evsel *find_evsel_group(struct evlist
> *perf_evlist,
> * when then group is left.
> */
> if (!has_constraint &&
> - ev->leader != metric_events[i]->leader)
> + strcmp(ev->leader->name, metric_events[i]->leader->name))
> break;
> if (!strcmp(metric_events[i]->name, ev->name)) {
> set_bit(ev->idx, evlist_used);
>
> which gives for my test metric:
>
> ./perf stat -v -M test_metric_inc sleep 1
> Using CPUID GenuineIntel-6-3D-4
> metric expr unc_cbo_xsnp_response.miss_xcore /
> unc_cbo_xsnp_response.miss_eviction for test_metric_inc
> found event unc_cbo_xsnp_response.miss_eviction
> found event unc_cbo_xsnp_response.miss_xcore
> adding
> {unc_cbo_xsnp_response.miss_eviction,unc_cbo_xsnp_response.miss_xcore}:W
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_1/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_eviction -> uncore_cbox_0/umask=0x81,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_1/umask=0x41,event=0x22/
> unc_cbo_xsnp_response.miss_xcore -> uncore_cbox_0/umask=0x41,event=0x22/
> Control descriptor is not initialized
> unc_cbo_xsnp_response.miss_eviction: 595175 1001021311 1001021311
> unc_cbo_xsnp_response.miss_eviction: 592516 1001020037 1001020037
> unc_cbo_xsnp_response.miss_xcore: 39139 1001021311 1001021311
> unc_cbo_xsnp_response.miss_xcore: 38718 1001020037 1001020037
>
> Performance counter stats for 'system wide':
>
> 1,187,691 unc_cbo_xsnp_response.miss_eviction # 0.07
> test_metric_inc
> 77,857 unc_cbo_xsnp_response.miss_xcore
>
>
> 1.001068918 seconds time elapsed
>
> John
Thanks John! I was able to repro the problem, let me investigate what
is happening here as it seems there may be something wrong with the
grouping logic.
Ian
