Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
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
> >
> > 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
Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
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 > > > > 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
Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
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 > > 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
Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
On Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen wrote: > From: Andi Kleen 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
Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
Em Thu, Oct 05, 2017 at 07:00:29PM -0700, Andi Kleen escreveu:
> From: Andi Kleen
>
> 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
> 0x60061500 at pc 0x59dfd7 bp 0x7ffc6b397000 sp 0x7ffc6b396ff8
> WRITE of size 8 at 0x60061500 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
> 0x60061500 is located 0 bytes to the right of 32-byte region
> [0x600614e0,0x60061500)
> 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 0x7f
Re: [PATCH 2/2] perf, tools, stat: Reset ids counter when retrying events
> Poisoned by user: f7 > ASan internal: fe > Forgot add. This should be Cc: sta...@vger.kernel.org # 4.4+ [it breaks pmu-tools] -Andi
