[PATCH v3 1/3] perf tools: Handle EINTR error for readn/writen

[PATCH v3 1/3] perf tools: Handle EINTR error for readn/writen

[Namhyung Kim]

Those readn/writen functions are to ensure read/write does I/O for
a given size exactly.  But ion() - its implementation - does not
handle in case it returns prematurely due to a signal.  As it's not
an error itself so just retry the operation.

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

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9f66549562bd..7fff6be07f07 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -166,6 +166,8 @@ static ssize_t ion(bool is_read, int fd, void *buf, size_t n)
 		ssize_t ret = is_read ? read(fd, buf, left) :
 					write(fd, buf, left);
 
+		if (ret < 0 && errno == EINTR)
+			continue;
 		if (ret <= 0)
 			return ret;
 
-- 
1.7.9.2

[PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Namhyung Kim]

Currently perf record doesn't propagate the exit status of a workload
given by the command line.  But sometimes it'd useful if it's
propagated so that a monitoring script can handle errors
appropriately.

To do that, it got rid of exit handlers and run/call them directly in
the __cmd_record().  I don't see any reason why those are in a form of
exit handlers in the first place.  Also it cleaned up the resouce
management code in record__exit().

With this change, perf record returns the child exit status in case of
normal termination.  (Not sure what should be returned on abnormal
cases though).

Example run of Stephane's case:

  $ perf record true && echo yes || echo no
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.013 MB perf.data (~589 samples) ]
  yes

  $ perf record false && echo yes || echo no
  [ perf record: Woken up 1 times to write data ]
  [ perf record: Captured and wrote 0.013 MB perf.data (~589 samples) ]
  no

And Jiri's case (error in parent):

  $ perf record -m 10G true && echo yes || echo no
  rounding mmap pages size to 17179869184 bytes (4194304 pages)
  failed to mmap with 12 (Cannot allocate memory)
  no

Reported-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c |  121 ++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 70 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index eb524f91bffe..b1523d8a6191 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -152,26 +152,6 @@ static void sig_handler(int sig)
 	signr = sig;
 }
 
-static void record__sig_exit(int exit_status __maybe_unused, void *arg)
-{
-	struct record *rec = arg;
-	int status;
-
-	if (rec->evlist->workload.pid > 0) {
-		if (!child_finished)
-			kill(rec->evlist->workload.pid, SIGTERM);
-
-		wait(&status);
-		if (WIFSIGNALED(status))
-			psignal(WTERMSIG(status), rec->progname);
-	}
-
-	if (signr == -1 || signr == SIGUSR1)
-		return;
-
-	signal(signr, SIG_DFL);
-}
-
 static int record__open(struct record *rec)
 {
 	char msg[512];
@@ -243,27 +223,6 @@ static int process_buildids(struct record *rec)
 					      size, &build_id__mark_dso_hit_ops);
 }
 
-static void record__exit(int status, void *arg)
-{
-	struct record *rec = arg;
-	struct perf_data_file *file = &rec->file;
-
-	if (status != 0)
-		return;
-
-	if (!file->is_pipe) {
-		rec->session->header.data_size += rec->bytes_written;
-
-		if (!rec->no_buildid)
-			process_buildids(rec);
-		perf_session__write_header(rec->session, rec->evlist,
-					   file->fd, true);
-		perf_session__delete(rec->session);
-		perf_evlist__delete(rec->evlist);
-		symbol__exit();
-	}
-}
-
 static void perf_event__synthesize_guest_os(struct machine *machine, void *data)
 {
 	int err;
@@ -356,6 +315,7 @@ static void workload_exec_failed_signal(int signo, siginfo_t *info,
 static int __cmd_record(struct record *rec, int argc, const char **argv)
 {
 	int err;
+	int status = 0;
 	unsigned long waking = 0;
 	const bool forks = argc > 0;
 	struct machine *machine;
@@ -367,7 +327,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 
 	rec->progname = argv[0];
 
-	on_exit(record__sig_exit, rec);
 	signal(SIGCHLD, sig_handler);
 	signal(SIGINT, sig_handler);
 	signal(SIGTERM, sig_handler);
@@ -394,26 +353,21 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 
 	if (record__open(rec) != 0) {
 		err = -1;
-		goto out_delete_session;
+		goto out_child;
 	}
 
 	if (!rec->evlist->nr_groups)
 		perf_header__clear_feat(&session->header, HEADER_GROUP_DESC);
 
-	/*
-	 * perf_session__delete(session) will be called at record__exit()
-	 */
-	on_exit(record__exit, rec);
-
 	if (file->is_pipe) {
 		err = perf_header__write_pipe(file->fd);
 		if (err < 0)
-			goto out_delete_session;
+			goto out_child;
 	} else {
 		err = perf_session__write_header(session, rec->evlist,
 						 file->fd, false);
 		if (err < 0)
-			goto out_delete_session;
+			goto out_child;
 	}
 
 	if (!rec->no_buildid
@@ -421,7 +375,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		pr_err("Couldn't generate buildids. "
 		       "Use --no-buildid to profile anyway.\n");
 		err = -1;
-		goto out_delete_session;
+		goto out_child;
 	}
 
 	machine = &session->machines.host;
@@ -431,7 +385,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 						   process_synthesized_event);
 		if (err < 0) {
 			pr_err("Couldn't synthesize attrs.\n");
-			goto out_delete_session;
+			goto out_child;
 		}
 
 		if (have_tracepoints(&rec->evlist->entries)) {
@@ -447,7 +401,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 								  process_synthesized_event);
 			if (err <= 0) {
 				pr_err("Couldn't record tracing data.\n");
-				goto out_delete_session;
+				goto out_child;
 			}
 			rec->bytes_written += err;
 		}
@@ -475,7 +429,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 	err = __machine__synthesize_threads(machine, tool, &opts->target, rec->evlist->threads,
 					    process_synthesized_event, opts->sample_address);
 	if (err != 0)
-		goto out_delete_session;
+		goto out_child;
 
 	if (rec->realtime_prio) {
 		struct sched_param param;
@@ -484,7 +438,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		if (sched_setscheduler(0, SCHED_FIFO, &param)) {
 			pr_err("Could not set realtime priority.\n");
 			err = -1;
-			goto out_delete_session;
+			goto out_child;
 		}
 	}
 
@@ -512,13 +466,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 
 		if (record__mmap_read_all(rec) < 0) {
 			err = -1;
-			goto out_delete_session;
+			goto out_child;
 		}
 
 		if (hits == rec->samples) {
 			if (done)
 				break;
 			err = poll(rec->evlist->pollfd, rec->evlist->nr_fds, -1);
+			if (err < 0 && errno == EINTR)
+				err = 0;
 			waking++;
 		}
 
@@ -538,28 +494,52 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		const char *emsg = strerror_r(workload_exec_errno, msg, sizeof(msg));
 		pr_err("Workload failed: %s\n", emsg);
 		err = -1;
-		goto out_delete_session;
+		goto out_child;
 	}
 
-	if (quiet || signr == SIGUSR1)
-		return 0;
+	if (!quiet) {
+		fprintf(stderr, "[ perf record: Woken up %ld times to write data ]\n", waking);
 
-	fprintf(stderr, "[ perf record: Woken up %ld times to write data ]\n", waking);
+		/*
+		 * Approximate RIP event size: 24 bytes.
+		 */
+		fprintf(stderr,
+			"[ perf record: Captured and wrote %.3f MB %s (~%" PRIu64 " samples) ]\n",
+			(double)rec->bytes_written / 1024.0 / 1024.0,
+			file->path,
+			rec->bytes_written / 24);
+	}
 
-	/*
-	 * Approximate RIP event size: 24 bytes.
-	 */
-	fprintf(stderr,
-		"[ perf record: Captured and wrote %.3f MB %s (~%" PRIu64 " samples) ]\n",
-		(double)rec->bytes_written / 1024.0 / 1024.0,
-		file->path,
-		rec->bytes_written / 24);
+out_child:
+	if (forks) {
+		int exit_status;
 
-	return 0;
+		if (!child_finished)
+			kill(rec->evlist->workload.pid, SIGTERM);
+
+		wait(&exit_status);
+
+		if (err < 0)
+			status = err;
+		else if (WIFEXITED(exit_status))
+			status = WEXITSTATUS(exit_status);
+		else if (WIFSIGNALED(status))
+			psignal(WTERMSIG(status), rec->progname);
+	} else
+		status = err;
+
+	if (!err && !file->is_pipe) {
+		rec->session->header.data_size += rec->bytes_written;
+
+		if (!rec->no_buildid)
+			process_buildids(rec);
+		perf_session__write_header(rec->session, rec->evlist,
+					   file->fd, true);
+	}
 
 out_delete_session:
 	perf_session__delete(session);
-	return err;
+	return status;
 }
 
 #define BRANCH_OPT(n, m) \
@@ -988,6 +968,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
 
 	err = __cmd_record(&record, argc, argv);
 out_symbol_exit:
+	perf_evlist__delete(rec->evlist);
 	symbol__exit();
 	return err;
 }
-- 
1.7.9.2

[PATCH v3 3/3] perf tools: Get rid of on_exit() feature test

[Namhyung Kim]

The on_exit() function was only used in perf record but it's gone in
previous patch.

Cc: Bernhard Rosenkraenzer <Bernhard.Rosenkranzer@linaro.org>
Cc: Irina Tirdea <irina.tirdea@intel.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c                     |   31 -----------------------
 tools/perf/config/Makefile                      |    8 ------
 tools/perf/config/feature-checks/Makefile       |    4 ---
 tools/perf/config/feature-checks/test-all.c     |    5 ----
 tools/perf/config/feature-checks/test-on-exit.c |   16 ------------
 5 files changed, 64 deletions(-)
 delete mode 100644 tools/perf/config/feature-checks/test-on-exit.c

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index b1523d8a6191..1c127806fcb1 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -30,37 +30,6 @@
 #include <sched.h>
 #include <sys/mman.h>
 
-#ifndef HAVE_ON_EXIT_SUPPORT
-#ifndef ATEXIT_MAX
-#define ATEXIT_MAX 32
-#endif
-static int __on_exit_count = 0;
-typedef void (*on_exit_func_t) (int, void *);
-static on_exit_func_t __on_exit_funcs[ATEXIT_MAX];
-static void *__on_exit_args[ATEXIT_MAX];
-static int __exitcode = 0;
-static void __handle_on_exit_funcs(void);
-static int on_exit(on_exit_func_t function, void *arg);
-#define exit(x) (exit)(__exitcode = (x))
-
-static int on_exit(on_exit_func_t function, void *arg)
-{
-	if (__on_exit_count == ATEXIT_MAX)
-		return -ENOMEM;
-	else if (__on_exit_count == 0)
-		atexit(__handle_on_exit_funcs);
-	__on_exit_funcs[__on_exit_count] = function;
-	__on_exit_args[__on_exit_count++] = arg;
-	return 0;
-}
-
-static void __handle_on_exit_funcs(void)
-{
-	int i;
-	for (i = 0; i < __on_exit_count; i++)
-		__on_exit_funcs[i] (__exitcode, __on_exit_args[i]);
-}
-#endif
 
 struct record {
 	struct perf_tool	tool;
diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
index ee21fa95ebcf..7b7003d11983 100644
--- a/tools/perf/config/Makefile
+++ b/tools/perf/config/Makefile
@@ -156,7 +156,6 @@ CORE_FEATURE_TESTS =			\
 	libpython-version		\
 	libslang			\
 	libunwind			\
-	on-exit				\
 	stackprotector-all		\
 	timerfd				\
 	libdw-dwarf-unwind
@@ -182,7 +181,6 @@ VF_FEATURE_TESTS =			\
 	libelf-getphdrnum		\
 	libelf-mmap			\
 	libpython-version		\
-	on-exit				\
 	stackprotector-all		\
 	timerfd				\
 	libunwind-debug-frame		\
@@ -541,12 +539,6 @@ ifneq ($(filter -lbfd,$(EXTLIBS)),)
   CFLAGS += -DHAVE_LIBBFD_SUPPORT
 endif
 
-ifndef NO_ON_EXIT
-  ifeq ($(feature-on-exit), 1)
-    CFLAGS += -DHAVE_ON_EXIT_SUPPORT
-  endif
-endif
-
 ifndef NO_BACKTRACE
   ifeq ($(feature-backtrace), 1)
     CFLAGS += -DHAVE_BACKTRACE_SUPPORT
diff --git a/tools/perf/config/feature-checks/Makefile b/tools/perf/config/feature-checks/Makefile
index 2da103c53f89..64c84e5f0514 100644
--- a/tools/perf/config/feature-checks/Makefile
+++ b/tools/perf/config/feature-checks/Makefile
@@ -24,7 +24,6 @@ FILES=					\
 	test-libslang.bin		\
 	test-libunwind.bin		\
 	test-libunwind-debug-frame.bin	\
-	test-on-exit.bin		\
 	test-stackprotector-all.bin	\
 	test-timerfd.bin		\
 	test-libdw-dwarf-unwind.bin
@@ -133,9 +132,6 @@ test-liberty-z.bin:
 test-cplus-demangle.bin:
 	$(BUILD) -liberty
 
-test-on-exit.bin:
-	$(BUILD)
-
 test-backtrace.bin:
 	$(BUILD)
 
diff --git a/tools/perf/config/feature-checks/test-all.c b/tools/perf/config/feature-checks/test-all.c
index fc37eb3ca17b..fe5c1e5c952f 100644
--- a/tools/perf/config/feature-checks/test-all.c
+++ b/tools/perf/config/feature-checks/test-all.c
@@ -69,10 +69,6 @@
 # include "test-libbfd.c"
 #undef main
 
-#define main main_test_on_exit
-# include "test-on-exit.c"
-#undef main
-
 #define main main_test_backtrace
 # include "test-backtrace.c"
 #undef main
@@ -110,7 +106,6 @@ int main(int argc, char *argv[])
 	main_test_gtk2(argc, argv);
 	main_test_gtk2_infobar(argc, argv);
 	main_test_libbfd();
-	main_test_on_exit();
 	main_test_backtrace();
 	main_test_libnuma();
 	main_test_timerfd();
diff --git a/tools/perf/config/feature-checks/test-on-exit.c b/tools/perf/config/feature-checks/test-on-exit.c
deleted file mode 100644
index 8e88b16e6ded..000000000000
--- a/tools/perf/config/feature-checks/test-on-exit.c
+++ /dev/null
@@ -1,16 +0,0 @@
-#include <stdio.h>
-#include <stdlib.h>
-
-static void exit_fn(int status, void *__data)
-{
-	printf("exit status: %d, data: %d\n", status, *(int *)__data);
-}
-
-static int data = 123;
-
-int main(void)
-{
-	on_exit(exit_fn, &data);
-
-	return 321;
-}
-- 
1.7.9.2

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Stephane Eranian]

On Thu, Apr 24, 2014 at 3:27 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> Currently perf record doesn't propagate the exit status of a workload
> given by the command line.  But sometimes it'd useful if it's
> propagated so that a monitoring script can handle errors
> appropriately.
>
> To do that, it got rid of exit handlers and run/call them directly in
> the __cmd_record().  I don't see any reason why those are in a form of
> exit handlers in the first place.  Also it cleaned up the resouce
> management code in record__exit().
>
> With this change, perf record returns the child exit status in case of
> normal termination.  (Not sure what should be returned on abnormal
> cases though).
>
> Example run of Stephane's case:
>
>   $ perf record true && echo yes || echo no
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.013 MB perf.data (~589 samples) ]
>   yes
>
>   $ perf record false && echo yes || echo no
>   [ perf record: Woken up 1 times to write data ]
>   [ perf record: Captured and wrote 0.013 MB perf.data (~589 samples) ]
>   no
>
> And Jiri's case (error in parent):
>
>   $ perf record -m 10G true && echo yes || echo no
>   rounding mmap pages size to 17179869184 bytes (4194304 pages)
>   failed to mmap with 12 (Cannot allocate memory)
>   no
>
Works for me, now. Thanks.
Acked-by: Stephane Eranian <eranian@google.com>
> Reported-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-record.c |  121 ++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 70 deletions(-)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index eb524f91bffe..b1523d8a6191 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -152,26 +152,6 @@ static void sig_handler(int sig)
>         signr = sig;
>  }
>
> -static void record__sig_exit(int exit_status __maybe_unused, void *arg)
> -{
> -       struct record *rec = arg;
> -       int status;
> -
> -       if (rec->evlist->workload.pid > 0) {
> -               if (!child_finished)
> -                       kill(rec->evlist->workload.pid, SIGTERM);
> -
> -               wait(&status);
> -               if (WIFSIGNALED(status))
> -                       psignal(WTERMSIG(status), rec->progname);
> -       }
> -
> -       if (signr == -1 || signr == SIGUSR1)
> -               return;
> -
> -       signal(signr, SIG_DFL);
> -}
> -
>  static int record__open(struct record *rec)
>  {
>         char msg[512];
> @@ -243,27 +223,6 @@ static int process_buildids(struct record *rec)
>                                               size, &build_id__mark_dso_hit_ops);
>  }
>
> -static void record__exit(int status, void *arg)
> -{
> -       struct record *rec = arg;
> -       struct perf_data_file *file = &rec->file;
> -
> -       if (status != 0)
> -               return;
> -
> -       if (!file->is_pipe) {
> -               rec->session->header.data_size += rec->bytes_written;
> -
> -               if (!rec->no_buildid)
> -                       process_buildids(rec);
> -               perf_session__write_header(rec->session, rec->evlist,
> -                                          file->fd, true);
> -               perf_session__delete(rec->session);
> -               perf_evlist__delete(rec->evlist);
> -               symbol__exit();
> -       }
> -}
> -
>  static void perf_event__synthesize_guest_os(struct machine *machine, void *data)
>  {
>         int err;
> @@ -356,6 +315,7 @@ static void workload_exec_failed_signal(int signo, siginfo_t *info,
>  static int __cmd_record(struct record *rec, int argc, const char **argv)
>  {
>         int err;
> +       int status = 0;
>         unsigned long waking = 0;
>         const bool forks = argc > 0;
>         struct machine *machine;
> @@ -367,7 +327,6 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>
>         rec->progname = argv[0];
>
> -       on_exit(record__sig_exit, rec);
>         signal(SIGCHLD, sig_handler);
>         signal(SIGINT, sig_handler);
>         signal(SIGTERM, sig_handler);
> @@ -394,26 +353,21 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>
>         if (record__open(rec) != 0) {
>                 err = -1;
> -               goto out_delete_session;
> +               goto out_child;
>         }
>
>         if (!rec->evlist->nr_groups)
>                 perf_header__clear_feat(&session->header, HEADER_GROUP_DESC);
>
> -       /*
> -        * perf_session__delete(session) will be called at record__exit()
> -        */
> -       on_exit(record__exit, rec);
> -
>         if (file->is_pipe) {
>                 err = perf_header__write_pipe(file->fd);
>                 if (err < 0)
> -                       goto out_delete_session;
> +                       goto out_child;
>         } else {
>                 err = perf_session__write_header(session, rec->evlist,
>                                                  file->fd, false);
>                 if (err < 0)
> -                       goto out_delete_session;
> +                       goto out_child;
>         }
>
>         if (!rec->no_buildid
> @@ -421,7 +375,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                 pr_err("Couldn't generate buildids. "
>                        "Use --no-buildid to profile anyway.\n");
>                 err = -1;
> -               goto out_delete_session;
> +               goto out_child;
>         }
>
>         machine = &session->machines.host;
> @@ -431,7 +385,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                                                    process_synthesized_event);
>                 if (err < 0) {
>                         pr_err("Couldn't synthesize attrs.\n");
> -                       goto out_delete_session;
> +                       goto out_child;
>                 }
>
>                 if (have_tracepoints(&rec->evlist->entries)) {
> @@ -447,7 +401,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                                                                   process_synthesized_event);
>                         if (err <= 0) {
>                                 pr_err("Couldn't record tracing data.\n");
> -                               goto out_delete_session;
> +                               goto out_child;
>                         }
>                         rec->bytes_written += err;
>                 }
> @@ -475,7 +429,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>         err = __machine__synthesize_threads(machine, tool, &opts->target, rec->evlist->threads,
>                                             process_synthesized_event, opts->sample_address);
>         if (err != 0)
> -               goto out_delete_session;
> +               goto out_child;
>
>         if (rec->realtime_prio) {
>                 struct sched_param param;
> @@ -484,7 +438,7 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                 if (sched_setscheduler(0, SCHED_FIFO, &param)) {
>                         pr_err("Could not set realtime priority.\n");
>                         err = -1;
> -                       goto out_delete_session;
> +                       goto out_child;
>                 }
>         }
>
> @@ -512,13 +466,15 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>
>                 if (record__mmap_read_all(rec) < 0) {
>                         err = -1;
> -                       goto out_delete_session;
> +                       goto out_child;
>                 }
>
>                 if (hits == rec->samples) {
>                         if (done)
>                                 break;
>                         err = poll(rec->evlist->pollfd, rec->evlist->nr_fds, -1);
> +                       if (err < 0 && errno == EINTR)
> +                               err = 0;
>                         waking++;
>                 }
>
> @@ -538,28 +494,52 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                 const char *emsg = strerror_r(workload_exec_errno, msg, sizeof(msg));
>                 pr_err("Workload failed: %s\n", emsg);
>                 err = -1;
> -               goto out_delete_session;
> +               goto out_child;
>         }
>
> -       if (quiet || signr == SIGUSR1)
> -               return 0;
> +       if (!quiet) {
> +               fprintf(stderr, "[ perf record: Woken up %ld times to write data ]\n", waking);
>
> -       fprintf(stderr, "[ perf record: Woken up %ld times to write data ]\n", waking);
> +               /*
> +                * Approximate RIP event size: 24 bytes.
> +                */
> +               fprintf(stderr,
> +                       "[ perf record: Captured and wrote %.3f MB %s (~%" PRIu64 " samples) ]\n",
> +                       (double)rec->bytes_written / 1024.0 / 1024.0,
> +                       file->path,
> +                       rec->bytes_written / 24);
> +       }
>
> -       /*
> -        * Approximate RIP event size: 24 bytes.
> -        */
> -       fprintf(stderr,
> -               "[ perf record: Captured and wrote %.3f MB %s (~%" PRIu64 " samples) ]\n",
> -               (double)rec->bytes_written / 1024.0 / 1024.0,
> -               file->path,
> -               rec->bytes_written / 24);
> +out_child:
> +       if (forks) {
> +               int exit_status;
>
> -       return 0;
> +               if (!child_finished)
> +                       kill(rec->evlist->workload.pid, SIGTERM);
> +
> +               wait(&exit_status);
> +
> +               if (err < 0)
> +                       status = err;
> +               else if (WIFEXITED(exit_status))
> +                       status = WEXITSTATUS(exit_status);
> +               else if (WIFSIGNALED(status))
> +                       psignal(WTERMSIG(status), rec->progname);
> +       } else
> +               status = err;
> +
> +       if (!err && !file->is_pipe) {
> +               rec->session->header.data_size += rec->bytes_written;
> +
> +               if (!rec->no_buildid)
> +                       process_buildids(rec);
> +               perf_session__write_header(rec->session, rec->evlist,
> +                                          file->fd, true);
> +       }
>
>  out_delete_session:
>         perf_session__delete(session);
> -       return err;
> +       return status;
>  }
>
>  #define BRANCH_OPT(n, m) \
> @@ -988,6 +968,7 @@ int cmd_record(int argc, const char **argv, const char *prefix __maybe_unused)
>
>         err = __cmd_record(&record, argc, argv);
>  out_symbol_exit:
> +       perf_evlist__delete(rec->evlist);
>         symbol__exit();
>         return err;
>  }
> --
> 1.7.9.2
>

Re: [PATCH v3 3/3] perf tools: Get rid of on_exit() feature test

[Stephane Eranian]

On Thu, Apr 24, 2014 at 3:27 PM, Namhyung Kim <namhyung@kernel.org> wrote:
> The on_exit() function was only used in perf record but it's gone in
> previous patch.
>
Acked-by: Stephane Eranian <eranian@google.com>

> Cc: Bernhard Rosenkraenzer <Bernhard.Rosenkranzer@linaro.org>
> Cc: Irina Tirdea <irina.tirdea@intel.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-record.c                     |   31 -----------------------
>  tools/perf/config/Makefile                      |    8 ------
>  tools/perf/config/feature-checks/Makefile       |    4 ---
>  tools/perf/config/feature-checks/test-all.c     |    5 ----
>  tools/perf/config/feature-checks/test-on-exit.c |   16 ------------
>  5 files changed, 64 deletions(-)
>  delete mode 100644 tools/perf/config/feature-checks/test-on-exit.c
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index b1523d8a6191..1c127806fcb1 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -30,37 +30,6 @@
>  #include <sched.h>
>  #include <sys/mman.h>
>
> -#ifndef HAVE_ON_EXIT_SUPPORT
> -#ifndef ATEXIT_MAX
> -#define ATEXIT_MAX 32
> -#endif
> -static int __on_exit_count = 0;
> -typedef void (*on_exit_func_t) (int, void *);
> -static on_exit_func_t __on_exit_funcs[ATEXIT_MAX];
> -static void *__on_exit_args[ATEXIT_MAX];
> -static int __exitcode = 0;
> -static void __handle_on_exit_funcs(void);
> -static int on_exit(on_exit_func_t function, void *arg);
> -#define exit(x) (exit)(__exitcode = (x))
> -
> -static int on_exit(on_exit_func_t function, void *arg)
> -{
> -       if (__on_exit_count == ATEXIT_MAX)
> -               return -ENOMEM;
> -       else if (__on_exit_count == 0)
> -               atexit(__handle_on_exit_funcs);
> -       __on_exit_funcs[__on_exit_count] = function;
> -       __on_exit_args[__on_exit_count++] = arg;
> -       return 0;
> -}
> -
> -static void __handle_on_exit_funcs(void)
> -{
> -       int i;
> -       for (i = 0; i < __on_exit_count; i++)
> -               __on_exit_funcs[i] (__exitcode, __on_exit_args[i]);
> -}
> -#endif
>
>  struct record {
>         struct perf_tool        tool;
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index ee21fa95ebcf..7b7003d11983 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -156,7 +156,6 @@ CORE_FEATURE_TESTS =                        \
>         libpython-version               \
>         libslang                        \
>         libunwind                       \
> -       on-exit                         \
>         stackprotector-all              \
>         timerfd                         \
>         libdw-dwarf-unwind
> @@ -182,7 +181,6 @@ VF_FEATURE_TESTS =                  \
>         libelf-getphdrnum               \
>         libelf-mmap                     \
>         libpython-version               \
> -       on-exit                         \
>         stackprotector-all              \
>         timerfd                         \
>         libunwind-debug-frame           \
> @@ -541,12 +539,6 @@ ifneq ($(filter -lbfd,$(EXTLIBS)),)
>    CFLAGS += -DHAVE_LIBBFD_SUPPORT
>  endif
>
> -ifndef NO_ON_EXIT
> -  ifeq ($(feature-on-exit), 1)
> -    CFLAGS += -DHAVE_ON_EXIT_SUPPORT
> -  endif
> -endif
> -
>  ifndef NO_BACKTRACE
>    ifeq ($(feature-backtrace), 1)
>      CFLAGS += -DHAVE_BACKTRACE_SUPPORT
> diff --git a/tools/perf/config/feature-checks/Makefile b/tools/perf/config/feature-checks/Makefile
> index 2da103c53f89..64c84e5f0514 100644
> --- a/tools/perf/config/feature-checks/Makefile
> +++ b/tools/perf/config/feature-checks/Makefile
> @@ -24,7 +24,6 @@ FILES=                                        \
>         test-libslang.bin               \
>         test-libunwind.bin              \
>         test-libunwind-debug-frame.bin  \
> -       test-on-exit.bin                \
>         test-stackprotector-all.bin     \
>         test-timerfd.bin                \
>         test-libdw-dwarf-unwind.bin
> @@ -133,9 +132,6 @@ test-liberty-z.bin:
>  test-cplus-demangle.bin:
>         $(BUILD) -liberty
>
> -test-on-exit.bin:
> -       $(BUILD)
> -
>  test-backtrace.bin:
>         $(BUILD)
>
> diff --git a/tools/perf/config/feature-checks/test-all.c b/tools/perf/config/feature-checks/test-all.c
> index fc37eb3ca17b..fe5c1e5c952f 100644
> --- a/tools/perf/config/feature-checks/test-all.c
> +++ b/tools/perf/config/feature-checks/test-all.c
> @@ -69,10 +69,6 @@
>  # include "test-libbfd.c"
>  #undef main
>
> -#define main main_test_on_exit
> -# include "test-on-exit.c"
> -#undef main
> -
>  #define main main_test_backtrace
>  # include "test-backtrace.c"
>  #undef main
> @@ -110,7 +106,6 @@ int main(int argc, char *argv[])
>         main_test_gtk2(argc, argv);
>         main_test_gtk2_infobar(argc, argv);
>         main_test_libbfd();
> -       main_test_on_exit();
>         main_test_backtrace();
>         main_test_libnuma();
>         main_test_timerfd();
> diff --git a/tools/perf/config/feature-checks/test-on-exit.c b/tools/perf/config/feature-checks/test-on-exit.c
> deleted file mode 100644
> index 8e88b16e6ded..000000000000
> --- a/tools/perf/config/feature-checks/test-on-exit.c
> +++ /dev/null
> @@ -1,16 +0,0 @@
> -#include <stdio.h>
> -#include <stdlib.h>
> -
> -static void exit_fn(int status, void *__data)
> -{
> -       printf("exit status: %d, data: %d\n", status, *(int *)__data);
> -}
> -
> -static int data = 123;
> -
> -int main(void)
> -{
> -       on_exit(exit_fn, &data);
> -
> -       return 321;
> -}
> --
> 1.7.9.2
>

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Jiri Olsa]

On Thu, Apr 24, 2014 at 10:27:33PM +0900, Namhyung Kim wrote:

SNIP

> Reported-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> ---
>  tools/perf/builtin-record.c |  121 ++++++++++++++++++-------------------------
>  1 file changed, 51 insertions(+), 70 deletions(-)
> 
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index eb524f91bffe..b1523d8a6191 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -152,26 +152,6 @@ static void sig_handler(int sig)
>  	signr = sig;

looks like there's no use for 'signr' now.. could be removed

>  }
>  
> -static void record__sig_exit(int exit_status __maybe_unused, void *arg)
> -{
> -	struct record *rec = arg;
> -	int status;
> -
> -	if (rec->evlist->workload.pid > 0) {
> -		if (!child_finished)
> -			kill(rec->evlist->workload.pid, SIGTERM);
> -
> -		wait(&status);
> -		if (WIFSIGNALED(status))
> -			psignal(WTERMSIG(status), rec->progname);
> -	}
> -
> -	if (signr == -1 || signr == SIGUSR1)
> -		return;
> -
> -	signal(signr, SIG_DFL);

I was wondering what was this one for and found:

  perf_counter tools: Propagate signals properly
  commit f7b7c26e01e51fe46097e11f179dc71ce7950084
  Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
  Date:   Wed Jun 10 15:55:59 2009 +0200

but I dont think we need to do that

thanks,
jirka

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Jiri Olsa]

On Thu, Apr 24, 2014 at 10:27:33PM +0900, Namhyung Kim wrote:

SNIP

> -		rec->bytes_written / 24);
> +out_child:
> +	if (forks) {
> +		int exit_status;
>  
> -	return 0;
> +		if (!child_finished)
> +			kill(rec->evlist->workload.pid, SIGTERM);

also while at it.. do we want to force SIGKILL in case we dont
get any response for SIGTERM? so we dont get record hanging like
for following program:

---
#include <signal.h>

static void sig_handler(int sig)
{
}

int main(int argc, char **argv)
{
        signal(SIGINT, sig_handler);
        signal(SIGTERM, sig_handler);

        while (1) {}

        return 0;
}
---

the change would go into separate patch of course,
something like in patch below

thanks,
jirka


---
diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 526edf5..a973ba5 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -479,9 +479,20 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 out_child:
 	if (forks) {
 		int exit_status;
+		int zleep = 0;
+
+		while (!child_finished) {
+			if (!zleep)
+				kill(rec->evlist->workload.pid, SIGTERM);
+			if (zleep == 2000) {
+				pr_info("Child killed by SIGKILL.\n");
+				kill(rec->evlist->workload.pid, SIGKILL);
+				break;
+			}
 
-		if (!child_finished)
-			kill(rec->evlist->workload.pid, SIGTERM);
+			usleep(1000);
+			zleep++;
+		}
 
 		wait(&exit_status);
 

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Peter Zijlstra]

On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> 
>   perf_counter tools: Propagate signals properly
>   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
>   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
>   Date:   Wed Jun 10 15:55:59 2009 +0200
> 
> but I dont think we need to do that

But but but, then you're re-introducing that fail again? That no good.

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Peter Zijlstra]

On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> > 
> >   perf_counter tools: Propagate signals properly
> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> >   Date:   Wed Jun 10 15:55:59 2009 +0200
> > 
> > but I dont think we need to do that
> 
> But but but, then you're re-introducing that fail again? That no good.

The thing is, just the return value is not sufficient to determine if
the child was interrupted.

See WAIT(2), things like WIFSIGNALED() will not work when you just
propagate the return value, you need to terminate the task with a
signal to propagate this.

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Jiri Olsa]

On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> > 
> >   perf_counter tools: Propagate signals properly
> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> >   Date:   Wed Jun 10 15:55:59 2009 +0200
> > 
> > but I dont think we need to do that
> 
> But but but, then you're re-introducing that fail again? That no good.

well, I was trying the testcase you mentioned in the changelog
and it seemed to work for me.. ;-) I guess I was lucky to hit
the bash time window..

  while :; do perf stat ./foo ; done

so how does this work? bash will kill the loop if perf's wait
status is WIFSIGNALED?

thanks,
jirka

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Jiri Olsa]

On Tue, Apr 29, 2014 at 01:33:04PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> > > 
> > >   perf_counter tools: Propagate signals properly
> > >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> > >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> > >   Date:   Wed Jun 10 15:55:59 2009 +0200
> > > 
> > > but I dont think we need to do that
> > 
> > But but but, then you're re-introducing that fail again? That no good.
> 
> The thing is, just the return value is not sufficient to determine if
> the child was interrupted.
> 
> See WAIT(2), things like WIFSIGNALED() will not work when you just
> propagate the return value, you need to terminate the task with a
> signal to propagate this.
> 

cool, I just asked this in another email ;-) thanks

jirka

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Peter Zijlstra]

On Thu, Apr 24, 2014 at 10:27:33PM +0900, Namhyung Kim wrote:
>  static void record__sig_exit(int exit_status __maybe_unused, void *arg)
>  {
> -	struct record *rec = arg;
> -	int status;
> -
> -	if (rec->evlist->workload.pid > 0) {
> -		if (!child_finished)
> -			kill(rec->evlist->workload.pid, SIGTERM);
> -
> -		wait(&status);
> -		if (WIFSIGNALED(status))
> -			psignal(WTERMSIG(status), rec->progname);
> -	}
> -
> -	if (signr == -1 || signr == SIGUSR1)
> +	if (signr == -1)
>  		return;
>  
>  	signal(signr, SIG_DFL);
>  }


> +out_child:
> +	if (forks) {
> +		int exit_status;
>  
> -	return 0;
> +		if (!child_finished)
> +			kill(rec->evlist->workload.pid, SIGTERM);
> +
> +		wait(&exit_status);
> +
> +		if (err < 0)
> +			status = err;
> +		else if (WIFEXITED(exit_status))
> +			status = WEXITSTATUS(exit_status);
> +		else if (WIFSIGNALED(status))
		{
		signr = WTERMSIG(status);
> +			psignal(WTERMSIG(status), rec->progname);
		}
> +	} else
> +		status = err;
> +
> +	if (!err && !file->is_pipe) {
> +		rec->session->header.data_size += rec->bytes_written;
> +
> +		if (!rec->no_buildid)
> +			process_buildids(rec);
> +		perf_session__write_header(rec->session, rec->evlist,
> +					   file->fd, true);
> +	}

Should do I suppose; not sure why the psignal() muck is in there, but it
don't hurt I suppose.

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Namhyung Kim]

Hi Jiri and Peter,

On Tue, 29 Apr 2014 13:37:47 +0200, Jiri Olsa wrote:
> On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
>> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
>> > 
>> >   perf_counter tools: Propagate signals properly
>> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
>> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> >   Date:   Wed Jun 10 15:55:59 2009 +0200
>> > 
>> > but I dont think we need to do that
>> 
>> But but but, then you're re-introducing that fail again? That no good.

FYI, it's already gone with 804f7ac78803 ("perf record: handle death by
SIGTERM").

>
> well, I was trying the testcase you mentioned in the changelog
> and it seemed to work for me.. ;-) I guess I was lucky to hit
> the bash time window..
>
>   while :; do perf stat ./foo ; done
>
> so how does this work? bash will kill the loop if perf's wait
> status is WIFSIGNALED?

I'm not sure but isn't it *bash* to catch signal and terminate the
loop?  It seems the wait status of child has no business with the loop
termination.  Am I missing something?

  $ cat suicide.c
  #include <signal.h>
  
  int main(void)
  {
    raise(SIGTERM);
    return 0;
  }
  
  $ gcc -o suicide suicide.c
  
  $ while :; do ./suicide; done
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  Terminated
  ...


Thanks,
Namhyung

[tip:perf/core] perf tools: Handle EINTR error for readn/writen

[tip-bot for Namhyung Kim]

Commit-ID:  e148c76083dc06ce618d768c0bee0a0edda96a54
Gitweb:     http://git.kernel.org/tip/e148c76083dc06ce618d768c0bee0a0edda96a54
Author:     Namhyung Kim <namhyung@kernel.org>
AuthorDate: Thu, 24 Apr 2014 22:27:32 +0900
Committer:  Jiri Olsa <jolsa@kernel.org>
CommitDate: Tue, 29 Apr 2014 14:26:30 +0200

perf tools: Handle EINTR error for readn/writen

Those readn/writen functions are to ensure read/write does I/O for
a given size exactly.  But ion() - its implementation - does not
handle in case it returns prematurely due to a signal.  As it's not
an error itself so just retry the operation.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/1398346054-3322-1-git-send-email-namhyung@kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
 tools/perf/util/util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
index 9f66549..7fff6be 100644
--- a/tools/perf/util/util.c
+++ b/tools/perf/util/util.c
@@ -166,6 +166,8 @@ static ssize_t ion(bool is_read, int fd, void *buf, size_t n)
 		ssize_t ret = is_read ? read(fd, buf, left) :
 					write(fd, buf, left);
 
+		if (ret < 0 && errno == EINTR)
+			continue;
 		if (ret <= 0)
 			return ret;
 

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Jiri Olsa]

On Wed, Apr 30, 2014 at 09:24:08AM +0900, Namhyung Kim wrote:
> Hi Jiri and Peter,
> 
> On Tue, 29 Apr 2014 13:37:47 +0200, Jiri Olsa wrote:
> > On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
> >> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> >> > 
> >> >   perf_counter tools: Propagate signals properly
> >> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> >> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> >> >   Date:   Wed Jun 10 15:55:59 2009 +0200
> >> > 
> >> > but I dont think we need to do that
> >> 
> >> But but but, then you're re-introducing that fail again? That no good.
> 
> FYI, it's already gone with 804f7ac78803 ("perf record: handle death by
> SIGTERM").

oops, sry for late reply.. I just saw v4

I see, so that kill got deleted, only 'signal(signr, SIG_DFL);' stayed

I think we could take this patch, since it's not introducing regression
(if there's any) in this regard and figure out the bash stuff below later

> 
> >
> > well, I was trying the testcase you mentioned in the changelog
> > and it seemed to work for me.. ;-) I guess I was lucky to hit
> > the bash time window..
> >
> >   while :; do perf stat ./foo ; done
> >
> > so how does this work? bash will kill the loop if perf's wait
> > status is WIFSIGNALED?
> 
> I'm not sure but isn't it *bash* to catch signal and terminate the
> loop?  It seems the wait status of child has no business with the loop
> termination.  Am I missing something?

peterz, any comment? before we start digging in bash.. ;-)

thanks,
jirka

> 
>   $ cat suicide.c
>   #include <signal.h>
>   
>   int main(void)
>   {
>     raise(SIGTERM);
>     return 0;
>   }
>   
>   $ gcc -o suicide suicide.c
>   
>   $ while :; do ./suicide; done
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   Terminated
>   ...
> 
> 
> Thanks,
> Namhyung

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On Wed, Apr 30, 2014 at 09:24:08AM +0900, Namhyung Kim wrote:
> Hi Jiri and Peter,
> 
> On Tue, 29 Apr 2014 13:37:47 +0200, Jiri Olsa wrote:
> > On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
> >> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
> >> > 
> >> >   perf_counter tools: Propagate signals properly
> >> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> >> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
> >> >   Date:   Wed Jun 10 15:55:59 2009 +0200
> >> > 
> >> > but I dont think we need to do that
> >> 
> >> But but but, then you're re-introducing that fail again? That no good.
> 
> FYI, it's already gone with 804f7ac78803 ("perf record: handle death by
> SIGTERM").
> 
> >
> > well, I was trying the testcase you mentioned in the changelog
> > and it seemed to work for me.. ;-) I guess I was lucky to hit
> > the bash time window..
> >
> >   while :; do perf stat ./foo ; done
> >
> > so how does this work? bash will kill the loop if perf's wait
> > status is WIFSIGNALED?
> 
> I'm not sure but isn't it *bash* to catch signal and terminate the
> loop?  It seems the wait status of child has no business with the loop
> termination.  Am I missing something?
> 
>   $ cat suicide.c
>   #include <signal.h>
>   
>   int main(void)
>   {
>     raise(SIGTERM);
>     return 0;
>   }
>   

SIGTERM isn't the problem. SIGINT is. Typically when you run:

while :; do perf stat ./foo ; done

Its foo that is running, so when you press ^C, you'll SIGINT foo. foo
will then exit, perf stat will notice the exit, exit itself and because
the loop doesn't look at the return value of perf stat, simply
continues.

What I want, and fixed back then, is that if you press ^C foo
terminates, perf stat/record/etc. will finish, but then terminate with
the same signal. In that case bash finally sees the SIGINT and will in
fact terminate the loop.

try:

$ while :; do ./foo /bin/sleep 5 ; done

and try and break out using ^C

---
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <signal.h>
#include <sys/types.h>
#include <sys/wait.h>

int main(int argc, char *argv[], char *envp[])
{
	pid_t pid = fork();
	if (!pid) /* child */ {
		execve(argv[1], argv+1, envp);
		perror("execve");
		return;
	}

	signal(SIGINT, SIG_IGN);
	waitpid(pid, NULL, 0);
	return 0;
}

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Stephane Eranian]

On Wed, May 7, 2014 at 5:04 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Apr 30, 2014 at 09:24:08AM +0900, Namhyung Kim wrote:
>> Hi Jiri and Peter,
>>
>> On Tue, 29 Apr 2014 13:37:47 +0200, Jiri Olsa wrote:
>> > On Tue, Apr 29, 2014 at 01:19:39PM +0200, Peter Zijlstra wrote:
>> >> On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
>> >> >
>> >> >   perf_counter tools: Propagate signals properly
>> >> >   commit f7b7c26e01e51fe46097e11f179dc71ce7950084
>> >> >   Author: Peter Zijlstra <a.p.zijlstra@chello.nl>
>> >> >   Date:   Wed Jun 10 15:55:59 2009 +0200
>> >> >
>> >> > but I dont think we need to do that
>> >>
>> >> But but but, then you're re-introducing that fail again? That no good.
>>
>> FYI, it's already gone with 804f7ac78803 ("perf record: handle death by
>> SIGTERM").
>>
>> >
>> > well, I was trying the testcase you mentioned in the changelog
>> > and it seemed to work for me.. ;-) I guess I was lucky to hit
>> > the bash time window..
>> >
>> >   while :; do perf stat ./foo ; done
>> >
>> > so how does this work? bash will kill the loop if perf's wait
>> > status is WIFSIGNALED?
>>
>> I'm not sure but isn't it *bash* to catch signal and terminate the
>> loop?  It seems the wait status of child has no business with the loop
>> termination.  Am I missing something?
>>
>>   $ cat suicide.c
>>   #include <signal.h>
>>
>>   int main(void)
>>   {
>>     raise(SIGTERM);
>>     return 0;
>>   }
>>
>
> SIGTERM isn't the problem. SIGINT is. Typically when you run:
>
> while :; do perf stat ./foo ; done
>
> Its foo that is running, so when you press ^C, you'll SIGINT foo. foo
> will then exit, perf stat will notice the exit, exit itself and because
> the loop doesn't look at the return value of perf stat, simply
> continues.
>
> What I want, and fixed back then, is that if you press ^C foo
> terminates, perf stat/record/etc. will finish, but then terminate with
> the same signal. In that case bash finally sees the SIGINT and will in
> fact terminate the loop.
>
> try:
>
> $ while :; do ./foo /bin/sleep 5 ; done
>
> and try and break out using ^C
>
What I usually do here is hit ^Z, then kill the job.
But I agree it would be nicer to handle this case automatically.

> ---
> #include <stdio.h>
> #include <stdlib.h>
> #include <unistd.h>
> #include <signal.h>
> #include <sys/types.h>
> #include <sys/wait.h>
>
> int main(int argc, char *argv[], char *envp[])
> {
>         pid_t pid = fork();
>         if (!pid) /* child */ {
>                 execve(argv[1], argv+1, envp);
>                 perror("execve");
>                 return;
>         }
>
>         signal(SIGINT, SIG_IGN);
>         waitpid(pid, NULL, 0);
>         return 0;
> }

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Peter Zijlstra]

[-- Attachment #1: Type: text/plain, Size: 644 bytes --]

On Wed, May 07, 2014 at 07:19:14PM +0200, Stephane Eranian wrote:
> > $ while :; do ./foo /bin/sleep 5 ; done
> >
> > and try and break out using ^C
> >
> What I usually do here is hit ^Z, then kill the job.
> But I agree it would be nicer to handle this case automatically.

So that used to work. And note that if you take out the signal(SIGINT,
SIG_IGN) from the proglet, it will actually propagate the SIGINT and
work as expected.

So its only because perf handles SIGINT -- to be able to finish the
stat/data record, that it doesn't. Which is why I propagated the signal
the child got killed with.

Now apparently someone broke that again.

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/3] perf record: Propagate exit status of a command line workload

[Namhyung Kim]

Hi Peter,

On Wed, 7 May 2014 19:35:32 +0200, Peter Zijlstra wrote:
> On Wed, May 07, 2014 at 07:19:14PM +0200, Stephane Eranian wrote:
>> > $ while :; do ./foo /bin/sleep 5 ; done
>> >
>> > and try and break out using ^C
>> >
>> What I usually do here is hit ^Z, then kill the job.
>> But I agree it would be nicer to handle this case automatically.
>
> So that used to work. And note that if you take out the signal(SIGINT,
> SIG_IGN) from the proglet, it will actually propagate the SIGINT and
> work as expected.
>
> So its only because perf handles SIGINT -- to be able to finish the
> stat/data record, that it doesn't. Which is why I propagated the signal
> the child got killed with.

Ah, okay.  But just propagating saved signal looks not enough since it's
overwritten by SIGCHLD as child exited.  I'll resend v5 soon.

Thanks,
Namhyung