Messages in this thread | | | Subject | Re: [PATCH 4/5] perf parse-events: Specially handle uncore event alias in small groups | From | "Liang, Kan" <> | Date | Tue, 24 Apr 2018 15:23:06 -0400 |
| |
On 4/24/2018 3:17 PM, Arnaldo Carvalho de Melo wrote: > Em Tue, Apr 24, 2018 at 11:20:13AM -0700, kan.liang@linux.intel.com escreveu: >> From: Kan Liang <kan.liang@linux.intel.com> >> >> Perf stat doesn't count the uncore event aliases from the same uncore >> block in a group, for example: > > This one is not applying to acme/perf/urgent, all the rest I got merged > there, the last one with that change from using strstr() to a new bool > in perf_evsel for the uniquifying operation having being performed. >
Sure. Thank you for letting me know.
Thanks, Kan
> - Arnaldo > >> perf stat -e '{unc_m_cas_count.all,unc_m_clockticks}' -a -I 1000 >> # time counts unit events >> 1.000447342 <not counted> unc_m_cas_count.all >> 1.000447342 <not counted> unc_m_clockticks >> 2.000740654 <not counted> unc_m_cas_count.all >> 2.000740654 <not counted> unc_m_clockticks >> >> The output is very misleading. It gives a wrong impression that the >> uncore event doesn't work. >> >> An uncore block could be composed by several PMUs. An uncore event alias >> is a joint name which means the same event runs on all PMUs of a block. >> Perf doesn't support mixed events from different PMUs in the same group. >> It is wrong to put uncore event aliases in a big group. >> >> The right way is to split the big group into multiple small groups which >> only include the events from the same PMU. >> Only uncore event aliases from the same uncore block should be specially >> handled here. It doesn't make sense to mix the uncore events with other >> uncore events from different blocks or even core events in a group. >> >> With the patch: >> # time counts unit events >> 1.001557653 140,833 unc_m_cas_count.all >> 1.001557653 1,330,231,332 unc_m_clockticks >> 2.002709483 85,007 unc_m_cas_count.all >> 2.002709483 1,429,494,563 unc_m_clockticks >> >> Reported-by: Andi Kleen <ak@linux.intel.com> >> Signed-off-by: Kan Liang <kan.liang@linux.intel.com> >> --- >> tools/perf/util/evsel.h | 1 + >> tools/perf/util/parse-events.c | 94 ++++++++++++++++++++++++++++++++++++++++-- >> tools/perf/util/parse-events.h | 7 +++- >> tools/perf/util/parse-events.y | 8 ++-- >> 4 files changed, 101 insertions(+), 9 deletions(-) >> >> diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h >> index d3ee3af..b8a1055 100644 >> --- a/tools/perf/util/evsel.h >> +++ b/tools/perf/util/evsel.h >> @@ -126,6 +126,7 @@ struct perf_evsel { >> bool precise_max; >> bool ignore_missing_thread; >> bool forced_leader; >> + bool use_uncore_alias; >> /* parse modifier helper */ >> int exclude_GH; >> int nr_members; >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index 2fb0272..7a836fe 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -1219,13 +1219,16 @@ int parse_events_add_numeric(struct parse_events_state *parse_state, >> >> int parse_events_add_pmu(struct parse_events_state *parse_state, >> struct list_head *list, char *name, >> - struct list_head *head_config, bool auto_merge_stats) >> + struct list_head *head_config, >> + bool auto_merge_stats, >> + bool use_alias) >> { >> struct perf_event_attr attr; >> struct perf_pmu_info info; >> struct perf_pmu *pmu; >> struct perf_evsel *evsel; >> struct parse_events_error *err = parse_state->error; >> + bool use_uncore_alias; >> LIST_HEAD(config_terms); >> >> pmu = perf_pmu__find(name); >> @@ -1244,11 +1247,14 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, >> memset(&attr, 0, sizeof(attr)); >> } >> >> + use_uncore_alias = (pmu->is_uncore && use_alias); >> + >> if (!head_config) { >> attr.type = pmu->type; >> evsel = __add_event(list, &parse_state->idx, &attr, NULL, pmu, NULL, auto_merge_stats); >> if (evsel) { >> evsel->pmu_name = name; >> + evsel->use_uncore_alias = use_uncore_alias; >> return 0; >> } else { >> return -ENOMEM; >> @@ -1282,6 +1288,7 @@ int parse_events_add_pmu(struct parse_events_state *parse_state, >> evsel->metric_expr = info.metric_expr; >> evsel->metric_name = info.metric_name; >> evsel->pmu_name = name; >> + evsel->use_uncore_alias = use_uncore_alias; >> } >> >> return evsel ? 0 : -ENOMEM; >> @@ -1317,7 +1324,7 @@ int parse_events_multi_pmu_add(struct parse_events_state *parse_state, >> list_add_tail(&term->list, head); >> >> if (!parse_events_add_pmu(parse_state, list, >> - pmu->name, head, true)) { >> + pmu->name, head, true, true)) { >> pr_debug("%s -> %s/%s/\n", str, >> pmu->name, alias->str); >> ok++; >> @@ -1339,7 +1346,85 @@ int parse_events__modifier_group(struct list_head *list, >> return parse_events__modifier_event(list, event_mod, true); >> } >> >> -void parse_events__set_leader(char *name, struct list_head *list) >> +/* >> + * Check if the two uncore PMUs are from the same uncore block >> + * The format of the uncore PMU name is uncore_#blockname_#pmuidx >> + */ >> +static bool is_same_uncore_block(const char *pmu_name_a, const char *pmu_name_b) >> +{ >> + char *end_a, *end_b; >> + >> + end_a = strrchr(pmu_name_a, '_'); >> + end_b = strrchr(pmu_name_b, '_'); >> + >> + if (!end_a || !end_b) >> + return false; >> + >> + if ((end_a - pmu_name_a) != (end_b - pmu_name_b)) >> + return false; >> + >> + return (0 == strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a)); >> +} >> + >> +static int >> +parse_events__set_leader_for_uncore_aliase(char *name, struct list_head *list, >> + struct parse_events_state *parse_state) >> +{ >> + struct perf_evsel *evsel, *leader; >> + uintptr_t *leaders; >> + int i = 0, nr_pmu = 0, total_members, ret = 0; >> + >> + leader = list_entry(list->next, struct perf_evsel, node); >> + evsel = list_entry(list->prev, struct perf_evsel, node); >> + total_members = evsel->idx - leader->idx + 1; >> + >> + leaders = calloc(total_members, sizeof(uintptr_t)); >> + if (!leaders) >> + return ret; >> + >> + __evlist__for_each_entry(list, evsel) { >> + >> + /* Only split the uncore group which members use alias */ >> + if (!evsel->use_uncore_alias) >> + goto out; >> + >> + /* The events must be from the same uncore block */ >> + if (!is_same_uncore_block(leader->pmu_name, evsel->pmu_name)) >> + goto out; >> + >> + if (!strcmp(leader->name, evsel->name)) >> + leaders[nr_pmu++] = (uintptr_t) evsel; >> + } >> + >> + /* only one event alias */ >> + if (nr_pmu == total_members) { >> + parse_state->nr_groups--; >> + goto handled; >> + } >> + >> + __evlist__for_each_entry(list, evsel) { >> + if (i >= nr_pmu) >> + i = 0; >> + evsel->leader = (struct perf_evsel *) leaders[i++]; >> + } >> + >> + for (i = 0; i < nr_pmu; i++) { >> + evsel = (struct perf_evsel *) leaders[i]; >> + evsel->nr_members = total_members / nr_pmu; >> + evsel->group_name = name ? strdup(name) : NULL; >> + } >> + >> + parse_state->nr_groups += nr_pmu - 1; >> + >> +handled: >> + ret = 1; >> +out: >> + free(leaders); >> + return ret; >> +} >> + >> +void parse_events__set_leader(char *name, struct list_head *list, >> + struct parse_events_state *parse_state) >> { >> struct perf_evsel *leader; >> >> @@ -1348,6 +1433,9 @@ void parse_events__set_leader(char *name, struct list_head *list) >> return; >> } >> >> + if (parse_events__set_leader_for_uncore_aliase(name, list, parse_state)) >> + return; >> + >> __perf_evlist__set_leader(list); >> leader = list_entry(list->next, struct perf_evsel, node); >> leader->group_name = name ? strdup(name) : NULL; >> diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h >> index 5015cfd..4473dac 100644 >> --- a/tools/perf/util/parse-events.h >> +++ b/tools/perf/util/parse-events.h >> @@ -167,7 +167,9 @@ int parse_events_add_breakpoint(struct list_head *list, int *idx, >> void *ptr, char *type, u64 len); >> int parse_events_add_pmu(struct parse_events_state *parse_state, >> struct list_head *list, char *name, >> - struct list_head *head_config, bool auto_merge_stats); >> + struct list_head *head_config, >> + bool auto_merge_stats, >> + bool use_alias); >> >> int parse_events_multi_pmu_add(struct parse_events_state *parse_state, >> char *str, >> @@ -178,7 +180,8 @@ int parse_events_copy_term_list(struct list_head *old, >> >> enum perf_pmu_event_symbol_type >> perf_pmu__parse_check(const char *name); >> -void parse_events__set_leader(char *name, struct list_head *list); >> +void parse_events__set_leader(char *name, struct list_head *list, >> + struct parse_events_state *parse_state); >> void parse_events_update_lists(struct list_head *list_event, >> struct list_head *list_all); >> void parse_events_evlist_error(struct parse_events_state *parse_state, >> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y >> index 7afeb80..e37608a 100644 >> --- a/tools/perf/util/parse-events.y >> +++ b/tools/perf/util/parse-events.y >> @@ -161,7 +161,7 @@ PE_NAME '{' events '}' >> struct list_head *list = $3; >> >> inc_group_count(list, _parse_state); >> - parse_events__set_leader($1, list); >> + parse_events__set_leader($1, list, _parse_state); >> $$ = list; >> } >> | >> @@ -170,7 +170,7 @@ PE_NAME '{' events '}' >> struct list_head *list = $2; >> >> inc_group_count(list, _parse_state); >> - parse_events__set_leader(NULL, list); >> + parse_events__set_leader(NULL, list, _parse_state); >> $$ = list; >> } >> >> @@ -232,7 +232,7 @@ PE_NAME opt_event_config >> YYABORT; >> >> ALLOC_LIST(list); >> - if (parse_events_add_pmu(_parse_state, list, $1, $2, false)) { >> + if (parse_events_add_pmu(_parse_state, list, $1, $2, false, false)) { >> struct perf_pmu *pmu = NULL; >> int ok = 0; >> char *pattern; >> @@ -251,7 +251,7 @@ PE_NAME opt_event_config >> free(pattern); >> YYABORT; >> } >> - if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true)) >> + if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false)) >> ok++; >> parse_events_terms__delete(terms); >> } >> -- >> 2.7.4
| |