Messages in this thread | | | Subject | Re: [V3 PATCH] perf parse-events: Specially handle uncore event alias in small groups | From | "Liang, Kan" <> | Date | Thu, 26 Apr 2018 12:48:28 -0400 |
| |
On 4/26/2018 12:14 PM, Jiri Olsa wrote: > On Wed, Apr 25, 2018 at 10:58:43AM -0700, kan.liang@linux.intel.com wrote: > > SNIP > >> -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 (strncmp(pmu_name_a, pmu_name_b, end_a - pmu_name_a) == 0); >> +} >> + >> +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; >> + bool is_leader = true; >> + 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); > > could you please use list_last_entry and list_first_entry in here? >
Sure.
>> + total_members = evsel->idx - leader->idx + 1; >> + >> + leaders = calloc(total_members, sizeof(uintptr_t)); >> + if (!leaders) >> + return ret; > > returns 0 in here
OK
> >> + >> + __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 (!is_leader) >> + continue; >> + /* >> + * If the event's PMU name starts to repeat, it must be a new >> + * event. That can be used to distinguish the leader from >> + * other members, even they have the same event name. >> + */ >> + if ((leader != evsel) && (leader->pmu_name == evsel->pmu_name)) { >> + is_leader = false; > > setting "is_leader = false" basically breaks the loop, > because of that !leader check above, so why continue? >
Because the evsel doesn't belong to leader anymore. We should not add it into leaders[].
>> + continue; >> + } >> + /* The name is always alias name */ >> + WARN_ON(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++]; >> + } > > it's not so obvious from the code how the groups > are set.. please describe that logic in the comment >
Sure. I will add more comments.
Thanks, Kan
> thanks, > jirka > >> + >> + 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; >> +}
| |