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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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, ¶m)) {
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
The on_exit() function was only used in perf record but it's gone in
previous patch.
Cc: Bernhard Rosenkraenzer <[email protected]>
Cc: Irina Tirdea <[email protected]>
Signed-off-by: Namhyung Kim <[email protected]>
---
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
On Thu, Apr 24, 2014 at 3:27 PM, Namhyung Kim <[email protected]> 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 <[email protected]>
> Reported-by: Stephane Eranian <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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, ¶m)) {
> 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
>
On Thu, Apr 24, 2014 at 3:27 PM, Namhyung Kim <[email protected]> wrote:
> The on_exit() function was only used in perf record but it's gone in
> previous patch.
>
Acked-by: Stephane Eranian <[email protected]>
> Cc: Bernhard Rosenkraenzer <[email protected]>
> Cc: Irina Tirdea <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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
>
On Thu, Apr 24, 2014 at 10:27:33PM +0900, Namhyung Kim wrote:
SNIP
> Reported-by: Stephane Eranian <[email protected]>
> Signed-off-by: Namhyung Kim <[email protected]>
> ---
> 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 <[email protected]>
Date: Wed Jun 10 15:55:59 2009 +0200
but I dont think we need to do that
thanks,
jirka
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);
On Tue, Apr 29, 2014 at 12:56:54PM +0200, Jiri Olsa wrote:
>
> perf_counter tools: Propagate signals properly
> commit f7b7c26e01e51fe46097e11f179dc71ce7950084
> Author: Peter Zijlstra <[email protected]>
> 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.
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 <[email protected]>
> > 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.
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 <[email protected]>
> > 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
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 <[email protected]>
> > > 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
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.
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 <[email protected]>
>> > 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