Re: [PATCH v6] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
From: kajoljain
Date: Tue Mar 17 2020 - 02:21:47 EST
Hi Arnaldo,
Can you pull this patch if it looks fine to you. Please let
me know if any changes require.
Thanks,
Kajol
On 2/21/20 3:41 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 new bool pointer 'evlist_used' to keep
> track of events which already matched with some group by setting it true.
> So, we skip all used events in list when we start comparision logic.
> Patch also make some changes in comparision logic, incase we get a match
> miss, we discard the whole match and start again with first event id in
> metric event.
>
> With this patch:
> In skylake platform:
>
> command:# ./perf stat -M CoreIPC,Instructions -C 0 sleep 1
>
> Performance counter stats for 'CPU(s) 0':
>
> 3,348,415 inst_retired.any # 0.3 CoreIPC
> 11,779,026 cycles
> 3,348,381 inst_retired.any # 3348381.0
> Instructions
>
> 1.001649056 seconds time elapsed
>
> command:# ./perf stat -M UPI,IPC sleep 1
>
> Performance counter stats for 'sleep 1':
>
> 1,023,148 uops_retired.retire_slots # 1.1 UPI
> 924,976 inst_retired.any
> 924,976 inst_retired.any # 0.6 IPC
> 1,489,414 cpu_clk_unhalted.thread
>
> 1.003064672 seconds time elapsed
>
> Signed-off-by: Kajol Jain <kjain@xxxxxxxxxxxxx>
> Acked-by: Jiri Olsa <jolsa@xxxxxxxxxx>
>
> Cc: Alexander Shishkin <alexander.shishkin@xxxxxxxxxxxxxxx>
> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> 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 | 49 +++++++++++++++++++++--------------
> 1 file changed, 30 insertions(+), 19 deletions(-)
>
> Changelog:
> v5 -> v6
> - Remove bool cast
> - Add Acked-by tag
>
> v4 -> v5
> - Made small fix to return from function 'metricgroup__setup_events'
> in case calloc fail
>
> v3 -> v4
> - Make 'evlist_used' a bool pointer.
>
> v2 -> v3
> - Add array in place of variable to keep track of matched events.
> Because incase we miss match in previous approach, all events will
> be rolled over in next condition. So, rather we add array and set
> it incase that variable already match with some group.
> - Suggested by Jiri Olsa
>
> v1 -> v2
> - Rather then adding static variable in metricgroup.c,
> add a new variable in evlist itself with name 'evlist_iter'
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..33bb138f7902 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -93,13 +93,16 @@ struct egroup {
> static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> const char **ids,
> int idnum,
> - struct evsel **metric_events)
> + struct evsel **metric_events,
> + bool *evlist_used)
> {
> struct evsel *ev;
> - int i = 0;
> + int i = 0, j = 0;
> bool leader_found;
>
> evlist__for_each_entry (perf_evlist, ev) {
> + if (evlist_used[j++])
> + continue;
> if (!strcmp(ev->name, ids[i])) {
> if (!metric_events[i])
> metric_events[i] = ev;
> @@ -107,22 +110,17 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> if (i == idnum)
> break;
> } else {
> - if (i + 1 == idnum) {
> - /* Discard the whole match and start again */
> - i = 0;
> - memset(metric_events, 0,
> - sizeof(struct evsel *) * idnum);
> - continue;
> - }
> -
> - if (!strcmp(ev->name, ids[i]))
> - metric_events[i] = ev;
> - else {
> - /* Discard the whole match and start again */
> - i = 0;
> - memset(metric_events, 0,
> - sizeof(struct evsel *) * idnum);
> - continue;
> + /* Discard the whole match and start again */
> + i = 0;
> + memset(metric_events, 0,
> + sizeof(struct evsel *) * idnum);
> +
> + if (!strcmp(ev->name, ids[i])) {
> + if (!metric_events[i])
> + metric_events[i] = ev;
> + i++;
> + if (i == idnum)
> + break;
> }
> }
> }
> @@ -144,7 +142,10 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> !strcmp(ev->name, metric_events[i]->name)) {
> ev->metric_leader = metric_events[i];
> }
> + j++;
> }
> + ev = metric_events[i];
> + evlist_used[ev->idx] = true;
> }
>
> return metric_events[0];
> @@ -160,6 +161,13 @@ static int metricgroup__setup_events(struct list_head *groups,
> int ret = 0;
> struct egroup *eg;
> struct evsel *evsel;
> + bool *evlist_used;
> +
> + evlist_used = calloc(perf_evlist->core.nr_entries, sizeof(bool));
> + if (!evlist_used) {
> + ret = -ENOMEM;
> + return ret;
> + }
>
> list_for_each_entry (eg, groups, nd) {
> struct evsel **metric_events;
> @@ -170,7 +178,7 @@ static int metricgroup__setup_events(struct list_head *groups,
> break;
> }
> evsel = find_evsel_group(perf_evlist, eg->ids, eg->idnum,
> - metric_events);
> + metric_events, evlist_used);
> if (!evsel) {
> pr_debug("Cannot resolve %s: %s\n",
> eg->metric_name, eg->metric_expr);
> @@ -194,6 +202,9 @@ static int metricgroup__setup_events(struct list_head *groups,
> expr->metric_events = metric_events;
> list_add(&expr->nd, &me->head);
> }
> +
> + free(evlist_used);
> +
> return ret;
> }
>
>