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

[pi3orama]

发自我的 iPhone

> 在 2015年9月8日，下午9:13，Jiri Olsa <jolsa@xxxxxxxxxx> 写道：
> 
>> 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
Sorry, what I wanted to say should be:
cpu_map__get_socket_id() and  build_cpu_topo()...

> 
> jirka

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/