* [PATCH v3] perf metricgroup: Fix uncore metric expressions
@ 2020-09-10 18:02 Ian Rogers
2020-09-11 3:07 ` Namhyung Kim
0 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2020-09-10 18:02 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Kajol Jain, Kan Liang, Jin Yao, Thomas Richter, linux-kernel
Cc: Stephane Eranian, Ian Rogers
A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
and uncore_imc/case_count_write/. These events open 6 events per socket
with pmu names of uncore_imc_[0-5]. The current metric setup code in
find_evsel_group assumes one ID will map to 1 event to be recorded in
metric_events. For events with multiple matches, the first event is
recorded in metric_events (avoiding matching >1 event with the same
name) and the evlist_used updated so that duplicate events aren't
removed when the evlist has unused events removed.
Before this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
Performance counter stats for 'system wide':
41.14 MiB uncore_imc/cas_count_read/
1,002,614,251 ns duration_time
1.002614251 seconds time elapsed
After this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
Performance counter stats for 'system wide':
157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
126.97 MiB uncore_imc/cas_count_write/
1,003,019,728 ns duration_time
v3. cleans up searching for the same event within metric_events to use a
helper and avoids a redundant search. It uses a continue loop to
make the search for similarly named events shorter.
v2. avoids iterating over the whole evlist as suggested by
namhyung@kernel.org. It also fixes the metric_leader computation
that was broken in the same commits.
Erroneous duplication introduced in:
commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
Reported-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
1 file changed, 56 insertions(+), 19 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 662f4e8777d5..7b66452cb9f9 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -149,6 +149,18 @@ static void expr_ids__exit(struct expr_ids *ids)
free(ids->id[i].id);
}
+static bool contains_event(struct evsel **metric_events, int num_events,
+ const char *event_name)
+{
+ int i;
+
+ for (i = 0; i < num_events; i++) {
+ if (!strcmp(metric_events[i]->name, event_name))
+ return true;
+ }
+ return false;
+}
+
/**
* Find a group of events in perf_evlist that correspond to those from a parsed
* metric expression. Note, as find_evsel_group is called in the same order as
@@ -179,7 +191,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
int i = 0, matched_events = 0, events_to_match;
const int idnum = (int)hashmap__size(&pctx->ids);
- /* duration_time is grouped separately. */
+ /*
+ * duration_time is always grouped separately, when events are grouped
+ * (ie has_constraint is false) then ignore it in the matching loop and
+ * add it to metric_events at the end.
+ */
if (!has_constraint &&
hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
events_to_match = idnum - 1;
@@ -206,23 +222,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
sizeof(struct evsel *) * idnum);
current_leader = ev->leader;
}
- if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
- if (has_constraint) {
- /*
- * Events aren't grouped, ensure the same event
- * isn't matched from two groups.
- */
- for (i = 0; i < matched_events; i++) {
- if (!strcmp(ev->name,
- metric_events[i]->name)) {
- break;
- }
- }
- if (i != matched_events)
- continue;
- }
+ /*
+ * Check for duplicate events with the same name. For example,
+ * uncore_imc/cas_count_read/ will turn into 6 events per socket
+ * on skylakex. Only the first such event is placed in
+ * metric_events. If events aren't grouped then this also
+ * ensures that the same event in different sibling groups
+ * aren't both added to metric_events.
+ */
+ if (contains_event(metric_events, matched_events, ev->name))
+ continue;
+ /* Does this event belong to the parse context? */
+ if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
metric_events[matched_events++] = ev;
- }
+
if (matched_events == events_to_match)
break;
}
@@ -238,7 +251,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
}
if (matched_events != idnum) {
- /* Not whole match */
+ /* Not a whole match */
return NULL;
}
@@ -246,8 +259,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
for (i = 0; i < idnum; i++) {
ev = metric_events[i];
- ev->metric_leader = ev;
+ /* Don't free the used events. */
set_bit(ev->idx, evlist_used);
+ /*
+ * The metric leader points to the identically named event in
+ * metric_events.
+ */
+ ev->metric_leader = ev;
+ /*
+ * Mark two events with identical names in the same group (or
+ * globally) as being in use as uncore events may be duplicated
+ * for each pmu. Set the metric leader of such events to be the
+ * event that appears in metric_events.
+ */
+ evlist__for_each_entry_continue(perf_evlist, ev) {
+ /*
+ * If events are grouped then the search can terminate
+ * when then group is left.
+ */
+ if (!has_constraint &&
+ ev->leader != metric_events[i]->leader)
+ break;
+ if (!strcmp(metric_events[i]->name, ev->name)) {
+ set_bit(ev->idx, evlist_used);
+ ev->metric_leader = metric_events[i];
+ }
+ }
}
return metric_events[0];
--
2.28.0.526.ge36021eeef-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] perf metricgroup: Fix uncore metric expressions
2020-09-10 18:02 [PATCH v3] perf metricgroup: Fix uncore metric expressions Ian Rogers
@ 2020-09-11 3:07 ` Namhyung Kim
2020-09-17 19:00 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 9+ messages in thread
From: Namhyung Kim @ 2020-09-11 3:07 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Kajol Jain,
Kan Liang, Jin Yao, Thomas Richter, linux-kernel,
Stephane Eranian
On Fri, Sep 11, 2020 at 3:02 AM Ian Rogers <irogers@google.com> wrote:
>
> A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> and uncore_imc/case_count_write/. These events open 6 events per socket
> with pmu names of uncore_imc_[0-5]. The current metric setup code in
> find_evsel_group assumes one ID will map to 1 event to be recorded in
> metric_events. For events with multiple matches, the first event is
> recorded in metric_events (avoiding matching >1 event with the same
> name) and the evlist_used updated so that duplicate events aren't
> removed when the evlist has unused events removed.
>
> Before this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
> Performance counter stats for 'system wide':
>
> 41.14 MiB uncore_imc/cas_count_read/
> 1,002,614,251 ns duration_time
>
> 1.002614251 seconds time elapsed
>
> After this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
> Performance counter stats for 'system wide':
>
> 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
> 126.97 MiB uncore_imc/cas_count_write/
> 1,003,019,728 ns duration_time
>
> v3. cleans up searching for the same event within metric_events to use a
> helper and avoids a redundant search. It uses a continue loop to
> make the search for similarly named events shorter.
> v2. avoids iterating over the whole evlist as suggested by
> namhyung@kernel.org. It also fixes the metric_leader computation
> that was broken in the same commits.
>
> Erroneous duplication introduced in:
> commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
>
> Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks
Namhyung
> ---
> tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
> 1 file changed, 56 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 662f4e8777d5..7b66452cb9f9 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -149,6 +149,18 @@ static void expr_ids__exit(struct expr_ids *ids)
> free(ids->id[i].id);
> }
>
> +static bool contains_event(struct evsel **metric_events, int num_events,
> + const char *event_name)
> +{
> + int i;
> +
> + for (i = 0; i < num_events; i++) {
> + if (!strcmp(metric_events[i]->name, event_name))
> + return true;
> + }
> + return false;
> +}
> +
> /**
> * Find a group of events in perf_evlist that correspond to those from a parsed
> * metric expression. Note, as find_evsel_group is called in the same order as
> @@ -179,7 +191,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> int i = 0, matched_events = 0, events_to_match;
> const int idnum = (int)hashmap__size(&pctx->ids);
>
> - /* duration_time is grouped separately. */
> + /*
> + * duration_time is always grouped separately, when events are grouped
> + * (ie has_constraint is false) then ignore it in the matching loop and
> + * add it to metric_events at the end.
> + */
> if (!has_constraint &&
> hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
> events_to_match = idnum - 1;
> @@ -206,23 +222,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> sizeof(struct evsel *) * idnum);
> current_leader = ev->leader;
> }
> - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> - if (has_constraint) {
> - /*
> - * Events aren't grouped, ensure the same event
> - * isn't matched from two groups.
> - */
> - for (i = 0; i < matched_events; i++) {
> - if (!strcmp(ev->name,
> - metric_events[i]->name)) {
> - break;
> - }
> - }
> - if (i != matched_events)
> - continue;
> - }
> + /*
> + * Check for duplicate events with the same name. For example,
> + * uncore_imc/cas_count_read/ will turn into 6 events per socket
> + * on skylakex. Only the first such event is placed in
> + * metric_events. If events aren't grouped then this also
> + * ensures that the same event in different sibling groups
> + * aren't both added to metric_events.
> + */
> + if (contains_event(metric_events, matched_events, ev->name))
> + continue;
> + /* Does this event belong to the parse context? */
> + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
> metric_events[matched_events++] = ev;
> - }
> +
> if (matched_events == events_to_match)
> break;
> }
> @@ -238,7 +251,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> }
>
> if (matched_events != idnum) {
> - /* Not whole match */
> + /* Not a whole match */
> return NULL;
> }
>
> @@ -246,8 +259,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>
> for (i = 0; i < idnum; i++) {
> ev = metric_events[i];
> - ev->metric_leader = ev;
> + /* Don't free the used events. */
> set_bit(ev->idx, evlist_used);
> + /*
> + * The metric leader points to the identically named event in
> + * metric_events.
> + */
> + ev->metric_leader = ev;
> + /*
> + * Mark two events with identical names in the same group (or
> + * globally) as being in use as uncore events may be duplicated
> + * for each pmu. Set the metric leader of such events to be the
> + * event that appears in metric_events.
> + */
> + evlist__for_each_entry_continue(perf_evlist, ev) {
> + /*
> + * If events are grouped then the search can terminate
> + * when then group is left.
> + */
> + if (!has_constraint &&
> + ev->leader != metric_events[i]->leader)
> + break;
> + if (!strcmp(metric_events[i]->name, ev->name)) {
> + set_bit(ev->idx, evlist_used);
> + ev->metric_leader = metric_events[i];
> + }
> + }
> }
>
> return metric_events[0];
> --
> 2.28.0.526.ge36021eeef-goog
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3] perf metricgroup: Fix uncore metric expressions
2020-09-11 3:07 ` Namhyung Kim
@ 2020-09-17 19:00 ` Arnaldo Carvalho de Melo
2020-09-17 20:18 ` [PATCH v4] " Ian Rogers
2020-09-17 20:20 ` [PATCH v3] " Ian Rogers
0 siblings, 2 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-17 19:00 UTC (permalink / raw)
To: Ian Rogers
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Kajol Jain, Kan Liang, Jin Yao,
Thomas Richter, linux-kernel, Stephane Eranian
Em Fri, Sep 11, 2020 at 12:07:35PM +0900, Namhyung Kim escreveu:
> On Fri, Sep 11, 2020 at 3:02 AM Ian Rogers <irogers@google.com> wrote:
> > v3. cleans up searching for the same event within metric_events to use a
> > helper and avoids a redundant search. It uses a continue loop to
> > make the search for similarly named events shorter.
> > v2. avoids iterating over the whole evlist as suggested by
> > namhyung@kernel.org. It also fixes the metric_leader computation
> > that was broken in the same commits.
> >
> > Erroneous duplication introduced in:
> > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> >
> > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
On my perf/urgent branch (upstream should be the same):
[acme@five perf]$ patch -p1 < /wb/1.patch
patching file tools/perf/util/metricgroup.c
Hunk #1 succeeded at 150 with fuzz 2 (offset 1 line).
Hunk #2 succeeded at 192 (offset 1 line).
Hunk #3 succeeded at 223 (offset 1 line).
Hunk #4 succeeded at 252 (offset 1 line).
Hunk #5 succeeded at 260 (offset 1 line).
[acme@five perf]$
I'm fixing it up, please check that doing this is safe on your side.
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v4] perf metricgroup: Fix uncore metric expressions
2020-09-17 19:00 ` Arnaldo Carvalho de Melo
@ 2020-09-17 20:18 ` Ian Rogers
2020-09-17 20:39 ` Arnaldo Carvalho de Melo
2020-09-17 20:20 ` [PATCH v3] " Ian Rogers
1 sibling, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2020-09-17 20:18 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel, netdev,
bpf
Cc: Stephane Eranian, Ian Rogers, Jin Yao
A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
and uncore_imc/case_count_write/. These events open 6 events per socket
with pmu names of uncore_imc_[0-5]. The current metric setup code in
find_evsel_group assumes one ID will map to 1 event to be recorded in
metric_events. For events with multiple matches, the first event is
recorded in metric_events (avoiding matching >1 event with the same
name) and the evlist_used updated so that duplicate events aren't
removed when the evlist has unused events removed.
Before this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
Performance counter stats for 'system wide':
41.14 MiB uncore_imc/cas_count_read/
1,002,614,251 ns duration_time
1.002614251 seconds time elapsed
After this change:
$ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
Performance counter stats for 'system wide':
157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
126.97 MiB uncore_imc/cas_count_write/
1,003,019,728 ns duration_time
Erroneous duplication introduced in:
commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
Reported-by: Jin Yao <yao.jin@linux.intel.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
1 file changed, 56 insertions(+), 19 deletions(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index ab5030fcfed4..d948a7f910cf 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids)
free(ids->id[i].id);
}
+static bool contains_event(struct evsel **metric_events, int num_events,
+ const char *event_name)
+{
+ int i;
+
+ for (i = 0; i < num_events; i++) {
+ if (!strcmp(metric_events[i]->name, event_name))
+ return true;
+ }
+ return false;
+}
+
/**
* Find a group of events in perf_evlist that correpond to those from a parsed
* metric expression. Note, as find_evsel_group is called in the same order as
@@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
int i = 0, matched_events = 0, events_to_match;
const int idnum = (int)hashmap__size(&pctx->ids);
- /* duration_time is grouped separately. */
+ /*
+ * duration_time is always grouped separately, when events are grouped
+ * (ie has_constraint is false) then ignore it in the matching loop and
+ * add it to metric_events at the end.
+ */
if (!has_constraint &&
hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
events_to_match = idnum - 1;
@@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
sizeof(struct evsel *) * idnum);
current_leader = ev->leader;
}
- if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
- if (has_constraint) {
- /*
- * Events aren't grouped, ensure the same event
- * isn't matched from two groups.
- */
- for (i = 0; i < matched_events; i++) {
- if (!strcmp(ev->name,
- metric_events[i]->name)) {
- break;
- }
- }
- if (i != matched_events)
- continue;
- }
+ /*
+ * Check for duplicate events with the same name. For example,
+ * uncore_imc/cas_count_read/ will turn into 6 events per socket
+ * on skylakex. Only the first such event is placed in
+ * metric_events. If events aren't grouped then this also
+ * ensures that the same event in different sibling groups
+ * aren't both added to metric_events.
+ */
+ if (contains_event(metric_events, matched_events, ev->name))
+ continue;
+ /* Does this event belong to the parse context? */
+ if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
metric_events[matched_events++] = ev;
- }
+
if (matched_events == events_to_match)
break;
}
@@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
}
if (matched_events != idnum) {
- /* Not whole match */
+ /* Not a whole match */
return NULL;
}
@@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
for (i = 0; i < idnum; i++) {
ev = metric_events[i];
- ev->metric_leader = ev;
+ /* Don't free the used events. */
set_bit(ev->idx, evlist_used);
+ /*
+ * The metric leader points to the identically named event in
+ * metric_events.
+ */
+ ev->metric_leader = ev;
+ /*
+ * Mark two events with identical names in the same group (or
+ * globally) as being in use as uncore events may be duplicated
+ * for each pmu. Set the metric leader of such events to be the
+ * event that appears in metric_events.
+ */
+ evlist__for_each_entry_continue(perf_evlist, ev) {
+ /*
+ * If events are grouped then the search can terminate
+ * when then group is left.
+ */
+ if (!has_constraint &&
+ ev->leader != metric_events[i]->leader)
+ break;
+ if (!strcmp(metric_events[i]->name, ev->name)) {
+ set_bit(ev->idx, evlist_used);
+ ev->metric_leader = metric_events[i];
+ }
+ }
}
return metric_events[0];
--
2.28.0.618.gf4bc123cb7-goog
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] perf metricgroup: Fix uncore metric expressions
2020-09-17 19:00 ` Arnaldo Carvalho de Melo
2020-09-17 20:18 ` [PATCH v4] " Ian Rogers
@ 2020-09-17 20:20 ` Ian Rogers
1 sibling, 0 replies; 9+ messages in thread
From: Ian Rogers @ 2020-09-17 20:20 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Kajol Jain, Kan Liang, Jin Yao,
Thomas Richter, linux-kernel, Stephane Eranian
Should be ok. I cherry-picked the patch on to a local checkout of
perf/urgent and sent as an in-reply-to here:
https://lore.kernel.org/lkml/20200917190026.GB1426933@kernel.org/T/#m6c12d6a224540ed5f222e58fff9807f1cdc4238f
Thanks,
Ian
On Thu, Sep 17, 2020 at 12:00 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Fri, Sep 11, 2020 at 12:07:35PM +0900, Namhyung Kim escreveu:
> > On Fri, Sep 11, 2020 at 3:02 AM Ian Rogers <irogers@google.com> wrote:
> > > v3. cleans up searching for the same event within metric_events to use a
> > > helper and avoids a redundant search. It uses a continue loop to
> > > make the search for similarly named events shorter.
> > > v2. avoids iterating over the whole evlist as suggested by
> > > namhyung@kernel.org. It also fixes the metric_leader computation
> > > that was broken in the same commits.
> > >
> > > Erroneous duplication introduced in:
> > > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> > >
> > > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
>
> On my perf/urgent branch (upstream should be the same):
>
> [acme@five perf]$ patch -p1 < /wb/1.patch
> patching file tools/perf/util/metricgroup.c
> Hunk #1 succeeded at 150 with fuzz 2 (offset 1 line).
> Hunk #2 succeeded at 192 (offset 1 line).
> Hunk #3 succeeded at 223 (offset 1 line).
> Hunk #4 succeeded at 252 (offset 1 line).
> Hunk #5 succeeded at 260 (offset 1 line).
> [acme@five perf]$
>
> I'm fixing it up, please check that doing this is safe on your side.
>
> - Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions
2020-09-17 20:18 ` [PATCH v4] " Ian Rogers
@ 2020-09-17 20:39 ` Arnaldo Carvalho de Melo
2020-09-18 1:47 ` Namhyung Kim
0 siblings, 1 reply; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-17 20:39 UTC (permalink / raw)
To: Namhyung Kim, Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau,
Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend,
KP Singh, Adrian Hunter, Andi Kleen, Athira Rajeev, linux-kernel,
netdev, bpf, Stephane Eranian, Jin Yao
Em Thu, Sep 17, 2020 at 01:18:07PM -0700, Ian Rogers escreveu:
> A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> and uncore_imc/case_count_write/. These events open 6 events per socket
> with pmu names of uncore_imc_[0-5]. The current metric setup code in
> find_evsel_group assumes one ID will map to 1 event to be recorded in
> metric_events. For events with multiple matches, the first event is
> recorded in metric_events (avoiding matching >1 event with the same
> name) and the evlist_used updated so that duplicate events aren't
> removed when the evlist has unused events removed.
Namhyung, please check if you still Acks this as you provided it for v3.
Thanks,
- Arnaldo
> Before this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
> Performance counter stats for 'system wide':
>
> 41.14 MiB uncore_imc/cas_count_read/
> 1,002,614,251 ns duration_time
>
> 1.002614251 seconds time elapsed
>
> After this change:
> $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
>
> Performance counter stats for 'system wide':
>
> 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
> 126.97 MiB uncore_imc/cas_count_write/
> 1,003,019,728 ns duration_time
>
> Erroneous duplication introduced in:
> commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
>
> Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
> 1 file changed, 56 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index ab5030fcfed4..d948a7f910cf 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids)
> free(ids->id[i].id);
> }
>
> +static bool contains_event(struct evsel **metric_events, int num_events,
> + const char *event_name)
> +{
> + int i;
> +
> + for (i = 0; i < num_events; i++) {
> + if (!strcmp(metric_events[i]->name, event_name))
> + return true;
> + }
> + return false;
> +}
> +
> /**
> * Find a group of events in perf_evlist that correpond to those from a parsed
> * metric expression. Note, as find_evsel_group is called in the same order as
> @@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> int i = 0, matched_events = 0, events_to_match;
> const int idnum = (int)hashmap__size(&pctx->ids);
>
> - /* duration_time is grouped separately. */
> + /*
> + * duration_time is always grouped separately, when events are grouped
> + * (ie has_constraint is false) then ignore it in the matching loop and
> + * add it to metric_events at the end.
> + */
> if (!has_constraint &&
> hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
> events_to_match = idnum - 1;
> @@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> sizeof(struct evsel *) * idnum);
> current_leader = ev->leader;
> }
> - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> - if (has_constraint) {
> - /*
> - * Events aren't grouped, ensure the same event
> - * isn't matched from two groups.
> - */
> - for (i = 0; i < matched_events; i++) {
> - if (!strcmp(ev->name,
> - metric_events[i]->name)) {
> - break;
> - }
> - }
> - if (i != matched_events)
> - continue;
> - }
> + /*
> + * Check for duplicate events with the same name. For example,
> + * uncore_imc/cas_count_read/ will turn into 6 events per socket
> + * on skylakex. Only the first such event is placed in
> + * metric_events. If events aren't grouped then this also
> + * ensures that the same event in different sibling groups
> + * aren't both added to metric_events.
> + */
> + if (contains_event(metric_events, matched_events, ev->name))
> + continue;
> + /* Does this event belong to the parse context? */
> + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
> metric_events[matched_events++] = ev;
> - }
> +
> if (matched_events == events_to_match)
> break;
> }
> @@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> }
>
> if (matched_events != idnum) {
> - /* Not whole match */
> + /* Not a whole match */
> return NULL;
> }
>
> @@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
>
> for (i = 0; i < idnum; i++) {
> ev = metric_events[i];
> - ev->metric_leader = ev;
> + /* Don't free the used events. */
> set_bit(ev->idx, evlist_used);
> + /*
> + * The metric leader points to the identically named event in
> + * metric_events.
> + */
> + ev->metric_leader = ev;
> + /*
> + * Mark two events with identical names in the same group (or
> + * globally) as being in use as uncore events may be duplicated
> + * for each pmu. Set the metric leader of such events to be the
> + * event that appears in metric_events.
> + */
> + evlist__for_each_entry_continue(perf_evlist, ev) {
> + /*
> + * If events are grouped then the search can terminate
> + * when then group is left.
> + */
> + if (!has_constraint &&
> + ev->leader != metric_events[i]->leader)
> + break;
> + if (!strcmp(metric_events[i]->name, ev->name)) {
> + set_bit(ev->idx, evlist_used);
> + ev->metric_leader = metric_events[i];
> + }
> + }
> }
>
> return metric_events[0];
> --
> 2.28.0.618.gf4bc123cb7-goog
>
--
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions
2020-09-17 20:39 ` Arnaldo Carvalho de Melo
@ 2020-09-18 1:47 ` Namhyung Kim
2020-09-23 19:59 ` Ian Rogers
0 siblings, 1 reply; 9+ messages in thread
From: Namhyung Kim @ 2020-09-18 1:47 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Ian Rogers, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, Adrian Hunter,
Andi Kleen, Athira Rajeev, linux-kernel, Network Development,
bpf, Stephane Eranian, Jin Yao
Hi Arnaldo,
On Fri, Sep 18, 2020 at 5:39 AM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> Em Thu, Sep 17, 2020 at 01:18:07PM -0700, Ian Rogers escreveu:
> > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> > and uncore_imc/case_count_write/. These events open 6 events per socket
> > with pmu names of uncore_imc_[0-5]. The current metric setup code in
> > find_evsel_group assumes one ID will map to 1 event to be recorded in
> > metric_events. For events with multiple matches, the first event is
> > recorded in metric_events (avoiding matching >1 event with the same
> > name) and the evlist_used updated so that duplicate events aren't
> > removed when the evlist has unused events removed.
>
> Namhyung, please check if you still Acks this as you provided it for v3.
Sure,
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks
Namhyung
>
> > Before this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > 41.14 MiB uncore_imc/cas_count_read/
> > 1,002,614,251 ns duration_time
> >
> > 1.002614251 seconds time elapsed
> >
> > After this change:
> > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> >
> > Performance counter stats for 'system wide':
> >
> > 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
> > 126.97 MiB uncore_imc/cas_count_write/
> > 1,003,019,728 ns duration_time
> >
> > Erroneous duplication introduced in:
> > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> >
> > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
> > 1 file changed, 56 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > index ab5030fcfed4..d948a7f910cf 100644
> > --- a/tools/perf/util/metricgroup.c
> > +++ b/tools/perf/util/metricgroup.c
> > @@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids)
> > free(ids->id[i].id);
> > }
> >
> > +static bool contains_event(struct evsel **metric_events, int num_events,
> > + const char *event_name)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < num_events; i++) {
> > + if (!strcmp(metric_events[i]->name, event_name))
> > + return true;
> > + }
> > + return false;
> > +}
> > +
> > /**
> > * Find a group of events in perf_evlist that correpond to those from a parsed
> > * metric expression. Note, as find_evsel_group is called in the same order as
> > @@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > int i = 0, matched_events = 0, events_to_match;
> > const int idnum = (int)hashmap__size(&pctx->ids);
> >
> > - /* duration_time is grouped separately. */
> > + /*
> > + * duration_time is always grouped separately, when events are grouped
> > + * (ie has_constraint is false) then ignore it in the matching loop and
> > + * add it to metric_events at the end.
> > + */
> > if (!has_constraint &&
> > hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
> > events_to_match = idnum - 1;
> > @@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > sizeof(struct evsel *) * idnum);
> > current_leader = ev->leader;
> > }
> > - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> > - if (has_constraint) {
> > - /*
> > - * Events aren't grouped, ensure the same event
> > - * isn't matched from two groups.
> > - */
> > - for (i = 0; i < matched_events; i++) {
> > - if (!strcmp(ev->name,
> > - metric_events[i]->name)) {
> > - break;
> > - }
> > - }
> > - if (i != matched_events)
> > - continue;
> > - }
> > + /*
> > + * Check for duplicate events with the same name. For example,
> > + * uncore_imc/cas_count_read/ will turn into 6 events per socket
> > + * on skylakex. Only the first such event is placed in
> > + * metric_events. If events aren't grouped then this also
> > + * ensures that the same event in different sibling groups
> > + * aren't both added to metric_events.
> > + */
> > + if (contains_event(metric_events, matched_events, ev->name))
> > + continue;
> > + /* Does this event belong to the parse context? */
> > + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
> > metric_events[matched_events++] = ev;
> > - }
> > +
> > if (matched_events == events_to_match)
> > break;
> > }
> > @@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > }
> >
> > if (matched_events != idnum) {
> > - /* Not whole match */
> > + /* Not a whole match */
> > return NULL;
> > }
> >
> > @@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> >
> > for (i = 0; i < idnum; i++) {
> > ev = metric_events[i];
> > - ev->metric_leader = ev;
> > + /* Don't free the used events. */
> > set_bit(ev->idx, evlist_used);
> > + /*
> > + * The metric leader points to the identically named event in
> > + * metric_events.
> > + */
> > + ev->metric_leader = ev;
> > + /*
> > + * Mark two events with identical names in the same group (or
> > + * globally) as being in use as uncore events may be duplicated
> > + * for each pmu. Set the metric leader of such events to be the
> > + * event that appears in metric_events.
> > + */
> > + evlist__for_each_entry_continue(perf_evlist, ev) {
> > + /*
> > + * If events are grouped then the search can terminate
> > + * when then group is left.
> > + */
> > + if (!has_constraint &&
> > + ev->leader != metric_events[i]->leader)
> > + break;
> > + if (!strcmp(metric_events[i]->name, ev->name)) {
> > + set_bit(ev->idx, evlist_used);
> > + ev->metric_leader = metric_events[i];
> > + }
> > + }
> > }
> >
> > return metric_events[0];
> > --
> > 2.28.0.618.gf4bc123cb7-goog
> >
>
> --
>
> - Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions
2020-09-18 1:47 ` Namhyung Kim
@ 2020-09-23 19:59 ` Ian Rogers
2020-09-23 20:25 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 9+ messages in thread
From: Ian Rogers @ 2020-09-23 19:59 UTC (permalink / raw)
To: Namhyung Kim
Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, Adrian Hunter,
Andi Kleen, Athira Rajeev, linux-kernel, Network Development,
bpf, Stephane Eranian, Jin Yao
On Thu, Sep 17, 2020 at 6:47 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Arnaldo,
>
> On Fri, Sep 18, 2020 at 5:39 AM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > Em Thu, Sep 17, 2020 at 01:18:07PM -0700, Ian Rogers escreveu:
> > > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> > > and uncore_imc/case_count_write/. These events open 6 events per socket
> > > with pmu names of uncore_imc_[0-5]. The current metric setup code in
> > > find_evsel_group assumes one ID will map to 1 event to be recorded in
> > > metric_events. For events with multiple matches, the first event is
> > > recorded in metric_events (avoiding matching >1 event with the same
> > > name) and the evlist_used updated so that duplicate events aren't
> > > removed when the evlist has unused events removed.
> >
> > Namhyung, please check if you still Acks this as you provided it for v3.
>
> Sure,
>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks Namhyung and Arnaldo, could we merge this?
Ian
> Thanks
> Namhyung
>
> >
> > > Before this change:
> > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> > >
> > > Performance counter stats for 'system wide':
> > >
> > > 41.14 MiB uncore_imc/cas_count_read/
> > > 1,002,614,251 ns duration_time
> > >
> > > 1.002614251 seconds time elapsed
> > >
> > > After this change:
> > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> > >
> > > Performance counter stats for 'system wide':
> > >
> > > 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
> > > 126.97 MiB uncore_imc/cas_count_write/
> > > 1,003,019,728 ns duration_time
> > >
> > > Erroneous duplication introduced in:
> > > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> > >
> > > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > > tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
> > > 1 file changed, 56 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > index ab5030fcfed4..d948a7f910cf 100644
> > > --- a/tools/perf/util/metricgroup.c
> > > +++ b/tools/perf/util/metricgroup.c
> > > @@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids)
> > > free(ids->id[i].id);
> > > }
> > >
> > > +static bool contains_event(struct evsel **metric_events, int num_events,
> > > + const char *event_name)
> > > +{
> > > + int i;
> > > +
> > > + for (i = 0; i < num_events; i++) {
> > > + if (!strcmp(metric_events[i]->name, event_name))
> > > + return true;
> > > + }
> > > + return false;
> > > +}
> > > +
> > > /**
> > > * Find a group of events in perf_evlist that correpond to those from a parsed
> > > * metric expression. Note, as find_evsel_group is called in the same order as
> > > @@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > int i = 0, matched_events = 0, events_to_match;
> > > const int idnum = (int)hashmap__size(&pctx->ids);
> > >
> > > - /* duration_time is grouped separately. */
> > > + /*
> > > + * duration_time is always grouped separately, when events are grouped
> > > + * (ie has_constraint is false) then ignore it in the matching loop and
> > > + * add it to metric_events at the end.
> > > + */
> > > if (!has_constraint &&
> > > hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
> > > events_to_match = idnum - 1;
> > > @@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > sizeof(struct evsel *) * idnum);
> > > current_leader = ev->leader;
> > > }
> > > - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> > > - if (has_constraint) {
> > > - /*
> > > - * Events aren't grouped, ensure the same event
> > > - * isn't matched from two groups.
> > > - */
> > > - for (i = 0; i < matched_events; i++) {
> > > - if (!strcmp(ev->name,
> > > - metric_events[i]->name)) {
> > > - break;
> > > - }
> > > - }
> > > - if (i != matched_events)
> > > - continue;
> > > - }
> > > + /*
> > > + * Check for duplicate events with the same name. For example,
> > > + * uncore_imc/cas_count_read/ will turn into 6 events per socket
> > > + * on skylakex. Only the first such event is placed in
> > > + * metric_events. If events aren't grouped then this also
> > > + * ensures that the same event in different sibling groups
> > > + * aren't both added to metric_events.
> > > + */
> > > + if (contains_event(metric_events, matched_events, ev->name))
> > > + continue;
> > > + /* Does this event belong to the parse context? */
> > > + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
> > > metric_events[matched_events++] = ev;
> > > - }
> > > +
> > > if (matched_events == events_to_match)
> > > break;
> > > }
> > > @@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > }
> > >
> > > if (matched_events != idnum) {
> > > - /* Not whole match */
> > > + /* Not a whole match */
> > > return NULL;
> > > }
> > >
> > > @@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > >
> > > for (i = 0; i < idnum; i++) {
> > > ev = metric_events[i];
> > > - ev->metric_leader = ev;
> > > + /* Don't free the used events. */
> > > set_bit(ev->idx, evlist_used);
> > > + /*
> > > + * The metric leader points to the identically named event in
> > > + * metric_events.
> > > + */
> > > + ev->metric_leader = ev;
> > > + /*
> > > + * Mark two events with identical names in the same group (or
> > > + * globally) as being in use as uncore events may be duplicated
> > > + * for each pmu. Set the metric leader of such events to be the
> > > + * event that appears in metric_events.
> > > + */
> > > + evlist__for_each_entry_continue(perf_evlist, ev) {
> > > + /*
> > > + * If events are grouped then the search can terminate
> > > + * when then group is left.
> > > + */
> > > + if (!has_constraint &&
> > > + ev->leader != metric_events[i]->leader)
> > > + break;
> > > + if (!strcmp(metric_events[i]->name, ev->name)) {
> > > + set_bit(ev->idx, evlist_used);
> > > + ev->metric_leader = metric_events[i];
> > > + }
> > > + }
> > > }
> > >
> > > return metric_events[0];
> > > --
> > > 2.28.0.618.gf4bc123cb7-goog
> > >
> >
> > --
> >
> > - Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions
2020-09-23 19:59 ` Ian Rogers
@ 2020-09-23 20:25 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 9+ messages in thread
From: Arnaldo Carvalho de Melo @ 2020-09-23 20:25 UTC (permalink / raw)
To: Ian Rogers
Cc: Namhyung Kim, Peter Zijlstra, Ingo Molnar, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, Adrian Hunter,
Andi Kleen, Athira Rajeev, linux-kernel, Network Development,
bpf, Stephane Eranian, Jin Yao
Em Wed, Sep 23, 2020 at 12:59:28PM -0700, Ian Rogers escreveu:
> On Thu, Sep 17, 2020 at 6:47 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Hi Arnaldo,
> >
> > On Fri, Sep 18, 2020 at 5:39 AM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > Em Thu, Sep 17, 2020 at 01:18:07PM -0700, Ian Rogers escreveu:
> > > > A metric like DRAM_BW_Use has on SkylakeX events uncore_imc/cas_count_read/
> > > > and uncore_imc/case_count_write/. These events open 6 events per socket
> > > > with pmu names of uncore_imc_[0-5]. The current metric setup code in
> > > > find_evsel_group assumes one ID will map to 1 event to be recorded in
> > > > metric_events. For events with multiple matches, the first event is
> > > > recorded in metric_events (avoiding matching >1 event with the same
> > > > name) and the evlist_used updated so that duplicate events aren't
> > > > removed when the evlist has unused events removed.
> > >
> > > Namhyung, please check if you still Acks this as you provided it for v3.
> >
> > Sure,
> >
> > Acked-by: Namhyung Kim <namhyung@kernel.org>
>
> Thanks Namhyung and Arnaldo, could we merge this?
[acme@five perf]$ git log --oneline perf/urgent
dcc81be0fc4e6694 (quaco/perf/urgent, perf/urgent) perf metricgroup: Fix uncore metric expressions
0f1b550e29c1f952 perf parse-event: Release cpu_map refcount if evsel alloc failed
5d680be3b014d583 perf parse-event: Fix cpu map refcounting
5925fa68fe824465 (torvalds/master) Merge tag 'perf-tools-fixes-for-v5.9-2020-09-16' of git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux
I have it already locally, need to finish a round of testing so that I
can push it publicly,
- Arnaldo
> Ian
>
> > Thanks
> > Namhyung
> >
> > >
> > > > Before this change:
> > > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> > > >
> > > > Performance counter stats for 'system wide':
> > > >
> > > > 41.14 MiB uncore_imc/cas_count_read/
> > > > 1,002,614,251 ns duration_time
> > > >
> > > > 1.002614251 seconds time elapsed
> > > >
> > > > After this change:
> > > > $ /tmp/perf/perf stat -M DRAM_BW_Use -a -- sleep 1
> > > >
> > > > Performance counter stats for 'system wide':
> > > >
> > > > 157.47 MiB uncore_imc/cas_count_read/ # 0.00 DRAM_BW_Use
> > > > 126.97 MiB uncore_imc/cas_count_write/
> > > > 1,003,019,728 ns duration_time
> > > >
> > > > Erroneous duplication introduced in:
> > > > commit 2440689d62e9 ("perf metricgroup: Remove duped metric group events").
> > > >
> > > > Fixes: ded80bda8bc9 ("perf expr: Migrate expr ids table to a hashmap").
> > > > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > > ---
> > > > tools/perf/util/metricgroup.c | 75 ++++++++++++++++++++++++++---------
> > > > 1 file changed, 56 insertions(+), 19 deletions(-)
> > > >
> > > > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> > > > index ab5030fcfed4..d948a7f910cf 100644
> > > > --- a/tools/perf/util/metricgroup.c
> > > > +++ b/tools/perf/util/metricgroup.c
> > > > @@ -150,6 +150,18 @@ static void expr_ids__exit(struct expr_ids *ids)
> > > > free(ids->id[i].id);
> > > > }
> > > >
> > > > +static bool contains_event(struct evsel **metric_events, int num_events,
> > > > + const char *event_name)
> > > > +{
> > > > + int i;
> > > > +
> > > > + for (i = 0; i < num_events; i++) {
> > > > + if (!strcmp(metric_events[i]->name, event_name))
> > > > + return true;
> > > > + }
> > > > + return false;
> > > > +}
> > > > +
> > > > /**
> > > > * Find a group of events in perf_evlist that correpond to those from a parsed
> > > > * metric expression. Note, as find_evsel_group is called in the same order as
> > > > @@ -180,7 +192,11 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > > int i = 0, matched_events = 0, events_to_match;
> > > > const int idnum = (int)hashmap__size(&pctx->ids);
> > > >
> > > > - /* duration_time is grouped separately. */
> > > > + /*
> > > > + * duration_time is always grouped separately, when events are grouped
> > > > + * (ie has_constraint is false) then ignore it in the matching loop and
> > > > + * add it to metric_events at the end.
> > > > + */
> > > > if (!has_constraint &&
> > > > hashmap__find(&pctx->ids, "duration_time", (void **)&val_ptr))
> > > > events_to_match = idnum - 1;
> > > > @@ -207,23 +223,20 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > > sizeof(struct evsel *) * idnum);
> > > > current_leader = ev->leader;
> > > > }
> > > > - if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr)) {
> > > > - if (has_constraint) {
> > > > - /*
> > > > - * Events aren't grouped, ensure the same event
> > > > - * isn't matched from two groups.
> > > > - */
> > > > - for (i = 0; i < matched_events; i++) {
> > > > - if (!strcmp(ev->name,
> > > > - metric_events[i]->name)) {
> > > > - break;
> > > > - }
> > > > - }
> > > > - if (i != matched_events)
> > > > - continue;
> > > > - }
> > > > + /*
> > > > + * Check for duplicate events with the same name. For example,
> > > > + * uncore_imc/cas_count_read/ will turn into 6 events per socket
> > > > + * on skylakex. Only the first such event is placed in
> > > > + * metric_events. If events aren't grouped then this also
> > > > + * ensures that the same event in different sibling groups
> > > > + * aren't both added to metric_events.
> > > > + */
> > > > + if (contains_event(metric_events, matched_events, ev->name))
> > > > + continue;
> > > > + /* Does this event belong to the parse context? */
> > > > + if (hashmap__find(&pctx->ids, ev->name, (void **)&val_ptr))
> > > > metric_events[matched_events++] = ev;
> > > > - }
> > > > +
> > > > if (matched_events == events_to_match)
> > > > break;
> > > > }
> > > > @@ -239,7 +252,7 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > > }
> > > >
> > > > if (matched_events != idnum) {
> > > > - /* Not whole match */
> > > > + /* Not a whole match */
> > > > return NULL;
> > > > }
> > > >
> > > > @@ -247,8 +260,32 @@ static struct evsel *find_evsel_group(struct evlist *perf_evlist,
> > > >
> > > > for (i = 0; i < idnum; i++) {
> > > > ev = metric_events[i];
> > > > - ev->metric_leader = ev;
> > > > + /* Don't free the used events. */
> > > > set_bit(ev->idx, evlist_used);
> > > > + /*
> > > > + * The metric leader points to the identically named event in
> > > > + * metric_events.
> > > > + */
> > > > + ev->metric_leader = ev;
> > > > + /*
> > > > + * Mark two events with identical names in the same group (or
> > > > + * globally) as being in use as uncore events may be duplicated
> > > > + * for each pmu. Set the metric leader of such events to be the
> > > > + * event that appears in metric_events.
> > > > + */
> > > > + evlist__for_each_entry_continue(perf_evlist, ev) {
> > > > + /*
> > > > + * If events are grouped then the search can terminate
> > > > + * when then group is left.
> > > > + */
> > > > + if (!has_constraint &&
> > > > + ev->leader != metric_events[i]->leader)
> > > > + break;
> > > > + if (!strcmp(metric_events[i]->name, ev->name)) {
> > > > + set_bit(ev->idx, evlist_used);
> > > > + ev->metric_leader = metric_events[i];
> > > > + }
> > > > + }
> > > > }
> > > >
> > > > return metric_events[0];
> > > > --
> > > > 2.28.0.618.gf4bc123cb7-goog
> > > >
> > >
> > > --
> > >
> > > - Arnaldo
--
- Arnaldo
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-23 20:25 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-10 18:02 [PATCH v3] perf metricgroup: Fix uncore metric expressions Ian Rogers
2020-09-11 3:07 ` Namhyung Kim
2020-09-17 19:00 ` Arnaldo Carvalho de Melo
2020-09-17 20:18 ` [PATCH v4] " Ian Rogers
2020-09-17 20:39 ` Arnaldo Carvalho de Melo
2020-09-18 1:47 ` Namhyung Kim
2020-09-23 19:59 ` Ian Rogers
2020-09-23 20:25 ` Arnaldo Carvalho de Melo
2020-09-17 20:20 ` [PATCH v3] " Ian Rogers
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).