From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754842AbbIHNN4 (ORCPT ); Tue, 8 Sep 2015 09:13:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45921 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754670AbbIHNNx (ORCPT ); Tue, 8 Sep 2015 09:13:53 -0400 Date: Tue, 8 Sep 2015 15:13:49 +0200 From: Jiri Olsa To: "Wangnan (F)" Cc: kan.liang@intel.com, acme@kernel.org, linux-kernel@vger.kernel.org, lizefan@huawei.com, pi3orama@163.com, Adrian Hunter , Andi Kleen , Jiri Olsa , Namhyung Kim , Stephane Eranian , Arnaldo Carvalho de Melo Subject: Re: [PATCH] perf report: Fix invalid memory accessing Message-ID: <20150908131349.GD2038@krava.brq.redhat.com> References: <1441630315-189525-1-git-send-email-wangnan0@huawei.com> <55ED90BE.9090706@huawei.com> <20150908073747.GA2038@krava.brq.redhat.com> <55EE9887.9030304@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55EE9887.9030304@huawei.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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