Messages in this thread | | | Date | Tue, 8 Sep 2015 16:12:55 +0800 | From | "Wangnan (F)" <> | Subject | Re: [PATCH] perf report: Fix invalid memory accessing |
| |
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?
Thank you.
| |