> > On 2015/9/8 15:37, Jiri Olsa wrote: > > On Mon, Sep 07, 2015 at 09:27:26PM +0800, Wangnan (F) wrote: > > > > SNIP > > > >> I found the problem. > >> > >> perf relies on build_cpu_topology() to fetch CPU_TOPOLOGY from sysfs. > >> It depend on the existance of > >> > >> /sys/devices/system/cpu/cpu%d/topology/core_siblings_list > >> > >> However, CPU can be canceled by hotcpu subsystem. After that the > >> directory of /sys/devices/system/cpu/cpu%d/topology is gone, which > >> causes perf's > >> write_cpu_topology() --> uild_cpu_topology() to fail, result in the > >> above perf.data. > >> > >> So I think my patch is required. > > no question there.. I just meant it should be placed in > > perf_event__preprocess_sample function with the rest of the 'al' > > initialization, like in the patch below? > > > > it does not compile, because there're many places calling it and it'd > > need changing all callers to pass env, which seems to require more > > changes.. > > > > also I'm not sure about removing: > > - al->socket = cpu_map__get_socket_id(al->cpu); > > > > > > Does any command actually need this initialized from current system? > > > > thanks, > > jirka > > > > > > --- > > diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c index > > 0bf8c9889fc0..3339d2579bfc 100644 > > --- a/tools/perf/util/event.c > > +++ b/tools/perf/util/event.c > > @@ -990,7 +990,8 @@ void thread__find_addr_location(struct thread > *thread, > > int perf_event__preprocess_sample(const union perf_event *event, > > struct machine *machine, > > struct addr_location *al, > > - struct perf_sample *sample) > > + struct perf_sample *sample, > > + struct perf_env *env) > > { > > u8 cpumode = event->header.misc & > PERF_RECORD_MISC_CPUMODE_MASK; > > struct thread *thread = machine__findnew_thread(machine, > > sample->pid, @@ -1021,7 +1022,10 @@ int > > perf_event__preprocess_sample(const union perf_event *event, > > > > al->sym = NULL; > > al->cpu = sample->cpu; > > - al->socket = cpu_map__get_socket_id(al->cpu); > > + > > + al.socket = -1; > > + if (env->cpu && al->cpu >= 0) > > + al.socket = env->cpu[al->cpu].socket_id; > > > > if (al->map) { > > struct dso *dso = al->map->dso; > > Now I understand your suggestion. You mean we can build env->cpu > before processing the first sample, then init al.socket using that map > instead of calling cpu_map__get_socket_id() unconditionally in an ad-hoc > way. > > And I have another question that, since build_cpu_topo() and > perf_event__preprocess_sample() are more or less doing similar things, > why we need both of them? > > Then we need more code for this bug... > > Kan Liang, do you have any suggestion? > >
I think Jirka's way is better. We should handle al.socket in one place for all tools. Now we already read env from file in perf_session__new for perf report. I think we only need to update env in perf_session__new for other tools. So perf_event__preprocess_sample can use it. Thanks, Kan