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

[Liang, Kan]

> 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@xxxxxxxxx escreveu:
> > > From: Kan Liang <kan.liang@xxxxxxxxx>
> > >
> > > 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.

> @@ -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.

Thanks,
Kan

 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/