lkml.org 
[lkml]   [2015]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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.




\
 
 \ /
  Last update: 2015-09-08 10:41    [W:0.096 / U:0.340 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site