* [PATCH v4] perf metricgroup: Fix uncore metric expressions [not found] <20200917190026.GB1426933@kernel.org> @ 2020-09-17 20:18 ` Ian Rogers 2020-09-17 20:39 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* Re: [PATCH v4] perf metricgroup: Fix uncore metric expressions 2020-09-17 20:18 ` [PATCH v4] perf metricgroup: Fix uncore metric expressions Ian Rogers @ 2020-09-17 20:39 ` Arnaldo Carvalho de Melo 2020-09-18 1:47 ` Namhyung Kim 0 siblings, 1 reply; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2020-09-23 20:25 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20200917190026.GB1426933@kernel.org>
2020-09-17 20:18 ` [PATCH v4] perf metricgroup: Fix uncore metric expressions 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
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).