Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1220155 > unrolled thread

[PATCH] perf report: Fix invalid memory accessing

Started byWang Nan <wangnan0@huawei.com>
First post2015-09-07 15:00 +0200
Last post2015-09-09 18:50 +0200
Articles 17 — 7 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] perf report: Fix invalid memory accessing Wang Nan <wangnan0@huawei.com> - 2015-09-07 15:00 +0200
    Re: [PATCH] perf report: Fix invalid memory accessing Jiri Olsa <jolsa@redhat.com> - 2015-09-07 15:10 +0200
      Re: [PATCH] perf report: Fix invalid memory accessing "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-07 15:10 +0200
    Re: [PATCH] perf report: Fix invalid memory accessing "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-07 15:30 +0200
      Re: [PATCH] perf report: Fix invalid memory accessing Jiri Olsa <jolsa@redhat.com> - 2015-09-08 09:40 +0200
        Re: [PATCH] perf report: Fix invalid memory accessing "Wangnan (F)" <wangnan0@huawei.com> - 2015-09-08 10:20 +0200
          Re: [PATCH] perf report: Fix invalid memory accessing Jiri Olsa <jolsa@redhat.com> - 2015-09-08 15:20 +0200
            Re: [PATCH] perf report: Fix invalid memory accessing pi3orama <pi3orama@163.com> - 2015-09-08 15:30 +0200
              Re: [PATCH] perf report: Fix invalid memory accessing Jiri Olsa <jolsa@redhat.com> - 2015-09-08 15:40 +0200
          RE: [PATCH] perf report: Fix invalid memory accessing "Liang, Kan" <kan.liang@intel.com> - 2015-09-08 15:50 +0200
        Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@redhat.com> - 2015-09-08 17:20 +0200
          Re: [PATCH] perf report: Fix invalid memory accessing Jiri Olsa <jolsa@redhat.com> - 2015-09-08 17:40 +0200
            Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-08 18:00 +0200
              Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-08 18:20 +0200
                Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-08 18:40 +0200
                  Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-09 18:10 +0200
                    Re: [PATCH] perf report: Fix invalid memory accessing Arnaldo Carvalho de Melo <acme@kernel.org> - 2015-09-09 18:50 +0200

#1220155 — [PATCH] perf report: Fix invalid memory accessing

FromWang Nan <wangnan0@huawei.com>
Date2015-09-07 15:00 +0200
Subject[PATCH] perf report: Fix invalid memory accessing
Message-ID<q64bE-60R-5@gated-at.bofh.it>
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

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

[toc] | [next] | [standalone]

#1220161

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-07 15:10 +0200
Message-ID<q64lj-6rt-15@gated-at.bofh.it>
In reply to#1220155
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220162

From"Wangnan (F)" <wangnan0@huawei.com>
Date2015-09-07 15:10 +0200
Message-ID<q64lj-6rt-17@gated-at.bofh.it>
In reply to#1220161

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


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

[toc] | [prev] | [next] | [standalone]

#1220171

From"Wangnan (F)" <wangnan0@huawei.com>
Date2015-09-07 15:30 +0200
Message-ID<q64EG-6NN-17@gated-at.bofh.it>
In reply to#1220155

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.


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

[toc] | [prev] | [next] | [standalone]

#1220546

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-08 09:40 +0200
Message-ID<q6lFv-63O-1@gated-at.bofh.it>
In reply to#1220171
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220589

From"Wangnan (F)" <wangnan0@huawei.com>
Date2015-09-08 10:20 +0200
Message-ID<q6mie-72x-27@gated-at.bofh.it>
In reply to#1220546

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.


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

[toc] | [prev] | [next] | [standalone]

#1220777

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-08 15:20 +0200
Message-ID<q6qYy-5iH-27@gated-at.bofh.it>
In reply to#1220589
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220786

Frompi3orama <pi3orama@163.com>
Date2015-09-08 15:30 +0200
Message-ID<q6r8f-5u9-25@gated-at.bofh.it>
In reply to#1220777

发自我的 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

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

[toc] | [prev] | [next] | [standalone]

#1220789

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-08 15:40 +0200
Message-ID<q6rhT-5Fw-9@gated-at.bofh.it>
In reply to#1220786
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220796

From"Liang, Kan" <kan.liang@intel.com>
Date2015-09-08 15:50 +0200
Message-ID<q6rrA-5QO-5@gated-at.bofh.it>
In reply to#1220589
DQoNCj4gDQo+IE9uIDIwMTUvOS84IDE1OjM3LCBKaXJpIE9sc2Egd3JvdGU6DQo+ID4gT24gTW9u
LCBTZXAgMDcsIDIwMTUgYXQgMDk6Mjc6MjZQTSArMDgwMCwgV2FuZ25hbiAoRikgd3JvdGU6DQo+
ID4NCj4gPiBTTklQDQo+ID4NCj4gPj4gSSBmb3VuZCB0aGUgcHJvYmxlbS4NCj4gPj4NCj4gPj4g
cGVyZiByZWxpZXMgb24gYnVpbGRfY3B1X3RvcG9sb2d5KCkgdG8gZmV0Y2ggQ1BVX1RPUE9MT0dZ
IGZyb20gc3lzZnMuDQo+ID4+IEl0IGRlcGVuZCBvbiB0aGUgZXhpc3RhbmNlIG9mDQo+ID4+DQo+
ID4+IC9zeXMvZGV2aWNlcy9zeXN0ZW0vY3B1L2NwdSVkL3RvcG9sb2d5L2NvcmVfc2libGluZ3Nf
bGlzdA0KPiA+Pg0KPiA+PiBIb3dldmVyLCBDUFUgY2FuIGJlIGNhbmNlbGVkIGJ5IGhvdGNwdSBz
dWJzeXN0ZW0uIEFmdGVyIHRoYXQgdGhlDQo+ID4+IGRpcmVjdG9yeSBvZiAvc3lzL2RldmljZXMv
c3lzdGVtL2NwdS9jcHUlZC90b3BvbG9neSBpcyBnb25lLCB3aGljaA0KPiA+PiBjYXVzZXMgcGVy
ZidzDQo+ID4+IHdyaXRlX2NwdV90b3BvbG9neSgpIC0tPiB1aWxkX2NwdV90b3BvbG9neSgpIHRv
IGZhaWwsIHJlc3VsdCBpbiB0aGUNCj4gPj4gYWJvdmUgcGVyZi5kYXRhLg0KPiA+Pg0KPiA+PiBT
byBJIHRoaW5rIG15IHBhdGNoIGlzIHJlcXVpcmVkLg0KPiA+IG5vIHF1ZXN0aW9uIHRoZXJlLi4g
SSBqdXN0IG1lYW50IGl0IHNob3VsZCBiZSBwbGFjZWQgaW4NCj4gPiBwZXJmX2V2ZW50X19wcmVw
cm9jZXNzX3NhbXBsZSBmdW5jdGlvbiB3aXRoIHRoZSByZXN0IG9mIHRoZSAnYWwnDQo+ID4gaW5p
dGlhbGl6YXRpb24sIGxpa2UgaW4gdGhlIHBhdGNoIGJlbG93Pw0KPiA+DQo+ID4gaXQgZG9lcyBu
b3QgY29tcGlsZSwgYmVjYXVzZSB0aGVyZSdyZSBtYW55IHBsYWNlcyBjYWxsaW5nIGl0IGFuZCBp
dCdkDQo+ID4gbmVlZCBjaGFuZ2luZyBhbGwgY2FsbGVycyB0byBwYXNzIGVudiwgd2hpY2ggc2Vl
bXMgdG8gcmVxdWlyZSBtb3JlDQo+ID4gY2hhbmdlcy4uDQo+ID4NCj4gPiBhbHNvIEknbSBub3Qg
c3VyZSBhYm91dCByZW1vdmluZzoNCj4gPiAtCWFsLT5zb2NrZXQgPSBjcHVfbWFwX19nZXRfc29j
a2V0X2lkKGFsLT5jcHUpOw0KPiA+DQo+ID4NCj4gPiBEb2VzIGFueSBjb21tYW5kIGFjdHVhbGx5
IG5lZWQgdGhpcyBpbml0aWFsaXplZCBmcm9tIGN1cnJlbnQgc3lzdGVtPw0KPiA+DQo+ID4gdGhh
bmtzLA0KPiA+IGppcmthDQo+ID4NCj4gPg0KPiA+IC0tLQ0KPiA+IGRpZmYgLS1naXQgYS90b29s
cy9wZXJmL3V0aWwvZXZlbnQuYyBiL3Rvb2xzL3BlcmYvdXRpbC9ldmVudC5jIGluZGV4DQo+ID4g
MGJmOGM5ODg5ZmMwLi4zMzM5ZDI1NzliZmMgMTAwNjQ0DQo+ID4gLS0tIGEvdG9vbHMvcGVyZi91
dGlsL2V2ZW50LmMNCj4gPiArKysgYi90b29scy9wZXJmL3V0aWwvZXZlbnQuYw0KPiA+IEBAIC05
OTAsNyArOTkwLDggQEAgdm9pZCB0aHJlYWRfX2ZpbmRfYWRkcl9sb2NhdGlvbihzdHJ1Y3QgdGhy
ZWFkDQo+ICp0aHJlYWQsDQo+ID4gICBpbnQgcGVyZl9ldmVudF9fcHJlcHJvY2Vzc19zYW1wbGUo
Y29uc3QgdW5pb24gcGVyZl9ldmVudCAqZXZlbnQsDQo+ID4gICAJCQkJICBzdHJ1Y3QgbWFjaGlu
ZSAqbWFjaGluZSwNCj4gPiAgIAkJCQkgIHN0cnVjdCBhZGRyX2xvY2F0aW9uICphbCwNCj4gPiAt
CQkJCSAgc3RydWN0IHBlcmZfc2FtcGxlICpzYW1wbGUpDQo+ID4gKwkJCQkgIHN0cnVjdCBwZXJm
X3NhbXBsZSAqc2FtcGxlLA0KPiA+ICsJCQkJICBzdHJ1Y3QgcGVyZl9lbnYgKmVudikNCj4gPiAg
IHsNCj4gPiAgIAl1OCBjcHVtb2RlID0gZXZlbnQtPmhlYWRlci5taXNjICYNCj4gUEVSRl9SRUNP
UkRfTUlTQ19DUFVNT0RFX01BU0s7DQo+ID4gICAJc3RydWN0IHRocmVhZCAqdGhyZWFkID0gbWFj
aGluZV9fZmluZG5ld190aHJlYWQobWFjaGluZSwNCj4gPiBzYW1wbGUtPnBpZCwgQEAgLTEwMjEs
NyArMTAyMiwxMCBAQCBpbnQNCj4gPiBwZXJmX2V2ZW50X19wcmVwcm9jZXNzX3NhbXBsZShjb25z
dCB1bmlvbiBwZXJmX2V2ZW50ICpldmVudCwNCj4gPg0KPiA+ICAgCWFsLT5zeW0gPSBOVUxMOw0K
PiA+ICAgCWFsLT5jcHUgPSBzYW1wbGUtPmNwdTsNCj4gPiAtCWFsLT5zb2NrZXQgPSBjcHVfbWFw
X19nZXRfc29ja2V0X2lkKGFsLT5jcHUpOw0KPiA+ICsNCj4gPiArCWFsLnNvY2tldCA9IC0xOw0K
PiA+ICsJaWYgKGVudi0+Y3B1ICYmIGFsLT5jcHUgPj0gMCkNCj4gPiArCQlhbC5zb2NrZXQgPSBl
bnYtPmNwdVthbC0+Y3B1XS5zb2NrZXRfaWQ7DQo+ID4NCj4gPiAgIAlpZiAoYWwtPm1hcCkgew0K
PiA+ICAgCQlzdHJ1Y3QgZHNvICpkc28gPSBhbC0+bWFwLT5kc287DQo+IA0KPiBOb3cgSSB1bmRl
cnN0YW5kIHlvdXIgc3VnZ2VzdGlvbi4gWW91IG1lYW4gd2UgY2FuIGJ1aWxkIGVudi0+Y3B1DQo+
IGJlZm9yZSBwcm9jZXNzaW5nIHRoZSBmaXJzdCBzYW1wbGUsIHRoZW4gaW5pdCBhbC5zb2NrZXQg
dXNpbmcgdGhhdCBtYXANCj4gaW5zdGVhZCBvZiBjYWxsaW5nIGNwdV9tYXBfX2dldF9zb2NrZXRf
aWQoKSB1bmNvbmRpdGlvbmFsbHkgaW4gYW4gYWQtaG9jDQo+IHdheS4NCj4gDQo+IEFuZCBJIGhh
dmUgYW5vdGhlciBxdWVzdGlvbiB0aGF0LCBzaW5jZSBidWlsZF9jcHVfdG9wbygpIGFuZA0KPiBw
ZXJmX2V2ZW50X19wcmVwcm9jZXNzX3NhbXBsZSgpIGFyZSBtb3JlIG9yIGxlc3MgZG9pbmcgc2lt
aWxhciB0aGluZ3MsDQo+IHdoeSB3ZSBuZWVkIGJvdGggb2YgdGhlbT8NCj4gDQo+IFRoZW4gd2Ug
bmVlZCBtb3JlIGNvZGUgZm9yIHRoaXMgYnVnLi4uDQo+IA0KPiBLYW4gTGlhbmcsIGRvIHlvdSBo
YXZlIGFueSBzdWdnZXN0aW9uPw0KPiANCj4gDQoNCkkgdGhpbmsgSmlya2EncyB3YXkgaXMgYmV0
dGVyLiBXZSBzaG91bGQgaGFuZGxlIGFsLnNvY2tldCBpbiBvbmUgcGxhY2UgZm9yIGFsbCB0b29s
cy4NCg0KTm93IHdlIGFscmVhZHkgcmVhZCBlbnYgZnJvbSBmaWxlIGluIHBlcmZfc2Vzc2lvbl9f
bmV3IGZvciBwZXJmIHJlcG9ydC4NCkkgdGhpbmsgd2Ugb25seSBuZWVkIHRvIHVwZGF0ZSBlbnYg
aW4gcGVyZl9zZXNzaW9uX19uZXcgZm9yIG90aGVyIHRvb2xzLg0KU28gcGVyZl9ldmVudF9fcHJl
cHJvY2Vzc19zYW1wbGUgY2FuIHVzZSBpdC4NCg0KVGhhbmtzLA0KS2FuIA0KDQo=
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220919

FromArnaldo Carvalho de Melo <acme@redhat.com>
Date2015-09-08 17:20 +0200
Message-ID<q6sQF-80Q-9@gated-at.bofh.it>
In reply to#1220546
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220940

FromJiri Olsa <jolsa@redhat.com>
Date2015-09-08 17:40 +0200
Message-ID<q6ta3-8oh-51@gated-at.bofh.it>
In reply to#1220919
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220948

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-08 18:00 +0200
Message-ID<q6ttp-jn-23@gated-at.bofh.it>
In reply to#1220940
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220952

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-08 18:20 +0200
Message-ID<q6tMJ-Vo-9@gated-at.bofh.it>
In reply to#1220948
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1220972

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-08 18:40 +0200
Message-ID<q6u66-1i4-7@gated-at.bofh.it>
In reply to#1220952
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221543

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-09 18:10 +0200
Message-ID<q6Q6C-7ZD-9@gated-at.bofh.it>
In reply to#1220972
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1221564

FromArnaldo Carvalho de Melo <acme@kernel.org>
Date2015-09-09 18:50 +0200
Message-ID<q6QJk-ho-17@gated-at.bofh.it>
In reply to#1221543
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web