On 1/8/2020 2:58 PM, Kajol Jain wrote:
Commit f01642e4912b ("perf metricgroup: Support multiple
events for metricgroup") introduced support for multiple events
in a metric group. But with the current upstream, metric events
names are not printed properly incase we try to run multiple
metric groups with overlapping event.
With current upstream version, incase of overlapping metric events
issue is, we always start our comparision logic from start.
So, the events which already matched with some metric group also
take part in comparision logic. Because of that when we have overlapping
events, we end up matching current metric group event with already matched
one.
For example, in skylake machine we have metric event CoreIPC and
Instructions. Both of them need 'inst_retired.any' event value.
As events in Instructions is subset of events in CoreIPC, they
endup in pointing to same 'inst_retired.any' value.
In skylake platform:
command:# ./perf stat -M CoreIPC,Instructions -C 0 sleep 1
 Performance counter stats for 'CPU(s) 0':
ÂÂÂÂÂ 1,254,992,790ÂÂÂÂÂ inst_retired.anyÂÂÂÂÂÂÂÂÂ # 1254992790.0
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Instructions
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ #ÂÂÂÂÂ 1.3 CoreIPC
ÂÂÂÂÂÂÂ 977,172,805ÂÂÂÂÂ cycles
ÂÂÂÂÂ 1,254,992,756ÂÂÂÂÂ inst_retired.any
ÂÂÂÂÂÂÂ 1.000802596 seconds time elapsed
command:# sudo ./perf stat -M UPI,IPC sleep 1
ÂÂÂ Performance counter stats for 'sleep 1':
ÂÂÂÂÂÂÂÂÂÂÂ 948,650ÂÂÂÂÂ uops_retired.retire_slots
ÂÂÂÂÂÂÂÂÂÂÂ 866,182ÂÂÂÂÂ inst_retired.anyÂÂÂÂÂÂÂÂÂ #ÂÂÂÂÂ 0.7 IPC
ÂÂÂÂÂÂÂÂÂÂÂ 866,182ÂÂÂÂÂ inst_retired.any
ÂÂÂÂÂÂÂÂÂ 1,175,671ÂÂÂÂÂ cpu_clk_unhalted.thread
Patch fixes the issue by adding a static variable 'iterator_perf_evlist'
to keep track of events which already matched with some group. It points
to event in perf_evlist from where next match should start. Because we
need to make sure, we match correct set of events belongs to
corresponding metric group.
With this patch:
In skylake platform:
command:# ./perf stat -M CoreIPC,Instructions -C 0 sleep 1
 Performance counter stats for 'CPU(s) 0':
ÂÂÂÂÂÂÂ 149,481,533ÂÂÂÂÂ inst_retired.anyÂÂÂÂÂÂÂÂÂ #ÂÂÂÂÂ 0.8 CoreIPC
ÂÂÂÂÂÂÂ 186,244,218ÂÂÂÂÂ cycles
ÂÂÂÂÂÂÂ 149,479,362ÂÂÂÂÂ inst_retired.anyÂÂÂÂÂÂÂÂÂ # 149479362.0
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ Instructions
ÂÂÂÂÂÂÂ 1.001655885 seconds time elapsed
command:# ./perf stat -M UPI,IPC sleep 1
 Performance counter stats for 'CPU(s) 0':
ÂÂÂÂÂÂÂÂ 16,858,849ÂÂÂÂÂ uops_retired.retire_slots #ÂÂÂÂÂ 1.3 UPI
ÂÂÂÂÂÂÂÂ 12,529,178ÂÂÂÂÂ inst_retired.any
ÂÂÂÂÂÂÂÂ 12,529,558ÂÂÂÂÂ inst_retired.anyÂÂÂÂÂÂÂÂÂ #ÂÂÂÂÂ 0.3 IPC
ÂÂÂÂÂÂÂÂ 39,936,071ÂÂÂÂÂ cpu_clk_unhalted.thread
ÂÂÂÂÂÂÂ 1.001413978 seconds time elapsed
Signed-off-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
Cc: Jiri Olsa <jolsa@xxxxxxxxxx>
Cc: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
Cc: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
Cc: Madhavan Srinivasan <maddy@xxxxxxxxxxxxxxxxxx>
Cc: Anju T Sudhakar <anju@xxxxxxxxxxxxxxxxxx>
Cc: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
---
 tools/perf/util/metricgroup.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 35e151b8359b..58889b0496fb 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -90,16 +90,21 @@ struct egroup {
ÂÂÂÂÂ const char *metric_unit;
 };
 +static int iterator_perf_evlist;
+
 static struct evsel *find_evsel_group(struct evlist *perf_evlist,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const char **ids,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ int idnum,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ struct evsel **metric_events)
 {
ÂÂÂÂÂ struct evsel *ev;
-ÂÂÂ int i = 0;
+ÂÂÂ int i = 0, j = 0;
ÂÂÂÂÂ bool leader_found;
 Â evlist__for_each_entry (perf_evlist, ev) {
+ÂÂÂÂÂÂÂ j++;
+ÂÂÂÂÂÂÂ if (j <= iterator_perf_evlist)
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
ÂÂÂÂÂÂÂÂÂ if (!strcmp(ev->name, ids[i])) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!metric_events[i])
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ metric_events[i] = ev;
@@ -146,6 +151,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
ÂÂÂÂÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂÂÂÂÂ }
ÂÂÂÂÂ }
+ÂÂÂ iterator_perf_evlist = j;
 Â return metric_events[0];
 }
Thanks for reporting and fixing this issue.
I just have one question, do we really need a *static variable* to track the matched events? Perhaps using an input parameter?
Thanks
Jin Yao