* [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 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
* 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
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).