* [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
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 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 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
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
* 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
* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.