Re: [PATCH v2] tools/perf/metricgroup: Fix printing event names of metric group with multiple events incase of overlapping events
From: kajoljain
Date: Wed Jan 29 2020 - 01:40:44 EST
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@xxxxxxxxxxxxxxxxxx>
Cc: Anju T Sudhakar <anju@xxxxxxxxxxxxxxxxxx>
Cc: Ravi Bangoria <ravi.bangoria@xxxxxxxxxxxxx>
---
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);