* [PATCH] perf stat: Do not show stats if workload fails
@ 2013-12-20 5:52 David Ahern
2013-12-20 7:57 ` Ingo Molnar
0 siblings, 1 reply; 12+ messages in thread
From: David Ahern @ 2013-12-20 5:52 UTC (permalink / raw)
To: acme, linux-kernel; +Cc: David Ahern, Ingo Molnar, Stephane Eranian
Currently perf-stat attempts to show counter stats even if the workload
is bogus:
$ perf stat -- foo
foo: No such file or directory
Performance counter stats for 'foo':
<not counted> task-clock
<not counted> context-switches
<not counted> cpu-migrations
<not counted> page-faults
<not counted> cycles
<not counted> stalled-cycles-frontend
<not counted> stalled-cycles-backend
<not counted> instructions
<not counted> branches
<not counted> branch-misses
0.009769943 seconds time elapsed
It is impossible to differentiate all the failure modes, but it seems
reasonable that if the workload handling fails, perf-stat should not try
to print stats.
With this change:
$ perf stat -v -- foo
Failed to start workload
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Stephane Eranian <eranian@google.com>
---
tools/perf/builtin-stat.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index dab98b50c9fe..d6e6a0b031d9 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -586,7 +586,11 @@ static int __run_perf_stat(int argc, const char **argv)
clock_gettime(CLOCK_MONOTONIC, &ref_time);
if (forks) {
- perf_evlist__start_workload(evsel_list);
+ if (perf_evlist__start_workload(evsel_list) != 0) {
+ pr_err("Failed to start workload\n");
+ return -1;
+ }
+
handle_initial_delay();
if (interval) {
@@ -1793,7 +1797,10 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused)
run_idx + 1);
status = run_perf_stat(argc, argv);
- if (forever && status != -1) {
+ if (status < 0)
+ break;
+
+ if (forever) {
print_stat(argc, argv);
perf_stat__reset_stats(evsel_list);
}
--
1.8.3.4 (Apple Git-47)
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-20 5:52 [PATCH] perf stat: Do not show stats if workload fails David Ahern @ 2013-12-20 7:57 ` Ingo Molnar 2013-12-23 19:37 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 12+ messages in thread From: Ingo Molnar @ 2013-12-20 7:57 UTC (permalink / raw) To: David Ahern; +Cc: acme, linux-kernel, Stephane Eranian * David Ahern <dsahern@gmail.com> wrote: > Currently perf-stat attempts to show counter stats even if the workload > is bogus: > > $ perf stat -- foo > foo: No such file or directory > > Performance counter stats for 'foo': > > <not counted> task-clock > <not counted> context-switches > <not counted> cpu-migrations > <not counted> page-faults > <not counted> cycles > <not counted> stalled-cycles-frontend > <not counted> stalled-cycles-backend > <not counted> instructions > <not counted> branches > <not counted> branch-misses > > 0.009769943 seconds time elapsed > > It is impossible to differentiate all the failure modes, but it seems > reasonable that if the workload handling fails, perf-stat should not try > to print stats. > > With this change: > > $ perf stat -v -- foo > Failed to start workload > > Signed-off-by: David Ahern <dsahern@gmail.com> > Cc: Ingo Molnar <mingo@kernel.org> > Cc: Stephane Eranian <eranian@google.com> Nice! Acked-by: Ingo Molnar <mingo@kernel.org> Thanks, Ingo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-20 7:57 ` Ingo Molnar @ 2013-12-23 19:37 ` Arnaldo Carvalho de Melo 2013-12-24 0:29 ` David Ahern 0 siblings, 1 reply; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-23 19:37 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Fri, Dec 20, 2013 at 08:57:59AM +0100, Ingo Molnar escreveu: > * David Ahern <dsahern@gmail.com> wrote: > > > Currently perf-stat attempts to show counter stats even if the workload > > is bogus: > > > > $ perf stat -- foo > > foo: No such file or directory > > > > Performance counter stats for 'foo': > > > > <not counted> task-clock > > <not counted> context-switches > > <not counted> cpu-migrations > > <not counted> page-faults > > <not counted> cycles > > <not counted> stalled-cycles-frontend > > <not counted> stalled-cycles-backend > > <not counted> instructions > > <not counted> branches > > <not counted> branch-misses > > > > 0.009769943 seconds time elapsed > > > > It is impossible to differentiate all the failure modes, but it seems > > reasonable that if the workload handling fails, perf-stat should not try > > to print stats. > > > > With this change: > > > > $ perf stat -v -- foo > > Failed to start workload > > > > Signed-off-by: David Ahern <dsahern@gmail.com> > > Cc: Ingo Molnar <mingo@kernel.org> > > Cc: Stephane Eranian <eranian@google.com> > > Nice! > > Acked-by: Ingo Molnar <mingo@kernel.org> Good intent, but... [acme@ssdandy linux]$ perf stat usleep 1 Failed to start workload [acme@ssdandy linux]$ Your patch does: if (perf_evlist__start_workload(evsel_list) != 0) But: int perf_evlist__start_workload(struct perf_evlist *evlist) { if (evlist->workload.cork_fd > 0) { char bf = 0; int ret; /* * Remove the cork, let it rip! */ ret = write(evlist->workload.cork_fd, &bf, 1); if (ret < 0) perror("enable to write to pipe"); close(evlist->workload.cork_fd); return ret; } return 0; } Ret there is 1, so we need to change it to: return ret != 1 ? -1 : 0; Right? - Arnaldo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-23 19:37 ` Arnaldo Carvalho de Melo @ 2013-12-24 0:29 ` David Ahern 2013-12-24 12:53 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 12+ messages in thread From: David Ahern @ 2013-12-24 0:29 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian On 12/23/13, 2:37 PM, Arnaldo Carvalho de Melo wrote: > int perf_evlist__start_workload(struct perf_evlist *evlist) > { > if (evlist->workload.cork_fd > 0) { > char bf = 0; > int ret; > /* > * Remove the cork, let it rip! > */ > ret = write(evlist->workload.cork_fd, &bf, 1); > if (ret < 0) > perror("enable to write to pipe"); > > close(evlist->workload.cork_fd); > return ret; > } > > return 0; > } > > Ret there is 1, so we need to change it to: > > return ret != 1 ? -1 : 0; > > Right? Yes, nice catch. David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-24 0:29 ` David Ahern @ 2013-12-24 12:53 ` Arnaldo Carvalho de Melo 2013-12-24 13:30 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-24 12:53 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Mon, Dec 23, 2013 at 07:29:54PM -0500, David Ahern escreveu: > On 12/23/13, 2:37 PM, Arnaldo Carvalho de Melo wrote: > >int perf_evlist__start_workload(struct perf_evlist *evlist) > >{ > > if (evlist->workload.cork_fd > 0) { > > char bf = 0; > > /* Remove the cork, let it rip! */ > > int ret = write(evlist->workload.cork_fd, &bf, 1); > > if (ret < 0) > > perror("enable to write to pipe"); > > > > close(evlist->workload.cork_fd); > > return ret; > > } > > return 0; > >} > >Ret there is 1, so we need to change it to: > > return ret != 1 ? -1 : 0; > >Right? > Yes, nice catch. But this doesn't really matters, if one uses the more usual: if (function() < 0) pattern for error checking, there would be no problem, and we would only be checking that we told perf_evlist__prepare_workload()'s fork branch to call execvp, not to check what happens then, i.e. if it finds the specified workload in the system's PATH, if there are no permission of file format problems, etc. The thing to check is perf_evlist__{prepare,start}_workload notification errors using SIGUSR1, that we need to check for in the caller, and emit the message, no? - Arnaldo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-24 12:53 ` Arnaldo Carvalho de Melo @ 2013-12-24 13:30 ` Arnaldo Carvalho de Melo 2013-12-26 14:11 ` David Ahern 0 siblings, 1 reply; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-24 13:30 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Tue, Dec 24, 2013 at 09:53:42AM -0300, Arnaldo Carvalho de Melo escreveu: > The thing to check is perf_evlist__{prepare,start}_workload notification > errors using SIGUSR1, that we need to check for in the caller, and emit > the message, no? Something like this: 1. We tell perf_evlist__prepare_workload that we want a signal if execvp fails, it will be a SIGUSR1 2. We catch that signal in 'stat' and check that we got a signal, only problem so far with this signal maze is that we're getting a SIGCHLD while I was expecting a SIGUSR1... I.e. the "if (signr != -1) test really should be if (signr == SIGUSR1), but I'm getting a SIGCHLD there and the elves are tugging me away... - Arnaldo diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index dab98b50c9fe..d2350fef8cde 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -144,6 +144,7 @@ static struct timespec ref_time; static struct cpu_map *aggr_map; static int (*aggr_get_id)(struct cpu_map *m, int cpu); +static volatile int signr = -1; static volatile int done = 0; struct perf_stat { @@ -531,7 +532,7 @@ static int __run_perf_stat(int argc, const char **argv) if (forks) { if (perf_evlist__prepare_workload(evsel_list, &target, argv, - false, false) < 0) { + false, true) < 0) { perror("failed to prepare workload"); return -1; } @@ -598,6 +599,8 @@ static int __run_perf_stat(int argc, const char **argv) wait(&status); if (WIFSIGNALED(status)) psignal(WTERMSIG(status), argv[0]); + else if (signr != -1) + return -1; } else { handle_initial_delay(); while (!done) { @@ -1335,8 +1338,6 @@ static void print_stat(int argc, const char **argv) } } -static volatile int signr = -1; - static void skip_signal(int signo) { if ((child_pid == -1) || interval) @@ -1785,6 +1786,7 @@ int cmd_stat(int argc, const char **argv, const char *prefix __maybe_unused) signal(SIGCHLD, skip_signal); signal(SIGALRM, skip_signal); signal(SIGABRT, skip_signal); + signal(SIGUSR1, skip_signal); status = 0; for (run_idx = 0; forever || run_idx < run_count; run_idx++) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-24 13:30 ` Arnaldo Carvalho de Melo @ 2013-12-26 14:11 ` David Ahern 2013-12-26 14:15 ` Arnaldo Carvalho de Melo 2014-01-02 14:44 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 12+ messages in thread From: David Ahern @ 2013-12-26 14:11 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian On 12/24/13, 8:30 AM, Arnaldo Carvalho de Melo wrote: > Em Tue, Dec 24, 2013 at 09:53:42AM -0300, Arnaldo Carvalho de Melo escreveu: >> >The thing to check is perf_evlist__{prepare,start}_workload notification >> >errors using SIGUSR1, that we need to check for in the caller, and emit >> >the message, no? > Something like this: > > 1. We tell perf_evlist__prepare_workload that we want a signal if execvp > fails, it will be a SIGUSR1 > > 2. We catch that signal in 'stat' and check that we got a signal, only > problem so far with this signal maze is that we're getting a SIGCHLD > while I was expecting a SIGUSR1... I.e. the "if (signr != -1) test > really should be if (signr == SIGUSR1), but I'm getting a SIGCHLD there > and the elves are tugging me away... Did the elves release you? There are all kinds of failure paths with the workload functions. In the end I was focusing on perf-stat actually checking the rc on the start_workload function. If it fails, then write() failed and something happened to the workload process. In that case don't show the counters. Handling the other error paths with appropriate messages will take additional effort - as you are finding. ;-) David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-26 14:11 ` David Ahern @ 2013-12-26 14:15 ` Arnaldo Carvalho de Melo 2013-12-26 14:18 ` David Ahern 2014-01-02 14:44 ` Arnaldo Carvalho de Melo 1 sibling, 1 reply; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-26 14:15 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Thu, Dec 26, 2013 at 09:11:22AM -0500, David Ahern escreveu: > On 12/24/13, 8:30 AM, Arnaldo Carvalho de Melo wrote: > >Em Tue, Dec 24, 2013 at 09:53:42AM -0300, Arnaldo Carvalho de Melo escreveu: > >>>The thing to check is perf_evlist__{prepare,start}_workload notification > >>>errors using SIGUSR1, that we need to check for in the caller, and emit > >>>the message, no? > >Something like this: > > > >1. We tell perf_evlist__prepare_workload that we want a signal if execvp > >fails, it will be a SIGUSR1 > > > >2. We catch that signal in 'stat' and check that we got a signal, only > >problem so far with this signal maze is that we're getting a SIGCHLD > >while I was expecting a SIGUSR1... I.e. the "if (signr != -1) test > >really should be if (signr == SIGUSR1), but I'm getting a SIGCHLD there > >and the elves are tugging me away... > > Did the elves release you? > > There are all kinds of failure paths with the workload functions. In > the end I was focusing on perf-stat actually checking the rc on the > start_workload function. If it fails, then write() failed and > something happened to the workload process. In that case don't show > the counters. > > Handling the other error paths with appropriate messages will take > additional effort - as you are finding. ;-) Right, but I can't apply that patch, as it makes 'perf stat whatever-workload' to fail, as I realized when doing a demo to someone interested in using perf ;-\ So for now I'm not applying that one. Ah, at this point elves are everywhere, dammit! ;-) - Arnaldo ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-26 14:15 ` Arnaldo Carvalho de Melo @ 2013-12-26 14:18 ` David Ahern 2013-12-26 14:26 ` Arnaldo Carvalho de Melo 2013-12-26 14:55 ` Arnaldo Carvalho de Melo 0 siblings, 2 replies; 12+ messages in thread From: David Ahern @ 2013-12-26 14:18 UTC (permalink / raw) To: Arnaldo Carvalho de Melo; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian On 12/26/13, 9:15 AM, Arnaldo Carvalho de Melo wrote: > Right, but I can't apply that patch, as it makes 'perf stat > whatever-workload' to fail, as I realized when doing a demo to someone > interested in using perf ;-\ > > So for now I'm not applying that one. right, so you want one with < 0 check or wait for something else? I was not expecting to find it in your perf/core branch, yet there it is. > > Ah, at this point elves are everywhere, dammit! ;-) An elf put it there? David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-26 14:18 ` David Ahern @ 2013-12-26 14:26 ` Arnaldo Carvalho de Melo 2013-12-26 14:55 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-26 14:26 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Thu, Dec 26, 2013 at 09:18:17AM -0500, David Ahern escreveu: > On 12/26/13, 9:15 AM, Arnaldo Carvalho de Melo wrote: > >Right, but I can't apply that patch, as it makes 'perf stat > >whatever-workload' to fail, as I realized when doing a demo to someone > >interested in using perf ;-\ > > > >So for now I'm not applying that one. > > right, so you want one with < 0 check or wait for something else? I > was not expecting to find it in your perf/core branch, yet there it > is. I'll remove it from there, but try it, IIRC there will be some other problem :-\ I'd have to reread the messages I sent, but from what I recall the return from perf_evlist__start_workload() will _always_ be valid, i.e. what you're testing there is just if the parent wrote a byte to a pipe to signal the waiting child to call exec, and that _will_ work, the exec()? perhaps not, you'd have to setup the signal error reporting mechanism, etc. Perhaps this should be somehow done by perf_evlist__start_workload, so that what it reports is the result of the exec in the child, and not merely if it managed to tell it to try to exec... - Arnaldo > > > >Ah, at this point elves are everywhere, dammit! ;-) > > An elf put it there? Right, dwarves may be involved, didn't had the time to figure that out... > David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-26 14:18 ` David Ahern 2013-12-26 14:26 ` Arnaldo Carvalho de Melo @ 2013-12-26 14:55 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2013-12-26 14:55 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian Em Thu, Dec 26, 2013 at 09:18:17AM -0500, David Ahern escreveu: > On 12/26/13, 9:15 AM, Arnaldo Carvalho de Melo wrote: > >Right, but I can't apply that patch, as it makes 'perf stat > >whatever-workload' to fail, as I realized when doing a demo to someone > >interested in using perf ;-\ > > > >So for now I'm not applying that one. > > right, so you want one with < 0 check or wait for something else? I > was not expecting to find it in your perf/core branch, yet there it > is. Shouldn't be anymore, - Arnaldo > > > >Ah, at this point elves are everywhere, dammit! ;-) > > An elf put it there? > > David ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] perf stat: Do not show stats if workload fails 2013-12-26 14:11 ` David Ahern 2013-12-26 14:15 ` Arnaldo Carvalho de Melo @ 2014-01-02 14:44 ` Arnaldo Carvalho de Melo 1 sibling, 0 replies; 12+ messages in thread From: Arnaldo Carvalho de Melo @ 2014-01-02 14:44 UTC (permalink / raw) To: David Ahern; +Cc: Ingo Molnar, linux-kernel, Stephane Eranian [-- Attachment #1: Type: text/plain, Size: 4154 bytes --] Em Thu, Dec 26, 2013 at 09:11:22AM -0500, David Ahern escreveu: > On 12/24/13, 8:30 AM, Arnaldo Carvalho de Melo wrote: > >Em Tue, Dec 24, 2013 at 09:53:42AM -0300, Arnaldo Carvalho de Melo escreveu: > >>>The thing to check is perf_evlist__{prepare,start}_workload notification > >>>errors using SIGUSR1, that we need to check for in the caller, and emit > >>>the message, no? > >Something like this: > > > >1. We tell perf_evlist__prepare_workload that we want a signal if execvp > >fails, it will be a SIGUSR1 > > > >2. We catch that signal in 'stat' and check that we got a signal, only > >problem so far with this signal maze is that we're getting a SIGCHLD > >while I was expecting a SIGUSR1... I.e. the "if (signr != -1) test > >really should be if (signr == SIGUSR1), but I'm getting a SIGCHLD there > >and the elves are tugging me away... > > Did the elves release you? > > There are all kinds of failure paths with the workload functions. In > the end I was focusing on perf-stat actually checking the rc on the > start_workload function. If it fails, then write() failed and > something happened to the workload process. In that case don't show > the counters. > > Handling the other error paths with appropriate messages will take > additional effort - as you are finding. ;-) So, please try the attached patch, then apply this one on top, still needs work for 'record' and 'trace', that now don't print anything when a workload fails. I'll work on passing a pointer to evlist to the sigaction, then all this will be hidden away inside perf_evlist__prepare_workload, etc. diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 1c76c7a66f78..9d0d52d55ee6 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -58,6 +58,7 @@ #include "util/thread.h" #include "util/thread_map.h" +#include <signal.h> #include <stdlib.h> #include <sys/prctl.h> #include <locale.h> @@ -509,16 +510,17 @@ static void handle_initial_delay(void) } } -static volatile bool workload_exec_failed; +static volatile int workload_exec_errno; /* * perf_evlist__prepare_workload will send a SIGUSR1 * if the fork fails, since we asked by setting its * want_signal to true. */ -static void workload_exec_failed_signal(int signo __maybe_unused) +static void workload_exec_failed_signal(int signo __maybe_unused, siginfo_t *info, + void *ucontext __maybe_unused) { - workload_exec_failed = true; + workload_exec_errno = info->si_value.sival_int; } static int __run_perf_stat(int argc, const char **argv) @@ -596,13 +598,17 @@ static int __run_perf_stat(int argc, const char **argv) clock_gettime(CLOCK_MONOTONIC, &ref_time); if (forks) { + struct sigaction act = { + .sa_flags = SA_SIGINFO, + .sa_sigaction = workload_exec_failed_signal, + }; /* * perf_evlist__prepare_workload will, after we call * perf_evlist__start_Workload, send a SIGUSR1 if the exec call * fails, that we will catch in workload_signal to flip - * workload_exec_failed. + * workload_exec_errno. */ - signal(SIGUSR1, workload_exec_failed_signal); + sigaction(SIGUSR1, &act, NULL); perf_evlist__start_workload(evsel_list); handle_initial_delay(); @@ -615,8 +621,11 @@ static int __run_perf_stat(int argc, const char **argv) } wait(&status); - if (workload_exec_failed) + if (workload_exec_errno) { + const char *emsg = strerror_r(workload_exec_errno, msg, sizeof(msg)); + pr_err("Workload failed: %s\n", emsg); return -1; + } if (WIFSIGNALED(status)) psignal(WTERMSIG(status), argv[0]); diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c index b08a7ecdcea1..4a30c87d24ec 100644 --- a/tools/perf/util/evlist.c +++ b/tools/perf/util/evlist.c @@ -1073,9 +1073,14 @@ int perf_evlist__prepare_workload(struct perf_evlist *evlist, struct target *tar execvp(argv[0], (char **)argv); - perror(argv[0]); - if (want_signal) - kill(getppid(), SIGUSR1); + if (want_signal) { + union sigval val; + + val.sival_int = errno; + if (sigqueue(getppid(), SIGUSR1, val)) + perror(argv[0]); + } else + perror(argv[0]); exit(-1); } [-- Attachment #2: 0001-perf-stat-Don-t-show-counter-information-when-worklo.patch --] [-- Type: text/plain, Size: 3397 bytes --] >From b015481108106b7ef42d46a5096b572b1bd71b50 Mon Sep 17 00:00:00 2001 From: Arnaldo Carvalho de Melo <acme@redhat.com> Date: Sat, 28 Dec 2013 15:45:08 -0300 Subject: [PATCH] perf stat: Don't show counter information when workload fails When starting a workload 'stat' wasn't using prepare_workload evlist method's signal based exec() error reporting mechanism. Use it so that the we don't report 'not counted' counters. Before: [acme@zoo linux]$ perf stat dfadsfa dfadsfa: No such file or directory Performance counter stats for 'dfadsfa': <not counted> task-clock <not counted> context-switches <not counted> cpu-migrations <not counted> page-faults <not counted> cycles <not counted> stalled-cycles-frontend <not supported> stalled-cycles-backend <not counted> instructions <not counted> branches <not counted> branch-misses 0.001831462 seconds time elapsed [acme@zoo linux]$ After: [acme@zoo linux]$ perf stat dfadsfa dfadsfa: No such file or directory [acme@zoo linux]$ Reported-by: David Ahern <dsahern@gmail.com> Cc: Adrian Hunter <adrian.hunter@intel.com> Cc: David Ahern <dsahern@gmail.com> Cc: Frederic Weisbecker <fweisbec@gmail.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mike Galbraith <efault@gmx.de> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Paul Mackerras <paulus@samba.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Stephane Eranian <eranian@google.com> Link: http://lkml.kernel.org/n/tip-5yui3bv7e3hitxucnjsn6z8q@git.kernel.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> --- tools/perf/builtin-stat.c | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c index 106a5e5b7842..1c76c7a66f78 100644 --- a/tools/perf/builtin-stat.c +++ b/tools/perf/builtin-stat.c @@ -509,6 +509,18 @@ static void handle_initial_delay(void) } } +static volatile bool workload_exec_failed; + +/* + * perf_evlist__prepare_workload will send a SIGUSR1 + * if the fork fails, since we asked by setting its + * want_signal to true. + */ +static void workload_exec_failed_signal(int signo __maybe_unused) +{ + workload_exec_failed = true; +} + static int __run_perf_stat(int argc, const char **argv) { char msg[512]; @@ -529,7 +541,7 @@ static int __run_perf_stat(int argc, const char **argv) if (forks) { if (perf_evlist__prepare_workload(evsel_list, &target, argv, - false, false) < 0) { + false, true) < 0) { perror("failed to prepare workload"); return -1; } @@ -584,6 +596,14 @@ static int __run_perf_stat(int argc, const char **argv) clock_gettime(CLOCK_MONOTONIC, &ref_time); if (forks) { + /* + * perf_evlist__prepare_workload will, after we call + * perf_evlist__start_Workload, send a SIGUSR1 if the exec call + * fails, that we will catch in workload_signal to flip + * workload_exec_failed. + */ + signal(SIGUSR1, workload_exec_failed_signal); + perf_evlist__start_workload(evsel_list); handle_initial_delay(); @@ -594,6 +614,10 @@ static int __run_perf_stat(int argc, const char **argv) } } wait(&status); + + if (workload_exec_failed) + return -1; + if (WIFSIGNALED(status)) psignal(WTERMSIG(status), argv[0]); } else { -- 1.8.5.rc2 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2014-01-02 14:44 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-20 5:52 [PATCH] perf stat: Do not show stats if workload fails David Ahern 2013-12-20 7:57 ` Ingo Molnar 2013-12-23 19:37 ` Arnaldo Carvalho de Melo 2013-12-24 0:29 ` David Ahern 2013-12-24 12:53 ` Arnaldo Carvalho de Melo 2013-12-24 13:30 ` Arnaldo Carvalho de Melo 2013-12-26 14:11 ` David Ahern 2013-12-26 14:15 ` Arnaldo Carvalho de Melo 2013-12-26 14:18 ` David Ahern 2013-12-26 14:26 ` Arnaldo Carvalho de Melo 2013-12-26 14:55 ` Arnaldo Carvalho de Melo 2014-01-02 14:44 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox