Re: [PATCH] perf report: Fix invalid memory accessing

[Jiri Olsa <jolsa@redhat.com>]

On Tue, Sep 08, 2015 at 04:12:55PM +0800, Wangnan (F) wrote:
> 
> 
> 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

hum, that should be the case anyway.. features are read before events

> 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?

perf_event__preprocess_sample is called for each sample,
while build_cpu_topo is part of storing topology feature

jirka