[PATCH] perf report: Fix invalid memory accessing

[PATCH] perf report: Fix invalid memory accessing

[Wang Nan]

Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add
processor socket info to hist_entry and addr_location) reads env->cpu
array for each sample using index al.cpu. However, al.cpu can be -1 if
sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if
feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu
before setting al.socket.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Kan Liang <kan.liang@intel.com>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---

Although theoretically CPU_TOPOLOGY feature should always be selected by
'perf record', I did generate a perf.data without that feature. It has
header like this:

 # perf report -i ./bad.perf.data  --header-only          
 # ========
 # captured on: Thu Jan  8 09:30:15 2009
 # hostname : localhost
 # os release : 3.10.49-gd672fc4
 # perf version : 4.2.gc9df
 # arch : aarch64
 # nrcpus online : 8
 # nrcpus avail : 8
 # total memory : 1850768 kB
 # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 
 # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
 # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
 # pmu mappings: cpu = 4, software = 1, tracepoint = 2
 # ========
 #

It should be:

 # ========
 # captured on: Thu Jan  8 11:26:41 2009
 ...
 # HEADER_CPU_TOPOLOGY info available, use -I to display
 # pmu mappings: cpu = 4, software = 1, tracepoint = 2
 # ========

However, bad perf.data appears randomly. I can't stably reproduce it, so I
guess there might have another invalid memory accessing.

---
 tools/perf/builtin-report.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 4b43245..16d097d 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool,
 		return -1;
 	}
 
-	/* read socket id from perf.data for perf report */
-	al.socket = env->cpu[al.cpu].socket_id;
+	/*
+	 * read socket id from perf.data for perf report
+	 * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this
+	 * sample.
+	 * env->cpu is invalid if CPU_TOPOLOGY feature is not set in
+	 * header.
+	 */
+	al.socket = -1;
+	if (env->cpu && al.cpu >= 0)
+		al.socket = env->cpu[al.cpu].socket_id;
 
 	if (rep->hide_unresolved && al.sym == NULL)
 		goto out_put;
-- 
1.8.3.4

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

[Jiri Olsa]

On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote:
> Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add
> processor socket info to hist_entry and addr_location) reads env->cpu
> array for each sample using index al.cpu. However, al.cpu can be -1 if
> sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if
> feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu
> before setting al.socket.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Kan Liang <kan.liang@intel.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Andi Kleen <ak@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
> 
> Although theoretically CPU_TOPOLOGY feature should always be selected by
> 'perf record', I did generate a perf.data without that feature. It has
> header like this:
> 
>  # perf report -i ./bad.perf.data  --header-only          
>  # ========
>  # captured on: Thu Jan  8 09:30:15 2009
>  # hostname : localhost
>  # os release : 3.10.49-gd672fc4
>  # perf version : 4.2.gc9df
>  # arch : aarch64
>  # nrcpus online : 8
>  # nrcpus avail : 8
>  # total memory : 1850768 kB
>  # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5 
>  # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
>  # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
>  # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>  # ========
>  #
> 
> It should be:
> 
>  # ========
>  # captured on: Thu Jan  8 11:26:41 2009
>  ...
>  # HEADER_CPU_TOPOLOGY info available, use -I to display
>  # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>  # ========
> 
> However, bad perf.data appears randomly. I can't stably reproduce it, so I
> guess there might have another invalid memory accessing.
> 
> ---
>  tools/perf/builtin-report.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 4b43245..16d097d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool,
>  		return -1;
>  	}
>  
> -	/* read socket id from perf.data for perf report */
> -	al.socket = env->cpu[al.cpu].socket_id;
> +	/*
> +	 * read socket id from perf.data for perf report
> +	 * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this
> +	 * sample.
> +	 * env->cpu is invalid if CPU_TOPOLOGY feature is not set in
> +	 * header.
> +	 */
> +	al.socket = -1;
> +	if (env->cpu && al.cpu >= 0)
> +		al.socket = env->cpu[al.cpu].socket_id;

perf_event__preprocess_sample initializes al.socket from current system
do we want to move this over there?

also this change is just report specific, and we could need
this in at least perf top

jirka

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

[Wangnan (F)]

On 2015/9/7 21:03, Jiri Olsa wrote:
> On Mon, Sep 07, 2015 at 12:51:55PM +0000, Wang Nan wrote:
>> Commit e1e499aba570a2ea84d29822b7ea637ac41d9a51 (perf tools: Add
>> processor socket info to hist_entry and addr_location) reads env->cpu
>> array for each sample using index al.cpu. However, al.cpu can be -1 if
>> sample doesn't select PERF_SAMPLE_CPU. Also, env->cpu can be invalid if
>> feature CPU_TOPOLOGY not selected. We should validate env->cpu and al.cpu
>> before setting al.socket.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Kan Liang <kan.liang@intel.com>
>> Cc: Adrian Hunter <adrian.hunter@intel.com>
>> Cc: Andi Kleen <ak@linux.intel.com>
>> Cc: Jiri Olsa <jolsa@kernel.org>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Stephane Eranian <eranian@google.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> ---
>>
>> Although theoretically CPU_TOPOLOGY feature should always be selected by
>> 'perf record', I did generate a perf.data without that feature. It has
>> header like this:
>>
>>   # perf report -i ./bad.perf.data  --header-only
>>   # ========
>>   # captured on: Thu Jan  8 09:30:15 2009
>>   # hostname : localhost
>>   # os release : 3.10.49-gd672fc4
>>   # perf version : 4.2.gc9df
>>   # arch : aarch64
>>   # nrcpus online : 8
>>   # nrcpus avail : 8
>>   # total memory : 1850768 kB
>>   # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5
>>   # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
>>   # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
>>   # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>>   # ========
>>   #
>>
>> It should be:
>>
>>   # ========
>>   # captured on: Thu Jan  8 11:26:41 2009
>>   ...
>>   # HEADER_CPU_TOPOLOGY info available, use -I to display
>>   # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>>   # ========
>>
>> However, bad perf.data appears randomly. I can't stably reproduce it, so I
>> guess there might have another invalid memory accessing.
>>
>> ---
>>   tools/perf/builtin-report.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
>> index 4b43245..16d097d 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -158,8 +158,16 @@ static int process_sample_event(struct perf_tool *tool,
>>   		return -1;
>>   	}
>>   
>> -	/* read socket id from perf.data for perf report */
>> -	al.socket = env->cpu[al.cpu].socket_id;
>> +	/*
>> +	 * read socket id from perf.data for perf report
>> +	 * al.cpu is invalid if PERF_SAMPLE_CPU is not selected by this
>> +	 * sample.
>> +	 * env->cpu is invalid if CPU_TOPOLOGY feature is not set in
>> +	 * header.
>> +	 */
>> +	al.socket = -1;
>> +	if (env->cpu && al.cpu >= 0)
>> +		al.socket = env->cpu[al.cpu].socket_id;
> perf_event__preprocess_sample initializes al.socket from current system

No. For 'perf report' it initializes al.cpu from sample.


Commit message of e1e499aba570a2ea84d29822b7ea637ac41d9a51:

     Finor 'perf report', the socket id info is from perf.data.

     For others, the socket id info is from current system.

And at least checking of env->cpu is essential. I'm looking the problem 
I reported.
Looks like build_cpu_topology() is possible to fail.

Thank you.
> do we want to move this over there?
>
> also this change is just report specific, and we could need
> this in at least perf top
>
> jirka

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

[Wangnan (F)]

On 2015/9/7 20:51, Wang Nan wrote:


[SNIP]

> Although theoretically CPU_TOPOLOGY feature should always be selected by
> 'perf record', I did generate a perf.data without that feature. It has
> header like this:
>
>   # perf report -i ./bad.perf.data  --header-only
>   # ========
>   # captured on: Thu Jan  8 09:30:15 2009
>   # hostname : localhost
>   # os release : 3.10.49-gd672fc4
>   # perf version : 4.2.gc9df
>   # arch : aarch64
>   # nrcpus online : 8
>   # nrcpus avail : 8
>   # total memory : 1850768 kB
>   # cmdline : /system/bin/perf record -e sync:sync_timeline -e kgsl:kgsl_register_event -g -a sleep 5
>   # event : name = sync:sync_timeline, , id = { 1107, 1108, 1109, 1110, 1111, 1112 }, type = 2, size = 112, config = 0x3e7, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, mmap = 1, comm = 1, task = 1, sample_id_all = 1, exclude_guest = 1, mmap2 = 1, comm_exec = 1
>   # event : name = kgsl:kgsl_register_event, , id = { 1113, 1114, 1115, 1116, 1117, 1118 }, type = 2, size = 112, config = 0x350, { sample_period, sample_freq } = 1, sample_type = IP|TID|TIME|CALLCHAIN|ID|CPU|PERIOD|RAW, read_format = ID, disabled = 1, inherit = 1, sample_id_all = 1, exclude_guest = 1
>   # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>   # ========
>   #
>
> It should be:
>
>   # ========
>   # captured on: Thu Jan  8 11:26:41 2009
>   ...
>   # HEADER_CPU_TOPOLOGY info available, use -I to display
>   # pmu mappings: cpu = 4, software = 1, tracepoint = 2
>   # ========
>
> However, bad perf.data appears randomly. I can't stably reproduce it, so I
> guess there might have another invalid memory accessing.
>
>

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.

Thank you.

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

[Jiri Olsa]

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;

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

[Wangnan (F)]

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.

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

[Jiri Olsa]

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

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

[pi3orama]

发自我的 iPhone

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

> 
> jirka

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

[Jiri Olsa]

On Tue, Sep 08, 2015 at 09:16:31PM +0800, pi3orama wrote:

SNIP

> >> 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()...

cpu_map__get_socket_id is also used by perf stat

jirka

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

[Liang, Kan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3190 bytes --]



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

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 09:37:47AM +0200, Jiri Olsa escreveu:
> 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..

Humm, I think that we can have a pointer to the current perf_env, be it
from the current machine, or from the machine environment in the
perf.data file in struct machine, that way we don't need to change that
function prototype, I'm prototyping this now, will post a patch.

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

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

[Jiri Olsa]

On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Tue, Sep 08, 2015 at 09:37:47AM +0200, Jiri Olsa escreveu:
> > 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..
> 
> Humm, I think that we can have a pointer to the current perf_env, be it
> from the current machine, or from the machine environment in the
> perf.data file in struct machine, that way we don't need to change that
> function prototype, I'm prototyping this now, will post a patch.

I was thinking of that.. but the perf_env is actualyl related to the
perf.data not to the current machine.. I think it should be part of
the session or perf_header

jirka

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu:
> > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote:
> > > Humm, I think that we can have a pointer to the current perf_env, be it
> > > from the current machine, or from the machine environment in the
> > > perf.data file in struct machine, that way we don't need to change that
> > > function prototype, I'm prototyping this now, will post a patch.
> > 
> > I was thinking of that.. but the perf_env is actualyl related to the
> > perf.data not to the current machine.. I think it should be part of
> > the session or perf_header
> 
> But what if I want to trace only events that take place in some specific
> socket, i.e. what to do when perf_session is not used at all and we are
> not dealing with any header, since there are no files involved?

So, this is the continuation of this patch:

commit ce80d3bef9ff97638ca57a5659ef6ad356f35047
Author: Kan Liang <kan.liang@intel.com>
Date:   Fri Aug 28 05:48:04 2015 -0400

    perf tools: Rename perf_session_env to perf_env
    
    As it is not necessarily tied to a perf.data file and needs using in
    places where a perf_session is not required.
    
    Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

-----------------------------

perf_env not necessarily is related to a perf.data file, we need even to
move it away from header.h.

I am looking now at where to populate perf_env and set it to
machine->env when no perf.data files are being accessed.

I  should have seen the use cpu_map__get_socket_id() in
perf_event__preprocess_sample(), that is unnaceptable, as it will parse
that file for each sample, right ;-\

Right now we don't have that much use for the other fields in
'perf_env', just for the CPU topology information, that we will set in
addr_location for each sample, but we can have uses for that later,
think about a TUI interface for 'perf trace' where we will show what was
the command line, etc.

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu:
> > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Humm, I think that we can have a pointer to the current perf_env, be it
> > > > from the current machine, or from the machine environment in the
> > > > perf.data file in struct machine, that way we don't need to change that
> > > > function prototype, I'm prototyping this now, will post a patch.
> > > 
> > > I was thinking of that.. but the perf_env is actualyl related to the
> > > perf.data not to the current machine.. I think it should be part of
> > > the session or perf_header
> > 
> > But what if I want to trace only events that take place in some specific
> > socket, i.e. what to do when perf_session is not used at all and we are
> > not dealing with any header, since there are no files involved?
> 
> So, this is the continuation of this patch:
> 
> commit ce80d3bef9ff97638ca57a5659ef6ad356f35047
> Author: Kan Liang <kan.liang@intel.com>
> Date:   Fri Aug 28 05:48:04 2015 -0400
> 
>     perf tools: Rename perf_session_env to perf_env
>     
>     As it is not necessarily tied to a perf.data file and needs using in
>     places where a perf_session is not required.
>     
>     Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> -----------------------------
> 
> perf_env not necessarily is related to a perf.data file, we need even to
> move it away from header.h.
> 
> I am looking now at where to populate perf_env and set it to
> machine->env when no perf.data files are being accessed.
> 
> I  should have seen the use cpu_map__get_socket_id() in
> perf_event__preprocess_sample(), that is unnaceptable, as it will parse
> that file for each sample, right ;-\
> 
> Right now we don't have that much use for the other fields in
> 'perf_env', just for the CPU topology information, that we will set in
> addr_location for each sample, but we can have uses for that later,
> think about a TUI interface for 'perf trace' where we will show what was
> the command line, etc.

Argh, so in the patch introducing this al.socket thing it would first
parse the value from the current system, reading sysfs, etc, then, in
the 'report' case it would just throw this information away:

-       /* read socket id from perf.data for perf report */
-       al.socket = env->cpu[al.cpu].socket_id;

We really should do this in perf_event__preprocess_sample() and read the
topology information just once, probably using the same routine that
creates the perf.data file env record.

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 08, 2015 at 12:49:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Sep 08, 2015 at 05:34:56PM +0200, Jiri Olsa escreveu:
> > > > On Tue, Sep 08, 2015 at 12:18:13PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Humm, I think that we can have a pointer to the current perf_env, be it
> > > > > from the current machine, or from the machine environment in the
> > > > > perf.data file in struct machine, that way we don't need to change that
> > > > > function prototype, I'm prototyping this now, will post a patch.
> > > > 
> > > > I was thinking of that.. but the perf_env is actualyl related to the
> > > > perf.data not to the current machine.. I think it should be part of
> > > > the session or perf_header
> > > 
> > > But what if I want to trace only events that take place in some specific
> > > socket, i.e. what to do when perf_session is not used at all and we are
> > > not dealing with any header, since there are no files involved?
> > 
> > So, this is the continuation of this patch:
> > 
> > commit ce80d3bef9ff97638ca57a5659ef6ad356f35047
> > Author: Kan Liang <kan.liang@intel.com>
> > Date:   Fri Aug 28 05:48:04 2015 -0400
> > 
> >     perf tools: Rename perf_session_env to perf_env
> >     
> >     As it is not necessarily tied to a perf.data file and needs using in
> >     places where a perf_session is not required.
> >     
> >     Suggested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > 
> > -----------------------------
> > 
> > perf_env not necessarily is related to a perf.data file, we need even to
> > move it away from header.h.
> > 
> > I am looking now at where to populate perf_env and set it to
> > machine->env when no perf.data files are being accessed.
> > 
> > I  should have seen the use cpu_map__get_socket_id() in
> > perf_event__preprocess_sample(), that is unnaceptable, as it will parse
> > that file for each sample, right ;-\
> > 
> > Right now we don't have that much use for the other fields in
> > 'perf_env', just for the CPU topology information, that we will set in
> > addr_location for each sample, but we can have uses for that later,
> > think about a TUI interface for 'perf trace' where we will show what was
> > the command line, etc.
> 
> Argh, so in the patch introducing this al.socket thing it would first
> parse the value from the current system, reading sysfs, etc, then, in
> the 'report' case it would just throw this information away:
> 
> -       /* read socket id from perf.data for perf report */
> -       al.socket = env->cpu[al.cpu].socket_id;
> 
> We really should do this in perf_event__preprocess_sample() and read the
> topology information just once, probably using the same routine that
> creates the perf.data file env record.

Lunch break, but I'll continue the work I started at
https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Tue, Sep 08, 2015 at 01:35:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Right now we don't have that much use for the other fields in
> > > 'perf_env', just for the CPU topology information, that we will set in
> > > addr_location for each sample, but we can have uses for that later,
> > > think about a TUI interface for 'perf trace' where we will show what was
> > > the command line, etc.

> > Argh, so in the patch introducing this al.socket thing it would first
> > parse the value from the current system, reading sysfs, etc, then, in
> > the 'report' case it would just throw this information away:

> > -       /* read socket id from perf.data for perf report */
> > -       al.socket = env->cpu[al.cpu].socket_id;

> > We really should do this in perf_event__preprocess_sample() and read the
> > topology information just once, probably using the same routine that
> > creates the perf.data file env record.
 
> Lunch break, but I'll continue the work I started at
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core

Ok, I have them at my perf/env branch:

https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/env

    12  eefb0dbb1aa9 perf evsel: Remove forward declaration of 'struct perf_evlist'
    11  8d9c09de8465 Revert "perf evsel: Add a backpointer to the evlist a evsel is in"
    10  e3611f9dbced perf report: Do not blindly use env->cpu[al.cpu].socket_id
     9  6f9ad1cd48e8 perf event: Use machine->env to find the cpu -> socket mapping
     8  7b157d3eb7b4 perf machine: Add pointer to sample's environment
     7  cc393318b0c0 perf hists browser: Fixup the "cpu" column width calculation
     6  bc413472a275 perf top: Cache the cpu topology info when "-s socket" is used
     5  908f1939ad52 perf sort: Set flag stating if the "socket" key is being used
     4  81956a8be745 perf env: Introduce read_cpu_topology_map() method
     3  7d76ec2a6f18 perf env: Adopt perf_header__set_cmdline
     2  371495478108 perf env: Rename some leftovers from rename to perf_env
     1  7b1a26d8cff7 perf env: Move perf_env out of header.h and session.c into separate object

Now I see that one more probably is needed, the one adding the ->env
backpointer to struct evlist.

The ones fixing the problem are #9 and #10, the rest is infrastructure needed
for those fixes to work, and some are not strictly needed but as I was working
on it, couldn't resist cleaning up.

I'll test this some more, add one or more cleanups and post for review, if all
goes well, tomorrow I'll push it to Ingo.

- Arnaldo

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

[Arnaldo Carvalho de Melo]

Em Wed, Sep 09, 2015 at 01:06:40PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Tue, Sep 08, 2015 at 01:35:10PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Tue, Sep 08, 2015 at 01:13:59PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > Em Tue, Sep 08, 2015 at 12:58:31PM -0300, Arnaldo Carvalho de Melo escreveu:
> > > > Right now we don't have that much use for the other fields in
> > > > 'perf_env', just for the CPU topology information, that we will set in
> > > > addr_location for each sample, but we can have uses for that later,
> > > > think about a TUI interface for 'perf trace' where we will show what was
> > > > the command line, etc.
> 
> > > Argh, so in the patch introducing this al.socket thing it would first
> > > parse the value from the current system, reading sysfs, etc, then, in
> > > the 'report' case it would just throw this information away:
> 
> > > -       /* read socket id from perf.data for perf report */
> > > -       al.socket = env->cpu[al.cpu].socket_id;
> 
> > > We really should do this in perf_event__preprocess_sample() and read the
> > > topology information just once, probably using the same routine that
> > > creates the perf.data file env record.
>  
> > Lunch break, but I'll continue the work I started at
> > https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=tmp.perf/core
> 
> Ok, I have them at my perf/env branch:
> 
> https://git.kernel.org/cgit/linux/kernel/git/acme/linux.git/commit/?h=perf/env
> 
>     12  eefb0dbb1aa9 perf evsel: Remove forward declaration of 'struct perf_evlist'
>     11  8d9c09de8465 Revert "perf evsel: Add a backpointer to the evlist a evsel is in"
>     10  e3611f9dbced perf report: Do not blindly use env->cpu[al.cpu].socket_id
>      9  6f9ad1cd48e8 perf event: Use machine->env to find the cpu -> socket mapping
>      8  7b157d3eb7b4 perf machine: Add pointer to sample's environment
>      7  cc393318b0c0 perf hists browser: Fixup the "cpu" column width calculation
>      6  bc413472a275 perf top: Cache the cpu topology info when "-s socket" is used
>      5  908f1939ad52 perf sort: Set flag stating if the "socket" key is being used
>      4  81956a8be745 perf env: Introduce read_cpu_topology_map() method
>      3  7d76ec2a6f18 perf env: Adopt perf_header__set_cmdline
>      2  371495478108 perf env: Rename some leftovers from rename to perf_env
>      1  7b1a26d8cff7 perf env: Move perf_env out of header.h and session.c into separate object
> 
> Now I see that one more probably is needed, the one adding the ->env
> backpointer to struct evlist.

Ok, one more: 'perf top' needs --socket-filter, will do after lunch.

- Arnaldo
 
> The ones fixing the problem are #9 and #10, the rest is infrastructure needed
> for those fixes to work, and some are not strictly needed but as I was working
> on it, couldn't resist cleaning up.
> 
> I'll test this some more, add one or more cleanups and post for review, if all
> goes well, tomorrow I'll push it to Ingo.
> 
> - Arnaldo