[PATCH 1/2] libperf: Add perf_evlist__reset_id_hash()

[PATCH 1/2] libperf: Add perf_evlist__reset_id_hash()

[Namhyung Kim]

Add the perf_evlist__reset_id_hash() function to libperf API so that
it can be called to reset the hash table.  This is necessary for perf
stat to run the workload multiple times.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/lib/perf/evlist.c              | 13 +++++++++----
 tools/lib/perf/include/perf/evlist.h |  2 ++
 tools/lib/perf/libperf.map           |  1 +
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 17465d454a0e..a0aaf385cbb5 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -26,13 +26,10 @@
 
 void perf_evlist__init(struct perf_evlist *evlist)
 {
-	int i;
-
-	for (i = 0; i < PERF_EVLIST__HLIST_SIZE; ++i)
-		INIT_HLIST_HEAD(&evlist->heads[i]);
 	INIT_LIST_HEAD(&evlist->entries);
 	evlist->nr_entries = 0;
 	fdarray__init(&evlist->pollfd, 64);
+	perf_evlist__reset_id_hash(evlist);
 }
 
 static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
@@ -237,6 +234,14 @@ static void perf_evlist__id_hash(struct perf_evlist *evlist,
 	hlist_add_head(&sid->node, &evlist->heads[hash]);
 }
 
+void perf_evlist__reset_id_hash(struct perf_evlist *evlist)
+{
+	int i;
+
+	for (i = 0; i < PERF_EVLIST__HLIST_SIZE; ++i)
+		INIT_HLIST_HEAD(&evlist->heads[i]);
+}
+
 void perf_evlist__id_add(struct perf_evlist *evlist,
 			 struct perf_evsel *evsel,
 			 int cpu, int thread, u64 id)
diff --git a/tools/lib/perf/include/perf/evlist.h b/tools/lib/perf/include/perf/evlist.h
index 0a7479dc13bf..0085732e8cd9 100644
--- a/tools/lib/perf/include/perf/evlist.h
+++ b/tools/lib/perf/include/perf/evlist.h
@@ -46,4 +46,6 @@ LIBPERF_API struct perf_mmap *perf_evlist__next_mmap(struct perf_evlist *evlist,
 	     (pos) != NULL;						\
 	     (pos) = perf_evlist__next_mmap((evlist), (pos), overwrite))
 
+LIBPERF_API void perf_evlist__reset_id_hash(struct perf_evlist *evlist);
+
 #endif /* __LIBPERF_EVLIST_H */
diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
index 7be1af8a546c..285100143d89 100644
--- a/tools/lib/perf/libperf.map
+++ b/tools/lib/perf/libperf.map
@@ -42,6 +42,7 @@ LIBPERF_0.0.1 {
 		perf_evlist__munmap;
 		perf_evlist__filter_pollfd;
 		perf_evlist__next_mmap;
+		perf_evlist__reset_id_hash;
 		perf_mmap__consume;
 		perf_mmap__read_init;
 		perf_mmap__read_done;
-- 
2.30.0.617.g56c4b15f3c-goog

[PATCH 2/2] perf stat: Fix segfault when -r option is used

[Namhyung Kim]

I got a segfault when using -r option with event groups.  The option
makes it run the workload multiple times and it will reuse the evlist
and evsel for each run.

While most of resources are allocated and freed properly, the id hash
in the evlist was not and it resulted in a crash.  You can see it with
the address sanitizer like below:

  $ perf stat -r 100 -e '{cycles,instructions}' true
  =================================================================
  ==693052==ERROR: AddressSanitizer: heap-use-after-free on
      address 0x6080000003d0 at pc 0x558c57732835 bp 0x7fff1526adb0 sp 0x7fff1526ada8
  WRITE of size 8 at 0x6080000003d0 thread T0
    #0 0x558c57732834 in hlist_add_head /home/namhyung/project/linux/tools/include/linux/list.h:644
    #1 0x558c57732834 in perf_evlist__id_hash /home/namhyung/project/linux/tools/lib/perf/evlist.c:237
    #2 0x558c57732834 in perf_evlist__id_add /home/namhyung/project/linux/tools/lib/perf/evlist.c:244
    #3 0x558c57732834 in perf_evlist__id_add_fd /home/namhyung/project/linux/tools/lib/perf/evlist.c:285
    #4 0x558c5747733e in store_evsel_ids util/evsel.c:2765
    #5 0x558c5747733e in evsel__store_ids util/evsel.c:2782
    #6 0x558c5730b717 in __run_perf_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:895
    #7 0x558c5730b717 in run_perf_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:1014
    #8 0x558c5730b717 in cmd_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:2446
    #9 0x558c57427c24 in run_builtin /home/namhyung/project/linux/tools/perf/perf.c:313
    #10 0x558c572b1a48 in handle_internal_command /home/namhyung/project/linux/tools/perf/perf.c:365
    #11 0x558c572b1a48 in run_argv /home/namhyung/project/linux/tools/perf/perf.c:409
    #12 0x558c572b1a48 in main /home/namhyung/project/linux/tools/perf/perf.c:539
    #13 0x7fcadb9f7d09 in __libc_start_main ../csu/libc-start.c:308
    #14 0x558c572b60f9 in _start (/home/namhyung/project/linux/tools/perf/perf+0x45d0f9)

Actually the nodes in the hash table are struct perf_stream_id and
they were freed in the previous run.  Fix it by resetting the hash.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/evlist.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index 5121b4db66fe..882cd1f721d9 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1306,6 +1306,7 @@ void evlist__close(struct evlist *evlist)
 		perf_evsel__free_fd(&evsel->core);
 		perf_evsel__free_id(&evsel->core);
 	}
+	perf_evlist__reset_id_hash(&evlist->core);
 }
 
 static int evlist__create_syswide_maps(struct evlist *evlist)
-- 
2.30.0.617.g56c4b15f3c-goog

Re: [PATCH 2/2] perf stat: Fix segfault when -r option is used

[Namhyung Kim]

On Wed, Feb 24, 2021 at 5:11 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> I got a segfault when using -r option with event groups.  The option
> makes it run the workload multiple times and it will reuse the evlist
> and evsel for each run.

Well, it might not see a segfault because the freed memory region is
likely to be reused.  But you can see the bug clearly with asan.

Thanks,
Namhyung

>
> While most of resources are allocated and freed properly, the id hash
> in the evlist was not and it resulted in a crash.  You can see it with
> the address sanitizer like below:
>
>   $ perf stat -r 100 -e '{cycles,instructions}' true
>   =================================================================
>   ==693052==ERROR: AddressSanitizer: heap-use-after-free on
>       address 0x6080000003d0 at pc 0x558c57732835 bp 0x7fff1526adb0 sp 0x7fff1526ada8
>   WRITE of size 8 at 0x6080000003d0 thread T0
>     #0 0x558c57732834 in hlist_add_head /home/namhyung/project/linux/tools/include/linux/list.h:644
>     #1 0x558c57732834 in perf_evlist__id_hash /home/namhyung/project/linux/tools/lib/perf/evlist.c:237
>     #2 0x558c57732834 in perf_evlist__id_add /home/namhyung/project/linux/tools/lib/perf/evlist.c:244
>     #3 0x558c57732834 in perf_evlist__id_add_fd /home/namhyung/project/linux/tools/lib/perf/evlist.c:285
>     #4 0x558c5747733e in store_evsel_ids util/evsel.c:2765
>     #5 0x558c5747733e in evsel__store_ids util/evsel.c:2782
>     #6 0x558c5730b717 in __run_perf_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:895
>     #7 0x558c5730b717 in run_perf_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:1014
>     #8 0x558c5730b717 in cmd_stat /home/namhyung/project/linux/tools/perf/builtin-stat.c:2446
>     #9 0x558c57427c24 in run_builtin /home/namhyung/project/linux/tools/perf/perf.c:313
>     #10 0x558c572b1a48 in handle_internal_command /home/namhyung/project/linux/tools/perf/perf.c:365
>     #11 0x558c572b1a48 in run_argv /home/namhyung/project/linux/tools/perf/perf.c:409
>     #12 0x558c572b1a48 in main /home/namhyung/project/linux/tools/perf/perf.c:539
>     #13 0x7fcadb9f7d09 in __libc_start_main ../csu/libc-start.c:308
>     #14 0x558c572b60f9 in _start (/home/namhyung/project/linux/tools/perf/perf+0x45d0f9)
>
> Actually the nodes in the hash table are struct perf_stream_id and
> they were freed in the previous run.  Fix it by resetting the hash.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/util/evlist.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index 5121b4db66fe..882cd1f721d9 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1306,6 +1306,7 @@ void evlist__close(struct evlist *evlist)
>                 perf_evsel__free_fd(&evsel->core);
>                 perf_evsel__free_id(&evsel->core);
>         }
> +       perf_evlist__reset_id_hash(&evlist->core);
>  }
>
>  static int evlist__create_syswide_maps(struct evlist *evlist)
> --
> 2.30.0.617.g56c4b15f3c-goog
>

Re: [PATCH 1/2] libperf: Add perf_evlist__reset_id_hash()

[Namhyung Kim]

On Wed, Feb 24, 2021 at 5:11 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Add the perf_evlist__reset_id_hash() function to libperf API so that
> it can be called to reset the hash table.  This is necessary for perf
> stat to run the workload multiple times.
>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
[SNIP]
> diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
> index 7be1af8a546c..285100143d89 100644
> --- a/tools/lib/perf/libperf.map
> +++ b/tools/lib/perf/libperf.map
> @@ -42,6 +42,7 @@ LIBPERF_0.0.1 {
>                 perf_evlist__munmap;
>                 perf_evlist__filter_pollfd;
>                 perf_evlist__next_mmap;
> +               perf_evlist__reset_id_hash;
>                 perf_mmap__consume;
>                 perf_mmap__read_init;
>                 perf_mmap__read_done;

I saw perf_evsel__free_fd and perf_evsel__free_id is called from
util/evlist.c without being listed here.  Do we need to add them?

Thanks,
Namhyung

Re: [PATCH 1/2] libperf: Add perf_evlist__reset_id_hash()

[Jiri Olsa]

On Wed, Feb 24, 2021 at 05:42:15PM +0900, Namhyung Kim wrote:
> On Wed, Feb 24, 2021 at 5:11 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > Add the perf_evlist__reset_id_hash() function to libperf API so that
> > it can be called to reset the hash table.  This is necessary for perf
> > stat to run the workload multiple times.
> >
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> > ---
> [SNIP]
> > diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map
> > index 7be1af8a546c..285100143d89 100644
> > --- a/tools/lib/perf/libperf.map
> > +++ b/tools/lib/perf/libperf.map
> > @@ -42,6 +42,7 @@ LIBPERF_0.0.1 {
> >                 perf_evlist__munmap;
> >                 perf_evlist__filter_pollfd;
> >                 perf_evlist__next_mmap;
> > +               perf_evlist__reset_id_hash;
> >                 perf_mmap__consume;
> >                 perf_mmap__read_init;
> >                 perf_mmap__read_done;
> 
> I saw perf_evsel__free_fd and perf_evsel__free_id is called from
> util/evlist.c without being listed here.  Do we need to add them?

perf is special user of libperf and we link it statically
at the moment, so it's not needed

I think we should add perf_evlist__reset_id_hash to internal
header, because for libperf.so we expose only logical API units
with described usage, not just single (special purpose) function

thanks,
jirka