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

[Liang, Kan]

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

Hi Arnaldo,

Have you got a chance to check the patch?

Thanks,
Kan



> 
> - Arnaldo
--
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/