Re: [PATCH 2/5] perf,tools: check and re-organize evsel cpu maps

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Tue, Mar 03, 2015 at 05:09:29PM +0000, Liang, Kan escreveu:
> 
> 
> > Em Tue, Mar 03, 2015 at 01:09:29PM -0300, Arnaldo Carvalho de Melo
> > escreveu:
> > > Em Tue, Mar 03, 2015 at 03:54:43AM -0500, kan.liang@intel.com escreveu:
> > > > From: Kan Liang <kan.liang@intel.com>
> > > >
> > > > With the patch 1/5, it's possible to group read events from
> > > > different pmus. "-C" can be used to set cpu list. The cpu list may
> > > > be incompatible with pmu's cpumask.
> > > > This patch checks the event's cpu maps, and discard the incompatible
> > > > cpu maps.
> > > > event's cpu maps is saved in evsel->cpus during option parse. Then
> > > > the evlist's cpu maps is created in perf_evlist__create_maps. So the
> > > > cpu maps can be check and re-organized in perf_evlist__create_maps.
> > > > Only cpu_list need to check the cpu maps.
> > >
> > > Humm, I had something done in this area...
> > >
> > > Stephane complained about the confusion about which cpumap to use
> > with
> > > pmus, so I wrote a patch and sent an RFC, which I think I got no
> > > comments, lemme dig it...
> > 
> > Here it is, can you take a look? Stephane?
> > 
> 
> Your patch is more like my 3/5 patch. The difference is your patch force
> the evsel->cpus = evlist->cpus, if evsel->cpus == NULL. 
> My patch handle the evsel->cpus == NULL case when using it.

Idea is to use evsel->cpus always, not having to special case it and
fallback to evlist->cpus, so that we don't have to pass evlist around
that often.
 
> > @@ -1216,8 +1206,8 @@ static void print_aggr(char *prefix)
> >  		evlist__for_each(evsel_list, counter) {
> >  			val = ena = run = 0;
> >  			nr = 0;
> > -			for (cpu = 0; cpu < perf_evsel__nr_cpus(counter);
> > cpu++) {
> > -				cpu2 = perf_evsel__cpus(counter)-
> > >map[cpu];
> > +			for (cpu = 0; cpu < cpu_map__nr(counter->cpus);
> > cpu++) {
> > +				cpu2 = counter->cpus->map[cpu];
> >  				s2 = aggr_get_id(evsel_list->cpus, cpu2);
> >  				if (s2 != id)
> >  					continue;
> 
> print_aggr also need to be special handled. In the past, all events use
> evlist's cpu map,so it uses index to find the real cpu id.
> Now, event's cpu map are different. The s2 could be wrong.
> For example, evlist's cpu map is 0,4,5,18. Event's cpu map could be 0,18.
> When cpu == 1, the return of aggr_get_id must be wrong, since it
> still use index to find s2.
> My 3/5 patch introduce a function perf_evsel__get_cpumap_index
> to handle it.
> 
> Only your patch is not enough, we still need 2/5 and 4/5.
> 2/5 is used to check if the event's cpu maps are compatible as evlist's
> cpu map. For example, evlist's cpu map is 1,2,17. Event's cpu map
> could be 0,18. We can error out earlier.
> 4/5 is used to special handle the open and mmap. We need to do
> the same thing as what we did in print_aggr.

I'll try to go thru this tomorrow, thanks for checking.

- Arnaldo