tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events

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 <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Kan Liang <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Jin Yao <[email protected]>

Cc Madhavan Srinivasan <[email protected]>

Cc Anju T Sudhakar <[email protected]>

Cc Ravi Bangoria <[email protected]>

---
  tools/perf/util/metricgroup.c | 8 +++++++-
  1 file changed, 7 insertions(+), 1 deletion(-)

tools/perf/util/metricgroup.c +6 -1

index 35e151b8359b..58889b0496fb 100644
@@ -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;

Quoted 113 lines · Kajol Jain

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 <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Andi Kleen <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Kan Liang <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Jin Yao <[email protected]>

Cc Madhavan Srinivasan <[email protected]>

Cc Anju T Sudhakar <[email protected]>

Cc Ravi Bangoria <[email protected]>

---
  tools/perf/util/metricgroup.c | 8 +++++++-

  1 file changed, 7 insertions(+), 1 deletion(-)

tools/perf/util/metricgroup.c +5 -1

index 35e151b8359b..58889b0496fb 100644
@@ -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;

tools/perf/util/metricgroup.c static struct evsel *find_evsel_group(struct · line 157

Quoted 7 lines

tools/perf/util/metricgroup.c +1 -0

@@ -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

Quoted 22 lines

Quoted 12 lines

-    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;

tools/perf/util/metricgroup.c static struct evsel *find_evsel_group(struct · line 157

Quoted 7 lines

tools/perf/util/metricgroup.c +1 -0

@@ -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.

Quoted 2 lines

I just have one question, do we really need a *static variable* to track the matched events? Perhaps using an input parameter?

Hi Jin,

The other way I come up with to solve this issue is, making change in perf_evlist itself by adding some flag in event name, to keep track of matched events.

As if we change event name itself, next time when we compare it won't matched. But in that case we need to remove those flag later. Which will increase the

complexity. If you have any suggestions, please let me know.

On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:

SNIP

Quoted 33 lines

Quoted 21 lines

Quoted 12 lines

-    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;

tools/perf/util/metricgroup.c static struct evsel *find_evsel_group(struct · line 157

Quoted 7 lines

tools/perf/util/metricgroup.c +1 -0

@@ -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.

Quoted 2 lines

I just have one question, do we really need a *static variable* to track the matched events? Perhaps using an input parameter?

Hi Jin,

The other way I come up with to solve this issue is, making change in perf_evlist itself by adding some flag in event name, to keep track of matched events.

As if we change event name itself, next time when we compare it won't matched. But in that case we need to remove those flag later. Which will increase the

Quoted 1 line

complexity. If you have any suggestions, please let me know.

we already keep evsel::cpu_iter for similar concept

so I guess we could have some iterator_perf_evlist variable in evlist.. that is if we don't find other solution (other than static varable)

thanks,
jirka

Quoted 37 lines · Jiri Olsa

On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:

SNIP

Quoted 33 lines

Quoted 21 lines

Quoted 12 lines

-    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;

tools/perf/util/metricgroup.c static struct evsel *find_evsel_group(struct · line 157

Quoted 7 lines

tools/perf/util/metricgroup.c +1 -0

@@ -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.

Quoted 2 lines

I just have one question, do we really need a *static variable* to track the matched events? Perhaps using an input parameter?

Hi Jin,

The other way I come up with to solve this issue is, making change in perf_evlist itself by adding some flag in event name, to keep track of matched events.

As if we change event name itself, next time when we compare it won't matched. But in that case we need to remove those flag later. Which will increase the

Quoted 5 lines

Quoted 1 line

complexity. If you have any suggestions, please let me know.

we already keep evsel::cpu_iter for similar concept

so I guess we could have some iterator_perf_evlist variable in evlist.. that is if we don't find other solution (other than static varable)

Hi Jiri,

         Thanks for reviewing the patch. I checked 'evsel::cpu_iter' variable, I think it added recently and I am not able to find any similar kind of variable in

         evlist. Please let me know if my understanding is fine. Do you want me to add new variable in evlist itself or there is any other way possible.

Quoted 45 lines · kajoljain

Quoted 42 lines · Jiri Olsa

On Wed, Jan 08, 2020 at 02:41:35PM +0530, kajoljain wrote:

SNIP

Quoted 29 lines

Quoted 21 lines

Quoted 12 lines

-    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;

tools/perf/util/metricgroup.c static struct evsel *find_evsel_group(struct · line 157

Quoted 7 lines

tools/perf/util/metricgroup.c +1 -0

@@ -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.

Quoted 2 lines

I just have one question, do we really need a *static variable* to track the matched events? Perhaps using an input parameter?

Hi Jin,

The other way I come up with to solve this issue is, making change in perf_evlist itself by adding some flag in event name, to keep track of matched events.

Quoted 5 lines

As if we change event name itself, next time when we compare it won't matched. But in that case we need to remove those flag later. Which will increase the

complexity. If you have any suggestions, please let me know.

we already keep evsel::cpu_iter for similar concept

so I guess we could have some iterator_perf_evlist variable in evlist.. that is if we don't find other solution (other than static varable)

Quoted 8 lines

Hi Jiri,

         Thanks for reviewing the patch. I checked 'evsel::cpu_iter' variable, I think it added recently and I am not able to find any similar kind of variable in

         evlist. Please let me know if my understanding is fine. Do you want me to add new variable in evlist itself or there is any other way possible.

please check Arnaldo's tree:
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git perf/core

jirka