[PATCH 1/4] perf cgroup: Avoid needless closing of unopened fd

[PATCH 1/4] perf cgroup: Avoid needless closing of unopened fd

[Tommi Rantala]

Do not bother with close() if fd is not valid, just to silence valgrind:

    $ valgrind ./perf script
    ==59169== Memcheck, a memory error detector
    ==59169== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
    ==59169== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
    ==59169== Command: ./perf script
    ==59169==
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
---
 tools/perf/util/cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index b73fb7823048..050dea9f1e88 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -107,7 +107,8 @@ static int add_cgroup(struct evlist *evlist, const char *str)
 
 static void cgroup__delete(struct cgroup *cgroup)
 {
-	close(cgroup->fd);
+	if (cgroup->fd >= 0)
+		close(cgroup->fd);
 	zfree(&cgroup->name);
 	free(cgroup);
 }
-- 
2.25.2

[PATCH 2/4] perf tools: Move zstd_fini() to session deletion

[Tommi Rantala]

Move zstd_fini() call to perf_session__delete(), so that we always
cleanup the zstd state when deleting the session.

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
---
 tools/perf/builtin-inject.c | 1 -
 tools/perf/builtin-record.c | 1 -
 tools/perf/builtin-report.c | 1 -
 tools/perf/util/session.c   | 1 +
 4 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 7e124a7b8bfd..1ffb8393357a 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -836,7 +836,6 @@ int cmd_inject(int argc, const char **argv)
 	ret = __cmd_inject(&inject);
 
 out_delete:
-	zstd_fini(&(inject.session->zstd_data));
 	perf_session__delete(inject.session);
 	return ret;
 }
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 1ab349abe904..8ed00de1ca29 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1827,7 +1827,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	}
 
 out_delete_session:
-	zstd_fini(&session->zstd_data);
 	perf_session__delete(session);
 
 	if (!opts->no_bpf_event)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 26d8fc27e427..e06e14980264 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1579,7 +1579,6 @@ int cmd_report(int argc, const char **argv)
 		report.block_reports = NULL;
 	}
 
-	zstd_fini(&(session->zstd_data));
 	perf_session__delete(session);
 	return ret;
 }
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 0b0bfe5bef17..64e8b794b0bc 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -302,6 +302,7 @@ void perf_session__delete(struct perf_session *session)
 	machines__exit(&session->machines);
 	if (session->data)
 		perf_data__close(session->data);
+	zstd_fini(&session->zstd_data);
 	free(session);
 }
 
-- 
2.25.2

[PATCH 3/4] perf tools: Fix segfaults due to missing zstd decompressor initialization

[Tommi Rantala]

Initialization of zstd decompressor state is done for "perf report" and
"perf inject", but missing for other tools, causing segfaults e.g. with
"perf script" and "perf annotate" when zstd compressed data is used:

  # ./perf record -z -a -- sleep 1
  # gdb -q --args ./perf script
  (gdb) run
  Program received signal SIGSEGV, Segmentation fault.
  0x00007ffff771d4cb in ZSTD_decompressStream () from /lib64/libzstd.so.1
  (gdb) bt
  #0  0x00007ffff771d4cb in ZSTD_decompressStream () from /lib64/libzstd.so.1
  #1  0x00000000005c92a8 in zstd_decompress_stream (data=0xc3f8e0, src=0x7ffff6dd3038, src_size=255, dst=0x7ffff61ec028, dst_size=528384) at util/zstd.c:100
  #2  0x00000000005262ba in perf_session__process_compressed_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:73
  #3  0x000000000052a596 in perf_session__process_user_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:1581
  #4  0x000000000052ab4b in perf_session__process_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:1713
  #5  0x000000000052bed6 in process_simple (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:2209
  #6  0x000000000052bcfe in reader__process_events (rd=0x7fffffffb400, session=0xc38cc0, prog=0x7fffffffb420) at util/session.c:2175
  #7  0x000000000052bfc2 in __perf_session__process_events (session=0xc38cc0) at util/session.c:2232
  #8  0x000000000052c0f3 in perf_session__process_events (session=0xc38cc0) at util/session.c:2265
  #9  0x0000000000447266 in __cmd_script (script=0x7fffffffb5c0) at builtin-script.c:2608
  #10 0x000000000044ba79 in cmd_script (argc=0, argv=0x7fffffffd330) at builtin-script.c:3988
  #11 0x00000000004bf2b8 in run_builtin (p=0xa398f8 <commands+408>, argc=1, argv=0x7fffffffd330) at perf.c:312
  #12 0x00000000004bf525 in handle_internal_command (argc=1, argv=0x7fffffffd330) at perf.c:364
  #13 0x00000000004bf66c in run_argv (argcp=0x7fffffffd18c, argv=0x7fffffffd180) at perf.c:408
  #14 0x00000000004bfa38 in main (argc=1, argv=0x7fffffffd330) at perf.c:538

Split zstd_init() into zstd_decomp_init() and zstd_comp_init(), so that
we can do lazy initialization for the decompressor, and handle it all in
perf_session to make it easily available to all the tools.

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
---
 tools/perf/builtin-inject.c |  3 ---
 tools/perf/builtin-record.c |  2 +-
 tools/perf/builtin-report.c |  3 ---
 tools/perf/util/compress.h  | 10 ++++++++--
 tools/perf/util/session.c   |  3 +++
 tools/perf/util/zstd.c      | 14 +++++++++++++-
 6 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 1ffb8393357a..8cc9dff9e66b 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -803,9 +803,6 @@ int cmd_inject(int argc, const char **argv)
 	if (IS_ERR(inject.session))
 		return PTR_ERR(inject.session);
 
-	if (zstd_init(&(inject.session->zstd_data), 0) < 0)
-		pr_warning("Decompression initialization failed.\n");
-
 	if (inject.build_ids) {
 		/*
 		 * to make sure the mmap records are ordered correctly
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8ed00de1ca29..fa9c59fc4fe0 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -1461,7 +1461,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	fd = perf_data__fd(data);
 	rec->session = session;
 
-	if (zstd_init(&session->zstd_data, rec->opts.comp_level) < 0) {
+	if (zstd_comp_init(&session->zstd_data, rec->opts.comp_level) < 0) {
 		pr_err("Compression initialization failed.\n");
 		return -1;
 	}
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index e06e14980264..b85fcdebdb5a 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1355,9 +1355,6 @@ int cmd_report(int argc, const char **argv)
 	if (ret)
 		return ret;
 
-	if (zstd_init(&(session->zstd_data), 0) < 0)
-		pr_warning("Decompression initialization failed. Reported data may be incomplete.\n");
-
 	if (report.queue_size) {
 		ordered_events__set_alloc_size(&session->ordered_events,
 					       report.queue_size);
diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
index 0cd3369af2a4..aff9ce60dfb8 100644
--- a/tools/perf/util/compress.h
+++ b/tools/perf/util/compress.h
@@ -26,7 +26,8 @@ struct zstd_data {
 
 #ifdef HAVE_ZSTD_SUPPORT
 
-int zstd_init(struct zstd_data *data, int level);
+int zstd_comp_init(struct zstd_data *data, int level);
+int zstd_decomp_init(struct zstd_data *data);
 int zstd_fini(struct zstd_data *data);
 
 size_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t dst_size,
@@ -37,7 +38,12 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
 			      void *dst, size_t dst_size);
 #else /* !HAVE_ZSTD_SUPPORT */
 
-static inline int zstd_init(struct zstd_data *data __maybe_unused, int level __maybe_unused)
+static inline int zstd_comp_init(struct zstd_data *data __maybe_unused, int level __maybe_unused)
+{
+	return 0;
+}
+
+static inline int zstd_decomp_init(struct zstd_data *data __maybe_unused)
 {
 	return 0;
 }
diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
index 64e8b794b0bc..2bba731e7cbf 100644
--- a/tools/perf/util/session.c
+++ b/tools/perf/util/session.c
@@ -45,6 +45,9 @@ static int perf_session__process_compressed_event(struct perf_session *session,
 	size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
 	struct decomp *decomp, *decomp_last = session->decomp_last;
 
+	if (zstd_decomp_init(&session->zstd_data) < 0)
+		return -1;
+
 	if (decomp_last) {
 		decomp_last_rem = decomp_last->size - decomp_last->head;
 		decomp_len += decomp_last_rem;
diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
index d2202392ffdb..d789665e8c0c 100644
--- a/tools/perf/util/zstd.c
+++ b/tools/perf/util/zstd.c
@@ -5,10 +5,13 @@
 #include "util/compress.h"
 #include "util/debug.h"
 
-int zstd_init(struct zstd_data *data, int level)
+int zstd_decomp_init(struct zstd_data *data)
 {
 	size_t ret;
 
+	if (data->dstream)
+		return 0;
+
 	data->dstream = ZSTD_createDStream();
 	if (data->dstream == NULL) {
 		pr_err("Couldn't create decompression stream.\n");
@@ -18,9 +21,18 @@ int zstd_init(struct zstd_data *data, int level)
 	ret = ZSTD_initDStream(data->dstream);
 	if (ZSTD_isError(ret)) {
 		pr_err("Failed to initialize decompression stream: %s\n", ZSTD_getErrorName(ret));
+		ZSTD_freeDStream(data->dstream);
+		data->dstream = NULL;
 		return -1;
 	}
 
+	return 0;
+}
+
+int zstd_comp_init(struct zstd_data *data, int level)
+{
+	size_t ret;
+
 	if (!level)
 		return 0;
 
-- 
2.25.2

[PATCH 4/4] perf bench: Fix div-by-zero if runtime is zero

[Tommi Rantala]

Fix div-by-zero if runtime is zero:

  $ perf bench futex hash --runtime=0
  # Running 'futex/hash' benchmark:
  Run summary [PID 12090]: 4 threads, each operating on 1024 [private] futexes for 0 secs.
  Floating point exception (core dumped)

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
---
 tools/perf/bench/epoll-wait.c    | 3 ++-
 tools/perf/bench/futex-hash.c    | 3 ++-
 tools/perf/bench/futex-lock-pi.c | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/bench/epoll-wait.c b/tools/perf/bench/epoll-wait.c
index f938c585d512..cf797362675b 100644
--- a/tools/perf/bench/epoll-wait.c
+++ b/tools/perf/bench/epoll-wait.c
@@ -519,7 +519,8 @@ int bench_epoll_wait(int argc, const char **argv)
 		qsort(worker, nthreads, sizeof(struct worker), cmpworker);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 
 		update_stats(&throughput_stats, t);
 
diff --git a/tools/perf/bench/futex-hash.c b/tools/perf/bench/futex-hash.c
index 65eebe06c04d..915bf3da7ce2 100644
--- a/tools/perf/bench/futex-hash.c
+++ b/tools/perf/bench/futex-hash.c
@@ -205,7 +205,8 @@ int bench_futex_hash(int argc, const char **argv)
 	pthread_mutex_destroy(&thread_lock);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 		update_stats(&throughput_stats, t);
 		if (!silent) {
 			if (nfutexes == 1)
diff --git a/tools/perf/bench/futex-lock-pi.c b/tools/perf/bench/futex-lock-pi.c
index 89fd8f325f38..bb25d8beb3b8 100644
--- a/tools/perf/bench/futex-lock-pi.c
+++ b/tools/perf/bench/futex-lock-pi.c
@@ -211,7 +211,8 @@ int bench_futex_lock_pi(int argc, const char **argv)
 	pthread_mutex_destroy(&thread_lock);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 
 		update_stats(&throughput_stats, t);
 		if (!silent)
-- 
2.25.2

Re: [PATCH 1/4] perf cgroup: Avoid needless closing of unopened fd

[Jiri Olsa]

On Fri, Apr 17, 2020 at 04:23:26PM +0300, Tommi Rantala wrote:
> Do not bother with close() if fd is not valid, just to silence valgrind:
> 
>     $ valgrind ./perf script
>     ==59169== Memcheck, a memory error detector
>     ==59169== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
>     ==59169== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
>     ==59169== Command: ./perf script
>     ==59169==
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
>     ==59169== Warning: invalid file descriptor -1 in syscall close()
> 
> Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  tools/perf/util/cgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
> index b73fb7823048..050dea9f1e88 100644
> --- a/tools/perf/util/cgroup.c
> +++ b/tools/perf/util/cgroup.c
> @@ -107,7 +107,8 @@ static int add_cgroup(struct evlist *evlist, const char *str)
>  
>  static void cgroup__delete(struct cgroup *cgroup)
>  {
> -	close(cgroup->fd);
> +	if (cgroup->fd >= 0)
> +		close(cgroup->fd);
>  	zfree(&cgroup->name);
>  	free(cgroup);
>  }
> -- 
> 2.25.2
> 

Re: [PATCH 2/4] perf tools: Move zstd_fini() to session deletion

[Jiri Olsa]

On Fri, Apr 17, 2020 at 04:23:27PM +0300, Tommi Rantala wrote:
> Move zstd_fini() call to perf_session__delete(), so that we always
> cleanup the zstd state when deleting the session.

it shold be orthogonal with zstd_init calls, which
are not currently called within perf_session 

I guess  zstd_initcould moved to perf_session__new,
just need some nice way to pass rec->opts.comp_level

jirka

> 
> Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
> ---
>  tools/perf/builtin-inject.c | 1 -
>  tools/perf/builtin-record.c | 1 -
>  tools/perf/builtin-report.c | 1 -
>  tools/perf/util/session.c   | 1 +
>  4 files changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 7e124a7b8bfd..1ffb8393357a 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -836,7 +836,6 @@ int cmd_inject(int argc, const char **argv)
>  	ret = __cmd_inject(&inject);
>  
>  out_delete:
> -	zstd_fini(&(inject.session->zstd_data));
>  	perf_session__delete(inject.session);
>  	return ret;
>  }
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 1ab349abe904..8ed00de1ca29 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1827,7 +1827,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	}
>  
>  out_delete_session:
> -	zstd_fini(&session->zstd_data);
>  	perf_session__delete(session);
>  
>  	if (!opts->no_bpf_event)
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 26d8fc27e427..e06e14980264 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1579,7 +1579,6 @@ int cmd_report(int argc, const char **argv)
>  		report.block_reports = NULL;
>  	}
>  
> -	zstd_fini(&(session->zstd_data));
>  	perf_session__delete(session);
>  	return ret;
>  }
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 0b0bfe5bef17..64e8b794b0bc 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -302,6 +302,7 @@ void perf_session__delete(struct perf_session *session)
>  	machines__exit(&session->machines);
>  	if (session->data)
>  		perf_data__close(session->data);
> +	zstd_fini(&session->zstd_data);
>  	free(session);
>  }
>  
> -- 
> 2.25.2
> 

Re: [PATCH 3/4] perf tools: Fix segfaults due to missing zstd decompressor initialization

[Jiri Olsa]

On Fri, Apr 17, 2020 at 04:23:28PM +0300, Tommi Rantala wrote:
> Initialization of zstd decompressor state is done for "perf report" and
> "perf inject", but missing for other tools, causing segfaults e.g. with
> "perf script" and "perf annotate" when zstd compressed data is used:
> 
>   # ./perf record -z -a -- sleep 1
>   # gdb -q --args ./perf script
>   (gdb) run
>   Program received signal SIGSEGV, Segmentation fault.
>   0x00007ffff771d4cb in ZSTD_decompressStream () from /lib64/libzstd.so.1
>   (gdb) bt
>   #0  0x00007ffff771d4cb in ZSTD_decompressStream () from /lib64/libzstd.so.1
>   #1  0x00000000005c92a8 in zstd_decompress_stream (data=0xc3f8e0, src=0x7ffff6dd3038, src_size=255, dst=0x7ffff61ec028, dst_size=528384) at util/zstd.c:100
>   #2  0x00000000005262ba in perf_session__process_compressed_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:73
>   #3  0x000000000052a596 in perf_session__process_user_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:1581
>   #4  0x000000000052ab4b in perf_session__process_event (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:1713
>   #5  0x000000000052bed6 in process_simple (session=0xc38cc0, event=0x7ffff6dd3030, file_offset=11948080) at util/session.c:2209
>   #6  0x000000000052bcfe in reader__process_events (rd=0x7fffffffb400, session=0xc38cc0, prog=0x7fffffffb420) at util/session.c:2175
>   #7  0x000000000052bfc2 in __perf_session__process_events (session=0xc38cc0) at util/session.c:2232
>   #8  0x000000000052c0f3 in perf_session__process_events (session=0xc38cc0) at util/session.c:2265
>   #9  0x0000000000447266 in __cmd_script (script=0x7fffffffb5c0) at builtin-script.c:2608
>   #10 0x000000000044ba79 in cmd_script (argc=0, argv=0x7fffffffd330) at builtin-script.c:3988
>   #11 0x00000000004bf2b8 in run_builtin (p=0xa398f8 <commands+408>, argc=1, argv=0x7fffffffd330) at perf.c:312
>   #12 0x00000000004bf525 in handle_internal_command (argc=1, argv=0x7fffffffd330) at perf.c:364
>   #13 0x00000000004bf66c in run_argv (argcp=0x7fffffffd18c, argv=0x7fffffffd180) at perf.c:408
>   #14 0x00000000004bfa38 in main (argc=1, argv=0x7fffffffd330) at perf.c:538
> 
> Split zstd_init() into zstd_decomp_init() and zstd_comp_init(), so that
> we can do lazy initialization for the decompressor, and handle it all in
> perf_session to make it easily available to all the tools.

Alexey, could you please check on this one?

thanks,
jirka

> 
> Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
> ---
>  tools/perf/builtin-inject.c |  3 ---
>  tools/perf/builtin-record.c |  2 +-
>  tools/perf/builtin-report.c |  3 ---
>  tools/perf/util/compress.h  | 10 ++++++++--
>  tools/perf/util/session.c   |  3 +++
>  tools/perf/util/zstd.c      | 14 +++++++++++++-
>  6 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 1ffb8393357a..8cc9dff9e66b 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -803,9 +803,6 @@ int cmd_inject(int argc, const char **argv)
>  	if (IS_ERR(inject.session))
>  		return PTR_ERR(inject.session);
>  
> -	if (zstd_init(&(inject.session->zstd_data), 0) < 0)
> -		pr_warning("Decompression initialization failed.\n");
> -
>  	if (inject.build_ids) {
>  		/*
>  		 * to make sure the mmap records are ordered correctly
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 8ed00de1ca29..fa9c59fc4fe0 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -1461,7 +1461,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>  	fd = perf_data__fd(data);
>  	rec->session = session;
>  
> -	if (zstd_init(&session->zstd_data, rec->opts.comp_level) < 0) {
> +	if (zstd_comp_init(&session->zstd_data, rec->opts.comp_level) < 0) {
>  		pr_err("Compression initialization failed.\n");
>  		return -1;
>  	}
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index e06e14980264..b85fcdebdb5a 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1355,9 +1355,6 @@ int cmd_report(int argc, const char **argv)
>  	if (ret)
>  		return ret;
>  
> -	if (zstd_init(&(session->zstd_data), 0) < 0)
> -		pr_warning("Decompression initialization failed. Reported data may be incomplete.\n");
> -
>  	if (report.queue_size) {
>  		ordered_events__set_alloc_size(&session->ordered_events,
>  					       report.queue_size);
> diff --git a/tools/perf/util/compress.h b/tools/perf/util/compress.h
> index 0cd3369af2a4..aff9ce60dfb8 100644
> --- a/tools/perf/util/compress.h
> +++ b/tools/perf/util/compress.h
> @@ -26,7 +26,8 @@ struct zstd_data {
>  
>  #ifdef HAVE_ZSTD_SUPPORT
>  
> -int zstd_init(struct zstd_data *data, int level);
> +int zstd_comp_init(struct zstd_data *data, int level);
> +int zstd_decomp_init(struct zstd_data *data);
>  int zstd_fini(struct zstd_data *data);
>  
>  size_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_t dst_size,
> @@ -37,7 +38,12 @@ size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size
>  			      void *dst, size_t dst_size);
>  #else /* !HAVE_ZSTD_SUPPORT */
>  
> -static inline int zstd_init(struct zstd_data *data __maybe_unused, int level __maybe_unused)
> +static inline int zstd_comp_init(struct zstd_data *data __maybe_unused, int level __maybe_unused)
> +{
> +	return 0;
> +}
> +
> +static inline int zstd_decomp_init(struct zstd_data *data __maybe_unused)
>  {
>  	return 0;
>  }
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 64e8b794b0bc..2bba731e7cbf 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -45,6 +45,9 @@ static int perf_session__process_compressed_event(struct perf_session *session,
>  	size_t mmap_len, decomp_len = session->header.env.comp_mmap_len;
>  	struct decomp *decomp, *decomp_last = session->decomp_last;
>  
> +	if (zstd_decomp_init(&session->zstd_data) < 0)
> +		return -1;
> +
>  	if (decomp_last) {
>  		decomp_last_rem = decomp_last->size - decomp_last->head;
>  		decomp_len += decomp_last_rem;
> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index d2202392ffdb..d789665e8c0c 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
> @@ -5,10 +5,13 @@
>  #include "util/compress.h"
>  #include "util/debug.h"
>  
> -int zstd_init(struct zstd_data *data, int level)
> +int zstd_decomp_init(struct zstd_data *data)
>  {
>  	size_t ret;
>  
> +	if (data->dstream)
> +		return 0;
> +
>  	data->dstream = ZSTD_createDStream();
>  	if (data->dstream == NULL) {
>  		pr_err("Couldn't create decompression stream.\n");
> @@ -18,9 +21,18 @@ int zstd_init(struct zstd_data *data, int level)
>  	ret = ZSTD_initDStream(data->dstream);
>  	if (ZSTD_isError(ret)) {
>  		pr_err("Failed to initialize decompression stream: %s\n", ZSTD_getErrorName(ret));
> +		ZSTD_freeDStream(data->dstream);
> +		data->dstream = NULL;
>  		return -1;
>  	}
>  
> +	return 0;
> +}
> +
> +int zstd_comp_init(struct zstd_data *data, int level)
> +{
> +	size_t ret;
> +
>  	if (!level)
>  		return 0;
>  
> -- 
> 2.25.2
> 

Re: [PATCH 4/4] perf bench: Fix div-by-zero if runtime is zero

[Jiri Olsa]

On Fri, Apr 17, 2020 at 04:23:29PM +0300, Tommi Rantala wrote:
> Fix div-by-zero if runtime is zero:
> 
>   $ perf bench futex hash --runtime=0
>   # Running 'futex/hash' benchmark:
>   Run summary [PID 12090]: 4 threads, each operating on 1024 [private] futexes for 0 secs.
>   Floating point exception (core dumped)
> 
> Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>

Acked-by: Jiri Olsa <jolsa@redhat.com>

thanks,
jirka

> ---
>  tools/perf/bench/epoll-wait.c    | 3 ++-
>  tools/perf/bench/futex-hash.c    | 3 ++-
>  tools/perf/bench/futex-lock-pi.c | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/perf/bench/epoll-wait.c b/tools/perf/bench/epoll-wait.c
> index f938c585d512..cf797362675b 100644
> --- a/tools/perf/bench/epoll-wait.c
> +++ b/tools/perf/bench/epoll-wait.c
> @@ -519,7 +519,8 @@ int bench_epoll_wait(int argc, const char **argv)
>  		qsort(worker, nthreads, sizeof(struct worker), cmpworker);
>  
>  	for (i = 0; i < nthreads; i++) {
> -		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
> +		unsigned long t = bench__runtime.tv_sec > 0 ?
> +			worker[i].ops / bench__runtime.tv_sec : 0;
>  
>  		update_stats(&throughput_stats, t);
>  
> diff --git a/tools/perf/bench/futex-hash.c b/tools/perf/bench/futex-hash.c
> index 65eebe06c04d..915bf3da7ce2 100644
> --- a/tools/perf/bench/futex-hash.c
> +++ b/tools/perf/bench/futex-hash.c
> @@ -205,7 +205,8 @@ int bench_futex_hash(int argc, const char **argv)
>  	pthread_mutex_destroy(&thread_lock);
>  
>  	for (i = 0; i < nthreads; i++) {
> -		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
> +		unsigned long t = bench__runtime.tv_sec > 0 ?
> +			worker[i].ops / bench__runtime.tv_sec : 0;
>  		update_stats(&throughput_stats, t);
>  		if (!silent) {
>  			if (nfutexes == 1)
> diff --git a/tools/perf/bench/futex-lock-pi.c b/tools/perf/bench/futex-lock-pi.c
> index 89fd8f325f38..bb25d8beb3b8 100644
> --- a/tools/perf/bench/futex-lock-pi.c
> +++ b/tools/perf/bench/futex-lock-pi.c
> @@ -211,7 +211,8 @@ int bench_futex_lock_pi(int argc, const char **argv)
>  	pthread_mutex_destroy(&thread_lock);
>  
>  	for (i = 0; i < nthreads; i++) {
> -		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
> +		unsigned long t = bench__runtime.tv_sec > 0 ?
> +			worker[i].ops / bench__runtime.tv_sec : 0;
>  
>  		update_stats(&throughput_stats, t);
>  		if (!silent)
> -- 
> 2.25.2
> 

Re: [PATCH 4/4] perf bench: Fix div-by-zero if runtime is zero

[Arnaldo Carvalho de Melo]

Em Mon, Apr 20, 2020 at 11:05:26AM +0200, Jiri Olsa escreveu:
> On Fri, Apr 17, 2020 at 04:23:29PM +0300, Tommi Rantala wrote:
> > Fix div-by-zero if runtime is zero:
> > 
> >   $ perf bench futex hash --runtime=0
> >   # Running 'futex/hash' benchmark:
> >   Run summary [PID 12090]: 4 threads, each operating on 1024 [private] futexes for 0 secs.
> >   Floating point exception (core dumped)
> > 
> > Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied,

- Arnaldo

Re: [PATCH 1/4] perf cgroup: Avoid needless closing of unopened fd

[Arnaldo Carvalho de Melo]

Em Mon, Apr 20, 2020 at 10:48:47AM +0200, Jiri Olsa escreveu:
> On Fri, Apr 17, 2020 at 04:23:26PM +0300, Tommi Rantala wrote:
> > Do not bother with close() if fd is not valid, just to silence valgrind:
> > 
> >     $ valgrind ./perf script
> >     ==59169== Memcheck, a memory error detector
> >     ==59169== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
> >     ==59169== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
> >     ==59169== Command: ./perf script
> >     ==59169==
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> >     ==59169== Warning: invalid file descriptor -1 in syscall close()
> > 
> > Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
> 
> Acked-by: Jiri Olsa <jolsa@redhat.com>

Thanks, applied,

- Arnaldo

Re: [PATCH 2/4] perf tools: Move zstd_fini() to session deletion

[Rantala, Tommi T. (Nokia - FI/Espoo)]

On Mon, 2020-04-20 at 10:54 +0200, Jiri Olsa wrote:
> On Fri, Apr 17, 2020 at 04:23:27PM +0300, Tommi Rantala wrote:
> > Move zstd_fini() call to perf_session__delete(), so that we always
> > cleanup the zstd state when deleting the session.
> 
> it shold be orthogonal with zstd_init calls, which
> are not currently called within perf_session 
> 
> I guess  zstd_initcould moved to perf_session__new,
> just need some nice way to pass rec->opts.comp_level

Makes sense, I'll post v2 later.
-Tommi

[tip: perf/core] perf cgroup: Avoid needless closing of unopened fd

[tip-bot2 for Tommi Rantala]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     d2e7d8636fb7d3e30aa8f894003f9e293ea62eea
Gitweb:        https://git.kernel.org/tip/d2e7d8636fb7d3e30aa8f894003f9e293ea62eea
Author:        Tommi Rantala <tommi.t.rantala@nokia.com>
AuthorDate:    Fri, 17 Apr 2020 16:23:26 +03:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 22 Apr 2020 10:01:33 -03:00

perf cgroup: Avoid needless closing of unopened fd

Do not bother with close() if fd is not valid, just to silence valgrind:

    $ valgrind ./perf script
    ==59169== Memcheck, a memory error detector
    ==59169== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
    ==59169== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
    ==59169== Command: ./perf script
    ==59169==
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()
    ==59169== Warning: invalid file descriptor -1 in syscall close()

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Link: http://lore.kernel.org/lkml/20200417132330.119407-1-tommi.t.rantala@nokia.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/cgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/cgroup.c b/tools/perf/util/cgroup.c
index b73fb78..050dea9 100644
--- a/tools/perf/util/cgroup.c
+++ b/tools/perf/util/cgroup.c
@@ -107,7 +107,8 @@ found:
 
 static void cgroup__delete(struct cgroup *cgroup)
 {
-	close(cgroup->fd);
+	if (cgroup->fd >= 0)
+		close(cgroup->fd);
 	zfree(&cgroup->name);
 	free(cgroup);
 }

[tip: perf/core] perf bench: Fix div-by-zero if runtime is zero

[tip-bot2 for Tommi Rantala]

The following commit has been merged into the perf/core branch of tip:

Commit-ID:     41e7c32b978974adaadd4808ba42f9026634dca3
Gitweb:        https://git.kernel.org/tip/41e7c32b978974adaadd4808ba42f9026634dca3
Author:        Tommi Rantala <tommi.t.rantala@nokia.com>
AuthorDate:    Fri, 17 Apr 2020 16:23:29 +03:00
Committer:     Arnaldo Carvalho de Melo <acme@redhat.com>
CommitterDate: Wed, 22 Apr 2020 10:01:33 -03:00

perf bench: Fix div-by-zero if runtime is zero

Fix div-by-zero if runtime is zero:

  $ perf bench futex hash --runtime=0
  # Running 'futex/hash' benchmark:
  Run summary [PID 12090]: 4 threads, each operating on 1024 [private] futexes for 0 secs.
  Floating point exception (core dumped)

Signed-off-by: Tommi Rantala <tommi.t.rantala@nokia.com>
Acked-by: Jiri Olsa <jolsa@redhat.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Darren Hart <dvhart@infradead.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lore.kernel.org/lkml/20200417132330.119407-4-tommi.t.rantala@nokia.com
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/bench/epoll-wait.c    | 3 ++-
 tools/perf/bench/futex-hash.c    | 3 ++-
 tools/perf/bench/futex-lock-pi.c | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/perf/bench/epoll-wait.c b/tools/perf/bench/epoll-wait.c
index f938c58..cf79736 100644
--- a/tools/perf/bench/epoll-wait.c
+++ b/tools/perf/bench/epoll-wait.c
@@ -519,7 +519,8 @@ int bench_epoll_wait(int argc, const char **argv)
 		qsort(worker, nthreads, sizeof(struct worker), cmpworker);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 
 		update_stats(&throughput_stats, t);
 
diff --git a/tools/perf/bench/futex-hash.c b/tools/perf/bench/futex-hash.c
index 65eebe0..915bf3d 100644
--- a/tools/perf/bench/futex-hash.c
+++ b/tools/perf/bench/futex-hash.c
@@ -205,7 +205,8 @@ int bench_futex_hash(int argc, const char **argv)
 	pthread_mutex_destroy(&thread_lock);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 		update_stats(&throughput_stats, t);
 		if (!silent) {
 			if (nfutexes == 1)
diff --git a/tools/perf/bench/futex-lock-pi.c b/tools/perf/bench/futex-lock-pi.c
index 89fd8f3..bb25d8b 100644
--- a/tools/perf/bench/futex-lock-pi.c
+++ b/tools/perf/bench/futex-lock-pi.c
@@ -211,7 +211,8 @@ int bench_futex_lock_pi(int argc, const char **argv)
 	pthread_mutex_destroy(&thread_lock);
 
 	for (i = 0; i < nthreads; i++) {
-		unsigned long t = worker[i].ops / bench__runtime.tv_sec;
+		unsigned long t = bench__runtime.tv_sec > 0 ?
+			worker[i].ops / bench__runtime.tv_sec : 0;
 
 		update_stats(&throughput_stats, t);
 		if (!silent)