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

[Liang, Kan]

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