[PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds
@ 2017-10-06  2:00 Andi Kleen
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Andi Kleen @ 2017-10-06  2:00 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

Now that the xyarray stores the dimensions we can use those
to iterate over the FDs for a evsel.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index dd525417880a..3c697118e44b 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -557,14 +557,13 @@ static int perf_stat_synthesize_config(bool is_pipe)
 
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
 
-static int __store_counter_ids(struct perf_evsel *counter,
-			       struct cpu_map *cpus,
-			       struct thread_map *threads)
+static int __store_counter_ids(struct perf_evsel *counter)
 {
 	int cpu, thread;
 
-	for (cpu = 0; cpu < cpus->nr; cpu++) {
-		for (thread = 0; thread < threads->nr; thread++) {
+	for (cpu = 0; cpu < xyarray__max_x(counter->fd); cpu++) {
+		for (thread = 0; thread < xyarray__max_y(counter->fd);
+		     thread++) {
 			int fd = FD(counter, cpu, thread);
 
 			if (perf_evlist__id_add_fd(evsel_list, counter,
@@ -584,7 +583,7 @@ static int store_counter_ids(struct perf_evsel *counter)
 	if (perf_evsel__alloc_id(counter, cpus->nr, threads->nr))
 		return -ENOMEM;
 
-	return __store_counter_ids(counter, cpus, threads);
+	return __store_counter_ids(counter);
 }
 
 static bool perf_evsel__should_store_id(struct perf_evsel *counter)
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2017-10-06  2:00 [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Andi Kleen
@ 2017-10-06  2:00 ` Andi Kleen
  2017-10-06  3:36   ` Andi Kleen
                     ` (2 more replies)
  2018-02-21 14:33 ` [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Jiri Olsa
  2018-03-06  6:41 ` [tip:perf/core] perf " tip-bot for Andi Kleen
  2 siblings, 3 replies; 11+ messages in thread
From: Andi Kleen @ 2017-10-06  2:00 UTC (permalink / raw)
  To: acme; +Cc: jolsa, linux-kernel, Andi Kleen

From: Andi Kleen <ak@linux.intel.com>

perf stat can retry opening events. After opening an file descriptor
it adds the ids to the ecsel. Each event keeps a running
count of ids. When we decide to close an evsel and retry
with a different configuration this count needs to be reset,
otherwise it can overflow the buffer.

Always reset the ids count.

This fixes the following problem, triggered by a complex perf
stat configuration.

==666== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x600600001500 at pc 0x59dfd7 bp 0x7ffc6b397000 sp 0x7ffc6b396ff8
WRITE of size 8 at 0x600600001500 thread T0
    #0 0x59dfd6 in perf_evlist__id_add /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:515
    #1 0x59e291 in perf_evlist__id_add_fd /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:555
    #2 0x4865c6 in __store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:570
    #3 0x4867d0 in store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:587
    #4 0x487521 in __run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:703
    #5 0x487d6d in run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:805
    #6 0x492914 in cmd_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:2839
    #7 0x57479a in run_builtin /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:296
    #8 0x574b5f in handle_internal_command /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:348
    #9 0x574e3c in run_argv /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:392
    #10 0x575401 in main /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:536
    #11 0x7f7597af96e4 in __libc_start_main (/lib64/libc.so.6+0x206e4)
    #12 0x444598 in _start /home/abuild/rpmbuild/BUILD/glibc-2.22/csu/../sysdeps/x86_64/start.S:118
0x600600001500 is located 0 bytes to the right of 32-byte region [0x6006000014e0,0x600600001500)
allocated by thread T0 here:
    #0 0x7f759a698ab5 in calloc (/usr/lib64/libasan.so.0+0x15ab5)
    #1 0x5a6045 in zalloc /home/ak/hle/linux-hle-2.6/tools/perf/util/util.h:22
    #2 0x5ab51a in perf_evsel__alloc_id /home/ak/hle/linux-hle-2.6/tools/perf/util/evsel.c:1158
    #3 0x4867ae in store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:584
    #4 0x487521 in __run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:703
    #5 0x487d6d in run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:805
    #6 0x492914 in cmd_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:2839
    #7 0x57479a in run_builtin /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:296
    #8 0x574b5f in handle_internal_command /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:348
    #9 0x574e3c in run_argv /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:392
    #10 0x575401 in main /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:536
    #11 0x7f7597af96e4 in __libc_start_main (/lib64/libc.so.6+0x206e4)
SUMMARY: AddressSanitizer: heap-buffer-overflow /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:515 perf_evlist__id_add
Shadow bytes around the buggy address:
  0x0c013fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c013fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c013fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c013fff8280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c013fff8290: fa fa fa fa fa fa fa fa fa fa fa fa 00 00 00 00
=>0x0c013fff82a0:[fa]fa 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00
  0x0c013fff82b0: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
  0x0c013fff82c0: 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00
  0x0c013fff82d0: fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00
  0x0c013fff82e0: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
  0x0c013fff82f0: 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:     fa
  Heap righ redzone:     fb
  Freed Heap region:     fd
  Stack left redzone:    f1
  Stack mid redzone:     f2
  Stack right redzone:   f3
  Stack partial redzone: f4
  Stack after return:    f5
  Stack use after scope: f8
  Global redzone:        f9
  Global init order:     f6
  Poisoned by user:      f7
  ASan internal:         fe

Signed-off-by: Andi Kleen <ak@linux.intel.com>
---
 tools/perf/builtin-stat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 3c697118e44b..b54ec8497f97 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -652,6 +652,7 @@ static int __run_perf_stat(int argc, const char **argv)
 
 	evlist__for_each_entry(evsel_list, counter) {
 try_again:
+		counter->ids = 0;
 		if (create_perf_stat_counter(counter) < 0) {
 
 			/* Weak group failed. Reset the group. */
-- 
2.13.6

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
@ 2017-10-06  3:36   ` Andi Kleen
  2018-02-21 14:00   ` Arnaldo Carvalho de Melo
  2018-02-21 14:31   ` Jiri Olsa
  2 siblings, 0 replies; 11+ messages in thread
From: Andi Kleen @ 2017-10-06  3:36 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, jolsa, linux-kernel

>   Poisoned by user:      f7
>   ASan internal:         fe
> 

Forgot add. This should be 

Cc: stable@vger.kernel.org  # 4.4+

[it breaks pmu-tools]

-Andi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
  2017-10-06  3:36   ` Andi Kleen
@ 2018-02-21 14:00   ` Arnaldo Carvalho de Melo
  2018-02-21 14:39     ` Jiri Olsa
  2018-02-21 14:31   ` Jiri Olsa
  2 siblings, 1 reply; 11+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-21 14:00 UTC (permalink / raw)
  To: Andi Kleen; +Cc: Jiri Olsa, linux-kernel, Namhyung Kim, Andi Kleen

Em Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen escreveu:
> From: Andi Kleen <ak@linux.intel.com>
> 
> perf stat can retry opening events. After opening an file descriptor
> it adds the ids to the ecsel. Each event keeps a running
> count of ids.

Ok, and that is done in perf_evlist__id_add() where we also add things
to the evlist->heads hashtable, shouldn't we remove all these before
retrying? I.e. perf_evlist__close() shouldn't be resetting that
hashtable?

I.e. like in the following patch? Jiri? Namhyung?

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 7b7d535396f7..e8942456106e 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -37,13 +37,18 @@
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
 #define SID(e, x, y) xyarray__entry(e->sample_id, x, y)
 
-void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
-		       struct thread_map *threads)
+static void perf_evlist__init_heads(struct perf_evlist *evlist)
 {
 	int i;
 
 	for (i = 0; i < PERF_EVLIST__HLIST_SIZE; ++i)
 		INIT_HLIST_HEAD(&evlist->heads[i]);
+}
+
+void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
+		       struct thread_map *threads)
+{
+	perf_evlist__init_heads(evlist);
 	INIT_LIST_HEAD(&evlist->entries);
 	perf_evlist__set_maps(evlist, cpus, threads);
 	fdarray__init(&evlist->pollfd, 64);
@@ -1381,8 +1386,11 @@ void perf_evlist__close(struct perf_evlist *evlist)
 {
 	struct perf_evsel *evsel;
 
-	evlist__for_each_entry_reverse(evlist, evsel)
+	evlist__for_each_entry_reverse(evlist, evsel) {
 		perf_evsel__close(evsel);
+	}
+
+	perf_evlist__init_heads(evlist);
 }
 
 static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index ef351688b797..fba708bc9d98 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1932,6 +1932,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
 
 	perf_evsel__close_fd(evsel);
 	perf_evsel__free_fd(evsel);
+	evsel->ids = 0;
 }
 
 int perf_evsel__open_per_cpu(struct perf_evsel *evsel,

> When we decide to close an evsel and retry
> with a different configuration this count needs to be reset,
> otherwise it can overflow the buffer.
> 
> Always reset the ids count.
> 
> This fixes the following problem, triggered by a complex perf
> stat configuration.
> 
> ==666== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x600600001500 at pc 0x59dfd7 bp 0x7ffc6b397000 sp 0x7ffc6b396ff8
> WRITE of size 8 at 0x600600001500 thread T0
>     #0 0x59dfd6 in perf_evlist__id_add /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:515
>     #1 0x59e291 in perf_evlist__id_add_fd /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:555
>     #2 0x4865c6 in __store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:570
>     #3 0x4867d0 in store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:587
>     #4 0x487521 in __run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:703
>     #5 0x487d6d in run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:805
>     #6 0x492914 in cmd_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:2839
>     #7 0x57479a in run_builtin /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:296
>     #8 0x574b5f in handle_internal_command /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:348
>     #9 0x574e3c in run_argv /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:392
>     #10 0x575401 in main /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:536
>     #11 0x7f7597af96e4 in __libc_start_main (/lib64/libc.so.6+0x206e4)
>     #12 0x444598 in _start /home/abuild/rpmbuild/BUILD/glibc-2.22/csu/../sysdeps/x86_64/start.S:118
> 0x600600001500 is located 0 bytes to the right of 32-byte region [0x6006000014e0,0x600600001500)
> allocated by thread T0 here:
>     #0 0x7f759a698ab5 in calloc (/usr/lib64/libasan.so.0+0x15ab5)
>     #1 0x5a6045 in zalloc /home/ak/hle/linux-hle-2.6/tools/perf/util/util.h:22
>     #2 0x5ab51a in perf_evsel__alloc_id /home/ak/hle/linux-hle-2.6/tools/perf/util/evsel.c:1158
>     #3 0x4867ae in store_counter_ids /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:584
>     #4 0x487521 in __run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:703
>     #5 0x487d6d in run_perf_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:805
>     #6 0x492914 in cmd_stat /home/ak/hle/linux-hle-2.6/tools/perf/builtin-stat.c:2839
>     #7 0x57479a in run_builtin /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:296
>     #8 0x574b5f in handle_internal_command /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:348
>     #9 0x574e3c in run_argv /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:392
>     #10 0x575401 in main /home/ak/hle/linux-hle-2.6/tools/perf/perf.c:536
>     #11 0x7f7597af96e4 in __libc_start_main (/lib64/libc.so.6+0x206e4)
> SUMMARY: AddressSanitizer: heap-buffer-overflow /home/ak/hle/linux-hle-2.6/tools/perf/util/evlist.c:515 perf_evlist__id_add
> Shadow bytes around the buggy address:
>   0x0c013fff8250: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c013fff8260: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c013fff8270: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c013fff8280: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0c013fff8290: fa fa fa fa fa fa fa fa fa fa fa fa 00 00 00 00
> =>0x0c013fff82a0:[fa]fa 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00
>   0x0c013fff82b0: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
>   0x0c013fff82c0: 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00
>   0x0c013fff82d0: fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00
>   0x0c013fff82e0: 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00 fa fa
>   0x0c013fff82f0: 00 00 00 00 fa fa 00 00 00 00 fa fa 00 00 00 00
> Shadow byte legend (one shadow byte represents 8 application bytes):
>   Addressable:           00
>   Partially addressable: 01 02 03 04 05 06 07
>   Heap left redzone:     fa
>   Heap righ redzone:     fb
>   Freed Heap region:     fd
>   Stack left redzone:    f1
>   Stack mid redzone:     f2
>   Stack right redzone:   f3
>   Stack partial redzone: f4
>   Stack after return:    f5
>   Stack use after scope: f8
>   Global redzone:        f9
>   Global init order:     f6
>   Poisoned by user:      f7
>   ASan internal:         fe
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>
> ---
>  tools/perf/builtin-stat.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 3c697118e44b..b54ec8497f97 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -652,6 +652,7 @@ static int __run_perf_stat(int argc, const char **argv)
>  
>  	evlist__for_each_entry(evsel_list, counter) {
>  try_again:
> +		counter->ids = 0;
>  		if (create_perf_stat_counter(counter) < 0) {
>  
>  			/* Weak group failed. Reset the group. */
> -- 
> 2.13.6

^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2018-02-21 14:00   ` Arnaldo Carvalho de Melo
@ 2018-02-21 14:39     ` Jiri Olsa
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2018-02-21 14:39 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo
  Cc: Andi Kleen, Jiri Olsa, linux-kernel, Namhyung Kim, Andi Kleen

On Wed, Feb 21, 2018 at 11:00:02AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen escreveu:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > perf stat can retry opening events. After opening an file descriptor
> > it adds the ids to the ecsel. Each event keeps a running
> > count of ids.
> 
> Ok, and that is done in perf_evlist__id_add() where we also add things
> to the evlist->heads hashtable, shouldn't we remove all these before
> retrying? I.e. perf_evlist__close() shouldn't be resetting that
> hashtable?
> 
> I.e. like in the following patch? Jiri? Namhyung?
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 7b7d535396f7..e8942456106e 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -37,13 +37,18 @@
>  #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
>  #define SID(e, x, y) xyarray__entry(e->sample_id, x, y)
>  
> -void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
> -		       struct thread_map *threads)
> +static void perf_evlist__init_heads(struct perf_evlist *evlist)
>  {
>  	int i;
>  
>  	for (i = 0; i < PERF_EVLIST__HLIST_SIZE; ++i)
>  		INIT_HLIST_HEAD(&evlist->heads[i]);
> +}
> +
> +void perf_evlist__init(struct perf_evlist *evlist, struct cpu_map *cpus,
> +		       struct thread_map *threads)
> +{
> +	perf_evlist__init_heads(evlist);
>  	INIT_LIST_HEAD(&evlist->entries);
>  	perf_evlist__set_maps(evlist, cpus, threads);
>  	fdarray__init(&evlist->pollfd, 64);
> @@ -1381,8 +1386,11 @@ void perf_evlist__close(struct perf_evlist *evlist)
>  {
>  	struct perf_evsel *evsel;
>  
> -	evlist__for_each_entry_reverse(evlist, evsel)
> +	evlist__for_each_entry_reverse(evlist, evsel) {
>  		perf_evsel__close(evsel);
> +	}
> +
> +	perf_evlist__init_heads(evlist);
>  }
>  
>  static int perf_evlist__create_syswide_maps(struct perf_evlist *evlist)
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index ef351688b797..fba708bc9d98 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1932,6 +1932,7 @@ void perf_evsel__close(struct perf_evsel *evsel)
>  
>  	perf_evsel__close_fd(evsel);
>  	perf_evsel__free_fd(evsel);
> +	evsel->ids = 0;
>  }

yes, that seems correct

jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
  2017-10-06  3:36   ` Andi Kleen
  2018-02-21 14:00   ` Arnaldo Carvalho de Melo
@ 2018-02-21 14:31   ` Jiri Olsa
  2018-02-21 14:33     ` Arnaldo Carvalho de Melo
  2 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2018-02-21 14:31 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen

On Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>

ouch, sry for overlooking this

> 
> perf stat can retry opening events. After opening an file descriptor
> it adds the ids to the ecsel. Each event keeps a running
> count of ids. When we decide to close an evsel and retry
> with a different configuration this count needs to be reset,
> otherwise it can overflow the buffer.

how can this happen? we call store_counter_ids at the
end of the loop, when the evsel is all done and can't
be reconfigured

thanks,
jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2018-02-21 14:31   ` Jiri Olsa
@ 2018-02-21 14:33     ` Arnaldo Carvalho de Melo
  2018-02-21 14:37       ` Jiri Olsa
  0 siblings, 1 reply; 11+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-21 14:33 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: Andi Kleen, jolsa, linux-kernel, Andi Kleen

Em Wed, Feb 21, 2018 at 03:31:17PM +0100, Jiri Olsa escreveu:
> On Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> 
> ouch, sry for overlooking this
> 
> > 
> > perf stat can retry opening events. After opening an file descriptor
> > it adds the ids to the ecsel. Each event keeps a running
> > count of ids. When we decide to close an evsel and retry
> > with a different configuration this count needs to be reset,
> > otherwise it can overflow the buffer.
> 
> how can this happen? we call store_counter_ids at the
> end of the loop, when the evsel is all done and can't
> be reconfigured

Yeah, I couldn't figure out the exact sequence, but I think that we need
to reset those hash tables when doing a perf_evlist__close(), no? I.e.
when we open we may get new ids, so need to rehash?

- Arnaldo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
  2018-02-21 14:33     ` Arnaldo Carvalho de Melo
@ 2018-02-21 14:37       ` Jiri Olsa
  0 siblings, 0 replies; 11+ messages in thread
From: Jiri Olsa @ 2018-02-21 14:37 UTC (permalink / raw)
  To: Arnaldo Carvalho de Melo; +Cc: Andi Kleen, jolsa, linux-kernel, Andi Kleen

On Wed, Feb 21, 2018 at 11:33:37AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Feb 21, 2018 at 03:31:17PM +0100, Jiri Olsa escreveu:
> > On Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen wrote:
> > > From: Andi Kleen <ak@linux.intel.com>
> > 
> > ouch, sry for overlooking this
> > 
> > > 
> > > perf stat can retry opening events. After opening an file descriptor
> > > it adds the ids to the ecsel. Each event keeps a running
> > > count of ids. When we decide to close an evsel and retry
> > > with a different configuration this count needs to be reset,
> > > otherwise it can overflow the buffer.
> > 
> > how can this happen? we call store_counter_ids at the
> > end of the loop, when the evsel is all done and can't
> > be reconfigured
> 
> Yeah, I couldn't figure out the exact sequence, but I think that we need
> to reset those hash tables when doing a perf_evlist__close(), no? I.e.
> when we open we may get new ids, so need to rehash?

yes, I think we should reset it any time the event is closed
I'll check your changes 

jirka

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds
  2017-10-06  2:00 [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Andi Kleen
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
@ 2018-02-21 14:33 ` Jiri Olsa
  2018-02-21 14:37   ` Arnaldo Carvalho de Melo
  2018-03-06  6:41 ` [tip:perf/core] perf " tip-bot for Andi Kleen
  2 siblings, 1 reply; 11+ messages in thread
From: Jiri Olsa @ 2018-02-21 14:33 UTC (permalink / raw)
  To: Andi Kleen; +Cc: acme, jolsa, linux-kernel, Andi Kleen

On Thu, Oct 05, 2017 at 07:00:28PM -0700, Andi Kleen wrote:
> From: Andi Kleen <ak@linux.intel.com>
> 
> Now that the xyarray stores the dimensions we can use those
> to iterate over the FDs for a evsel.
> 
> Signed-off-by: Andi Kleen <ak@linux.intel.com>

Acked-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

> ---
>  tools/perf/builtin-stat.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index dd525417880a..3c697118e44b 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -557,14 +557,13 @@ static int perf_stat_synthesize_config(bool is_pipe)
>  
>  #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
>  
> -static int __store_counter_ids(struct perf_evsel *counter,
> -			       struct cpu_map *cpus,
> -			       struct thread_map *threads)
> +static int __store_counter_ids(struct perf_evsel *counter)
>  {
>  	int cpu, thread;
>  
> -	for (cpu = 0; cpu < cpus->nr; cpu++) {
> -		for (thread = 0; thread < threads->nr; thread++) {
> +	for (cpu = 0; cpu < xyarray__max_x(counter->fd); cpu++) {
> +		for (thread = 0; thread < xyarray__max_y(counter->fd);
> +		     thread++) {
>  			int fd = FD(counter, cpu, thread);
>  
>  			if (perf_evlist__id_add_fd(evsel_list, counter,
> @@ -584,7 +583,7 @@ static int store_counter_ids(struct perf_evsel *counter)
>  	if (perf_evsel__alloc_id(counter, cpus->nr, threads->nr))
>  		return -ENOMEM;
>  
> -	return __store_counter_ids(counter, cpus, threads);
> +	return __store_counter_ids(counter);
>  }
>  
>  static bool perf_evsel__should_store_id(struct perf_evsel *counter)
> -- 
> 2.13.6
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds
  2018-02-21 14:33 ` [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Jiri Olsa
@ 2018-02-21 14:37   ` Arnaldo Carvalho de Melo
  0 siblings, 0 replies; 11+ messages in thread
From: Arnaldo Carvalho de Melo @ 2018-02-21 14:37 UTC (permalink / raw)
  To: Jiri Olsa; +Cc: Andi Kleen, jolsa, linux-kernel, Andi Kleen

Em Wed, Feb 21, 2018 at 03:33:11PM +0100, Jiri Olsa escreveu:
> On Thu, Oct 05, 2017 at 07:00:28PM -0700, Andi Kleen wrote:
> > From: Andi Kleen <ak@linux.intel.com>
> > 
> > Now that the xyarray stores the dimensions we can use those
> > to iterate over the FDs for a evsel.
> > 
> > Signed-off-by: Andi Kleen <ak@linux.intel.com>
> 
> Acked-by: Jiri Olsa <jolsa@kernel.org>

Thanks, applied, and also sorry for having overlooked this :-\

- Arnaldo

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [tip:perf/core] perf stat: Use xyarray dimensions to iterate fds
  2017-10-06  2:00 [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Andi Kleen
  2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
  2018-02-21 14:33 ` [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Jiri Olsa
@ 2018-03-06  6:41 ` tip-bot for Andi Kleen
  2 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Andi Kleen @ 2018-03-06  6:41 UTC (permalink / raw)
  To: linux-tip-commits; +Cc: mingo, hpa, tglx, ak, linux-kernel, acme, jolsa

Commit-ID:  42811d509d6e0b0118918ce6be346be54d8e8801
Gitweb:     https://git.kernel.org/tip/42811d509d6e0b0118918ce6be346be54d8e8801
Author:     Andi Kleen <ak@linux.intel.com>
AuthorDate: Thu, 5 Oct 2017 19:00:28 -0700
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Wed, 21 Feb 2018 11:36:57 -0300

perf stat: Use xyarray dimensions to iterate fds

Now that the xyarray stores the dimensions we can use those
to iterate over the FDs for a evsel.

Signed-off-by: Andi Kleen <ak@linux.intel.com>
Acked-by: Jiri Olsa <jolsa@kernel.org>
Link: http://lkml.kernel.org/r/20171006020029.13339-1-andi@firstfloor.org
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-stat.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 2d49eccf98f2..fadcff52cd09 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -508,14 +508,13 @@ static int perf_stat_synthesize_config(bool is_pipe)
 
 #define FD(e, x, y) (*(int *)xyarray__entry(e->fd, x, y))
 
-static int __store_counter_ids(struct perf_evsel *counter,
-			       struct cpu_map *cpus,
-			       struct thread_map *threads)
+static int __store_counter_ids(struct perf_evsel *counter)
 {
 	int cpu, thread;
 
-	for (cpu = 0; cpu < cpus->nr; cpu++) {
-		for (thread = 0; thread < threads->nr; thread++) {
+	for (cpu = 0; cpu < xyarray__max_x(counter->fd); cpu++) {
+		for (thread = 0; thread < xyarray__max_y(counter->fd);
+		     thread++) {
 			int fd = FD(counter, cpu, thread);
 
 			if (perf_evlist__id_add_fd(evsel_list, counter,
@@ -535,7 +534,7 @@ static int store_counter_ids(struct perf_evsel *counter)
 	if (perf_evsel__alloc_id(counter, cpus->nr, threads->nr))
 		return -ENOMEM;
 
-	return __store_counter_ids(counter, cpus, threads);
+	return __store_counter_ids(counter);
 }
 
 static bool perf_evsel__should_store_id(struct perf_evsel *counter)

^ permalink raw reply related	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-03-06  6:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-06  2:00 [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Andi Kleen
2017-10-06  2:00 ` [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events Andi Kleen
2017-10-06  3:36   ` Andi Kleen
2018-02-21 14:00   ` Arnaldo Carvalho de Melo
2018-02-21 14:39     ` Jiri Olsa
2018-02-21 14:31   ` Jiri Olsa
2018-02-21 14:33     ` Arnaldo Carvalho de Melo
2018-02-21 14:37       ` Jiri Olsa
2018-02-21 14:33 ` [PATCH 1/2] perf, tools, stat: Use xyarray dimensions to iterate fds Jiri Olsa
2018-02-21 14:37   ` Arnaldo Carvalho de Melo
2018-03-06  6:41 ` [tip:perf/core] perf " tip-bot for Andi Kleen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox