* [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
@ 2020-01-22 6:47 Kajol Jain
2020-01-27 15:41 ` Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: Kajol Jain @ 2020-01-22 6:47 UTC (permalink / raw)
To: acme
Cc: linux-kernel, linux-perf-users, kjain, Alexander Shishkin,
Andi Kleen, Jiri Olsa, Kan Liang, Peter Zijlstra, Jin Yao,
Madhavan Srinivasan, Anju T Sudhakar, Ravi Bangoria
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 variable 'evlist_iter' in evlist
itself 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@linux.ibm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Jin Yao <yao.jin@linux.intel.com>
Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
tools/perf/util/evlist.h | 1 +
tools/perf/util/metricgroup.c | 14 +++++++++++++-
2 files changed, 14 insertions(+), 1 deletion(-)
---
Changelog:
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/evlist.h b/tools/perf/util/evlist.h
index f5bd5c386df1..255f872aee92 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -54,6 +54,7 @@ struct evlist {
bool enabled;
int id_pos;
int is_pos;
+ int evlist_iter;
u64 combined_sample_type;
enum bkw_mmap_state bkw_mmap_state;
struct {
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 02aee946b6c1..911fab4ac04b 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -96,10 +96,13 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
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 <= perf_evlist->evlist_iter)
+ continue;
if (!strcmp(ev->name, ids[i])) {
if (!metric_events[i])
metric_events[i] = ev;
@@ -147,6 +150,8 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
}
}
+ perf_evlist->evlist_iter = j;
+
return metric_events[0];
}
@@ -161,6 +166,13 @@ static int metricgroup__setup_events(struct list_head *groups,
struct egroup *eg;
struct evsel *evsel;
+ /*
+ * Reset the evlist_iter. This is needed because
+ * when we start comparision logic in find_evsel_group(),
+ * need to start it from very first event present in evlist.
+ */
+ perf_evlist->evlist_iter = 0;
+
list_for_each_entry (eg, groups, nd) {
struct evsel **metric_events;
--
2.21.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-22 6:47 [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events Kajol Jain
@ 2020-01-27 15:41 ` Jiri Olsa
2020-01-29 6:40 ` kajoljain
0 siblings, 1 reply; 4+ messages in thread
From: Jiri Olsa @ 2020-01-27 15:41 UTC (permalink / raw)
To: Kajol Jain
Cc: acme, linux-kernel, linux-perf-users, Alexander Shishkin,
Andi Kleen, Jiri Olsa, Kan Liang, Peter Zijlstra, Jin Yao,
Madhavan Srinivasan, Anju T Sudhakar, Ravi Bangoria
On Wed, Jan 22, 2020 at 12:17:21PM +0530, Kajol Jain wrote:
SNIP
> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
> ---
> tools/perf/util/evlist.h | 1 +
> tools/perf/util/metricgroup.c | 14 +++++++++++++-
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
> ---
> Changelog:
> 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/evlist.h b/tools/perf/util/evlist.h
> index f5bd5c386df1..255f872aee92 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -54,6 +54,7 @@ struct evlist {
> bool enabled;
> int id_pos;
> int is_pos;
> + int evlist_iter;
> u64 combined_sample_type;
> enum bkw_mmap_state bkw_mmap_state;
> struct {
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..911fab4ac04b 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -96,10 +96,13 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> 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 <= perf_evlist->evlist_iter)
> + continue;
hm, but that won't work when event does not match and all
is rolled over in th next condition, no?
how about something like below.. I only checked I got same
results as you did in the changelog, but haven't tested
otherwise.. especially I think that the check I removed
is redundant
could you please also add test for this testcase?
thanks,
jirka
---
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 02aee946b6c1..c12f3efccec8 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 used[])
{
struct evsel *ev;
- int i = 0;
+ int i = 0, j = 0;
bool leader_found;
evlist__for_each_entry (perf_evlist, ev) {
+ if (used[j++])
+ continue;
if (!strcmp(ev->name, ids[i])) {
if (!metric_events[i])
metric_events[i] = ev;
@@ -107,22 +110,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
if (i == idnum)
break;
} else {
- if (i + 1 == idnum) {
+ if (i) {
/* 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;
}
}
}
@@ -144,9 +136,11 @@ 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];
+ used[ev->idx] = true;
}
-
return metric_events[0];
}
@@ -160,6 +154,9 @@ static int metricgroup__setup_events(struct list_head *groups,
int ret = 0;
struct egroup *eg;
struct evsel *evsel;
+ bool used[perf_evlist->core.nr_entries];
+
+ memset(used, 0, perf_evlist->core.nr_entries);
list_for_each_entry (eg, groups, nd) {
struct evsel **metric_events;
@@ -170,7 +167,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, used);
if (!evsel) {
pr_debug("Cannot resolve %s: %s\n",
eg->metric_name, eg->metric_expr);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-27 15:41 ` Jiri Olsa
@ 2020-01-29 6:40 ` kajoljain
2020-01-29 8:00 ` Jiri Olsa
0 siblings, 1 reply; 4+ messages in thread
From: kajoljain @ 2020-01-29 6:40 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, linux-kernel, linux-perf-users, Alexander Shishkin,
Andi Kleen, Jiri Olsa, Kan Liang, Peter Zijlstra, Jin Yao,
Madhavan Srinivasan, Anju T Sudhakar, Ravi Bangoria
On 1/27/20 9:11 PM, Jiri Olsa wrote:
> On Wed, Jan 22, 2020 at 12:17:21PM +0530, Kajol Jain wrote:
>
> SNIP
>
>> Cc: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> Cc: Anju T Sudhakar <anju@linux.vnet.ibm.com>
>> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
>> ---
>> tools/perf/util/evlist.h | 1 +
>> tools/perf/util/metricgroup.c | 14 +++++++++++++-
>> 2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> ---
>> Changelog:
>> 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/evlist.h b/tools/perf/util/evlist.h
>> index f5bd5c386df1..255f872aee92 100644
>> --- a/tools/perf/util/evlist.h
>> +++ b/tools/perf/util/evlist.h
>> @@ -54,6 +54,7 @@ struct evlist {
>> bool enabled;
>> int id_pos;
>> int is_pos;
>> + int evlist_iter;
>> u64 combined_sample_type;
>> enum bkw_mmap_state bkw_mmap_state;
>> struct {
>> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> index 02aee946b6c1..911fab4ac04b 100644
>> --- a/tools/perf/util/metricgroup.c
>> +++ b/tools/perf/util/metricgroup.c
>> @@ -96,10 +96,13 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>> 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 <= perf_evlist->evlist_iter)
>> + continue;
> hm, but that won't work when event does not match and all
> is rolled over in th next condition, no?
Hi jiri,
Thanks for the review. Yes you are right. Need to take care of that
case.
>
> how about something like below.. I only checked I got same
> results as you did in the changelog, but haven't tested
> otherwise.. especially I think that the check I removed
> is redundant
>
> could you please also add test for this testcase?
>
> thanks,
> jirka
>
>
> ---
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 02aee946b6c1..c12f3efccec8 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 used[])
> {
> struct evsel *ev;
> - int i = 0;
> + int i = 0, j = 0;
> bool leader_found;
>
> evlist__for_each_entry (perf_evlist, ev) {
> + if (used[j++])
> + continue;
> if (!strcmp(ev->name, ids[i])) {
> if (!metric_events[i])
> metric_events[i] = ev;
> @@ -107,22 +110,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> if (i == idnum)
> break;
> } else {
> - if (i + 1 == idnum) {
> + if (i) {
> /* 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;
> }
Incase we have match miss, we need to restart the match comparison again.
But I think we can't totally remove this check as, we also need to make
sure we take current event in match logic. Maybe something like this:
} 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;
}
Please let me know if it sounds fine.
Thanks,
Kajol
> }
> }
> @@ -144,9 +136,11 @@ 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];
> + used[ev->idx] = true;
> }
> -
> return metric_events[0];
> }
>
> @@ -160,6 +154,9 @@ static int metricgroup__setup_events(struct list_head *groups,
> int ret = 0;
> struct egroup *eg;
> struct evsel *evsel;
> + bool used[perf_evlist->core.nr_entries];
> +
> + memset(used, 0, perf_evlist->core.nr_entries);
>
> list_for_each_entry (eg, groups, nd) {
> struct evsel **metric_events;
> @@ -170,7 +167,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, used);
> if (!evsel) {
> pr_debug("Cannot resolve %s: %s\n",
> eg->metric_name, eg->metric_expr);
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
2020-01-29 6:40 ` kajoljain
@ 2020-01-29 8:00 ` Jiri Olsa
0 siblings, 0 replies; 4+ messages in thread
From: Jiri Olsa @ 2020-01-29 8:00 UTC (permalink / raw)
To: kajoljain
Cc: acme, linux-kernel, linux-perf-users, Alexander Shishkin,
Andi Kleen, Jiri Olsa, Kan Liang, Peter Zijlstra, Jin Yao,
Madhavan Srinivasan, Anju T Sudhakar, Ravi Bangoria
On Wed, Jan 29, 2020 at 12:10:22PM +0530, kajoljain wrote:
SNIP
> > - i = 0;
> > - memset(metric_events, 0,
> > - sizeof(struct evsel *) * idnum);
> > - continue;
> > }
>
> Incase we have match miss, we need to restart the match comparison again.
>
> But I think we can't totally remove this check as, we also need to make sure
> we take current event in match logic. Maybe something like this:
>
> } 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;
> }
>
> Please let me know if it sounds fine.
ah right.. we need to compare the event with the first id, sounds good
thanks,
jirka
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-01-29 8:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22 6:47 [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events Kajol Jain
2020-01-27 15:41 ` Jiri Olsa
2020-01-29 6:40 ` kajoljain
2020-01-29 8:00 ` Jiri Olsa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).