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

2014-05-07 Thread Peter Zijlstra
On Wed, May 07, 2014 at 10:13:24AM +0900, Namhyung Kim wrote:
> - on_exit(record__sig_exit, rec);

NAK!


pgpZfXHB5LSj6.pgp
Description: PGP signature


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

2014-05-06 Thread 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 resource
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 
Acked-by: Stephane Eranian 
Signed-off-by: Namhyung Kim 
---
 tools/perf/builtin-record.c | 127 ++--
 1 file changed, 53 insertions(+), 74 deletions(-)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 8ce62ef7f6c3..e4cb53397b83 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -140,7 +140,6 @@ out:
 }
 
 static volatile int done = 0;
-static volatile int signr = -1;
 static volatile int child_finished = 0;
 
 static void sig_handler(int sig)
@@ -149,27 +148,6 @@ static void sig_handler(int sig)
child_finished = 1;
 
done = 1;
-   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)
@@ -243,27 +221,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;
@@ -344,18 +301,19 @@ static volatile int workload_exec_errno;
  * if the fork fails, since we asked by setting its
  * want_signal to true.
  */
-static void workload_exec_failed_signal(int signo, siginfo_t *info,
+static void workload_exec_failed_signal(int signo __maybe_unused,
+   siginfo_t *info,
void *ucontext __maybe_unused)
 {
workload_exec_errno = info->si_value.sival_int;
done = 1;
-   signr = signo;
child_finished = 1;
 }
 
 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 +325,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 +351,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 = p