Re: [PATCH v2 1/2] perf evlist: Ensure grouped events with same cpu map

[Jin, Yao]

Hi Jiri,

On 5/27/2020 11:20 AM, Jin, Yao wrote:
> Hi Jiri,
>
> On 5/26/2020 7:51 PM, Jiri Olsa wrote:
> > On Mon, May 25, 2020 at 02:55:58PM +0800, Jin Yao wrote:
> >
> > SNIP
> >
> > > diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> > > index 2a9de6491700..1161cffc0688 100644
> > > --- a/tools/perf/util/evlist.c
> > > +++ b/tools/perf/util/evlist.c
> > > @@ -1704,3 +1704,52 @@ struct evsel *perf_evlist__reset_weak_group(struct evlist *evsel_list,
> > > ÂÂÂÂÂ }
> > > ÂÂÂÂÂ return leader;
> > > Â }
> > > +
> > > +static bool cpus_map_matched(struct evsel *prev, struct evsel *evsel)
> > > +{
> > > +ÂÂÂ if (evsel->core.cpus->nr != prev->core.cpus->nr)
> > > +ÂÂÂÂÂÂÂ return false;
> > > +
> > > +ÂÂÂ for (int i = 0; i < evsel->core.cpus->nr; i++) {
> > > +ÂÂÂÂÂÂÂ if (evsel->core.cpus->map[i] != prev->core.cpus->map[i])
> > > +ÂÂÂÂÂÂÂÂÂÂÂ return false;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ return true;
> > > +}
> > > +
> > > +bool evlist__cpus_map_matched(struct evlist *evlist)
> > > +{
> > > +ÂÂÂ struct evsel *prev = evlist__first(evlist), *evsel = prev;
> > > +ÂÂÂ int nr_members = prev->core.nr_members;
> > > +
> > > +ÂÂÂ evlist__for_each_entry_continue(evlist, evsel) {
> > > +ÂÂÂÂÂÂÂ if (nr_members <= 1) {
> > > +ÂÂÂÂÂÂÂÂÂÂÂ prev = evsel;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ nr_members = evsel->core.nr_members;
> > > +ÂÂÂÂÂÂÂÂÂÂÂ continue;
> > > +ÂÂÂÂÂÂÂ }
> > > +
> > > +ÂÂÂÂÂÂÂ nr_members--;
> > > +
> > > +ÂÂÂÂÂÂÂ if (!cpus_map_matched(prev, evsel))
> > > +ÂÂÂÂÂÂÂÂÂÂÂ return false;
> > > +
> > > +ÂÂÂÂÂÂÂ prev = evsel;
> > > +ÂÂÂ }
> > > +
> > > +ÂÂÂ return true;
> > > +}
> > > +
> > > +void evlist__force_disable_group(struct evlist *evlist)
> > > +{
> > > +ÂÂÂ struct evsel *evsel;
> > > +
> > > +ÂÂÂ pr_warning("WARNING: event cpu maps are not fully matched, "
> > > +ÂÂÂÂÂÂÂÂÂÂ "stop event grouping\n");
> > > +
> > > +ÂÂÂ evlist__for_each_entry(evlist, evsel) {
> > > +ÂÂÂÂÂÂÂ evsel->leader = evsel;
> > > +ÂÂÂÂÂÂÂ evsel->core.nr_members = 0;
> > > +ÂÂÂ }
> > > +}
> >
> > I think this is too much, we need to disable only groups with not
> > matching cpus, not all of them, how about something like this
>
> Yes, that's too much.
>
> > ÂÂÂÂÂÂÂÂ struct evsel *pos;
> >
> > ÂÂÂÂÂÂÂÂ evlist__for_each_entry(evlist, evsel) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (evsel->leader == evsel)
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ if (!cpus_map_matched(evsel->leader, evsel))
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pr_warn("Disabling group...
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ for_each_group_member(pos, evsel->leader) {
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pos->leader = pos;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ evsel->core.nr_members = 0;
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ }
> > ÂÂÂÂÂÂÂÂ }
> >
> > jirka
>
> Hmm, change "!cpus_map_matched()" to "cpus_map_matched()"? and use for_each_group_evsel() to replace 
> for_each_group_member()?
>
> How about something like following?
>
> void evlist__check_cpu_maps(struct evlist *evlist)
> {
>  ÂÂÂÂstruct evsel *evsel, *pos;
>
>  ÂÂÂÂevlist__for_each_entry(evlist, evsel) {
>  ÂÂÂÂÂÂÂ if (evsel->leader == evsel)
>  ÂÂÂÂÂÂÂÂÂÂÂ continue;
>
>  ÂÂÂÂÂÂÂ if (cpu_maps_matched(evsel->leader, evsel))
>  ÂÂÂÂÂÂÂÂÂÂÂ continue;
>
>  ÂÂÂÂÂÂÂ pr_warning("WARNING: event cpu maps are not fully matched, "
>  ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "disable group\n");
>
>  ÂÂÂÂÂÂÂ for_each_group_evsel(pos, evsel->leader) {
>  ÂÂÂÂÂÂÂÂÂÂÂ pos->leader = pos;
>  ÂÂÂÂÂÂÂÂÂÂÂ pos->core.nr_members = 0;
>  ÂÂÂÂÂÂÂ }
>
>  ÂÂÂÂÂÂÂ /*
>  ÂÂÂÂÂÂÂÂ * For core & uncore mixed event group, for example,
>  ÂÂÂÂÂÂÂÂ * '{cycles,unc_cbo_cache_lookup.any_i}',
>  ÂÂÂÂÂÂÂÂ * In evlist:
>  ÂÂÂÂÂÂÂÂ * cycles,
>  ÂÂÂÂÂÂÂÂ * unc_cbo_cache_lookup.any_i,
>  ÂÂÂÂÂÂÂÂ * unc_cbo_cache_lookup.any_i,
>  ÂÂÂÂÂÂÂÂ * unc_cbo_cache_lookup.any_i,
>  ÂÂÂÂÂÂÂÂ * unc_cbo_cache_lookup.any_i,
>  ÂÂÂÂÂÂÂÂ *
>  ÂÂÂÂÂÂÂÂ * cycles is leader and all unc_cbo_cache_lookup.any_i
>  ÂÂÂÂÂÂÂÂ * point to this leader. But for_each_group_evsel can't
>  ÂÂÂÂÂÂÂÂ * iterate all members from cycles. It only iterates
>  ÂÂÂÂÂÂÂÂ * cycles and one unc_cbo_cache_lookup.any_i. So we
>  ÂÂÂÂÂÂÂÂ * set extra evsel here.
>  ÂÂÂÂÂÂÂÂ */
>  ÂÂÂÂÂÂÂ evsel->leader = evsel;
>  ÂÂÂÂÂÂÂ evsel->core.nr_members = 0;
>  ÂÂÂÂ}
> }
>
> Thanks
> Jin Yao

Issue is found!

It looks we can't set "pos->leader = pos" in either for_each_group_member() or in 
for_each_group_evsel() because it may exit the iteration immediately.

	evlist__for_each_entry(evlist, evsel) {
		if (evsel->leader == evsel)
			continue;

		if (cpu_maps_matched(evsel->leader, evsel))
			continue;

		pr_warning("WARNING: event cpu maps are not fully matched, "
			   "disable group\n");

		for_each_group_member(pos, evsel->leader) {
			pos->leader = pos;
			pos->core.nr_members = 0;
		}

Let me use the example of '{cycles,unc_cbo_cache_lookup.any_i}' again.

In evlist:
cycles,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,
unc_cbo_cache_lookup.any_i,

When we reach the for_each_group_member at first time, evsel is the first unc_cbo_cache_lookup.any_i 
and evsel->leader is cycles. pos is same as the evsel (the first unc_cbo_cache_lookup.any_i).

Once we execute "pos->leader = pos;", it's actually "evsel->leader = evsel". So now evsel->leader is 
changed to the first unc_cbo_cache_lookup.any_i.

In next iteration, pos is the second unc_cbo_cache_lookup.any_i. pos->leader is cycles but 
unfortunately evsel->leader has been changed to the first unc_cbo_cache_lookup.any_i. So iteration 
stops immediately.

I'm now thinking if we can solve this issue by an easy way.

Thanks
Jin Yao