Messages in this thread | | | Date | Fri, 4 Sep 2015 11:48:15 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH] perf tools: Fix gaps propagating maps |
| |
Em Fri, Sep 04, 2015 at 04:42:30PM +0300, Adrian Hunter escreveu: > On 04/09/15 16:28, Arnaldo Carvalho de Melo wrote: > > Em Fri, Sep 04, 2015 at 03:15:54PM +0300, Adrian Hunter escreveu: > >> A perf_evsel is a selected event containing the perf_event_attr > >> that is passed to perf_event_open(). A perf_evlist is a collection > >> of perf_evsel's. A perf_evlist also has lists of cpus and threads > >> (pids) on which to open the event. These lists are called 'maps' > >> and this patch is about how those 'maps' are propagated from the > >>> perf_evlist to the perf_evsels. > > > > Can't this be broken up in multiple patches, for instance this: > > Ok, might not be until next week though.
Take your time, it seems that so far the only problem is with Intel PT, with adding that sched_switch after the propagation was done, no?
And with kernel with PERF_RECORD_SWITCH, that I am pushing that other patch from you, to the support using it replacing sched:sched_Switch in the tooling side reduces the impact of this problem, right?
> > int perf_evlist__create_maps(struct perf_evlist *evlist, struct > > target *target) > > { > > + if (evlist->threads || evlist->cpus) > > + return -1; > > +
> Or you could just drop that chunk.
> > Looks like a fix that could be separated. Also FOO__propagate(.., false) > > to do the opposite of propagate seems confusing, how about > > FOO__unpropagate() if that verb exists? :-)
> Ok
> > Also, when unpropagating, you do: > > > > if (evsel->cpus == evlist->cpus) { > > cpu_map__put(evsel->cpus); > > evsel->cpus = NULL; > > } > > > > What if the PMU code _set_ it to the same cpus as in evlist->cpus, but > > now we're unpropagating to set to another CPU, in this case we will be > > changing the PMU setting with a new one. I.e. when a PMU sets it it > > should be sticky, no? > > We are comparing the pointer, so that won't happen unless the PMU actually > does evsel->cpus = evlist->cpus which seems unlikely.
> > I.e. we would have to know, in the evsel, if evsel->cpus was set by the > > PMU or any other future entity expecting this behaviour, so that we > > don't touch it, i.e. testing (evsel->cpus != evlist->cpus) when > > unpropagating doesn't seem to cut, right? > > I think the pointer comparison covers that. i.e. the pointers won't be the > same even if the cpus are.
It makes it more unlikely, yes.
I think that to be safe we should have a evsel->sticky_{cpu,threads}, well, at least the evsel->sticky_cpus seems needed due to the PMU usecase.
- Arnaldo
| |