Re: [V3 PATCH] perf parse-events: Specially handle uncore event alias in small groups

[Liang, Kan]

On 4/26/2018 12:14 PM, Jiri Olsa wrote:
> On Wed, Apr 25, 2018 at 10:58:43AM -0700, kan.liang@xxxxxxxxxxxxxxx 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;
> > +}