From da231338ec9c098707c8a1e4d8a50e2400e2fe17 Mon Sep 17 00:00:00 2001 From: Anand K Mistry Date: Wed, 13 May 2020 12:20:23 +1000 Subject: [PATCH] perf record: Use an eventfd to wakeup when done The setting and checking of 'done' contains a rare race where the signal handler setting 'done' is run after checking to break the loop, but before waiting in evlist__poll(). In this case, the main loop won't wake up until either another signal is sent, or the perf data fd causes a wake up. The following simple script can trigger this condition (but you might need to run it for several hours): for ((i = 0; i >= 0; i++)) ; do echo "Loop $i" delay=$(echo "scale=4; 0.1 * $RANDOM/32768" | bc) ./perf record -- sleep 30000000 >/dev/null& pid=$! sleep $delay kill -TERM $pid echo "PID $pid" wait $pid done At some point, the loop will stall. Adding logging, even though perf has received the SIGTERM and set 'done = 1', perf will remain sleeping until a second signal is sent. Committer notes: Make this dependent on HAVE_EVENTFD_SUPPORT, so that we continue building on older systems without the eventfd syscall. Signed-off-by: Anand K Mistry Acked-by: Jiri Olsa Cc: Alexander Shishkin Cc: Mark Rutland Cc: Namhyung Kim Cc: Peter Zijlstra Link: http://lore.kernel.org/lkml/20200513122012.v3.1.I4d7421c6bbb1f83ea58419082481082e19097841@changeid Signed-off-by: Arnaldo Carvalho de Melo --- tools/perf/builtin-record.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c index 4d4502b7fea0..c69f16361958 100644 --- a/tools/perf/builtin-record.c +++ b/tools/perf/builtin-record.c @@ -56,6 +56,9 @@ #include #include #include +#ifdef HAVE_EVENTFD_SUPPORT +#include +#endif #include #include #include @@ -538,6 +541,9 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size) static volatile int signr = -1; static volatile int child_finished; +#ifdef HAVE_EVENTFD_SUPPORT +static int done_fd = -1; +#endif static void sig_handler(int sig) { @@ -547,6 +553,21 @@ static void sig_handler(int sig) signr = sig; done = 1; +#ifdef HAVE_EVENTFD_SUPPORT +{ + u64 tmp = 1; + /* + * It is possible for this signal handler to run after done is checked + * in the main loop, but before the perf counter fds are polled. If this + * happens, the poll() will continue to wait even though done is set, + * and will only break out if either another signal is received, or the + * counters are ready for read. To ensure the poll() doesn't sleep when + * done is set, use an eventfd (done_fd) to wake up the poll(). + */ + if (write(done_fd, &tmp, sizeof(tmp)) < 0) + pr_err("failed to signal wakeup fd, error: %m\n"); +} +#endif // HAVE_EVENTFD_SUPPORT } static void sigsegv_handler(int sig) @@ -1547,6 +1568,20 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) pr_err("Compression initialization failed.\n"); return -1; } +#ifdef HAVE_EVENTFD_SUPPORT + done_fd = eventfd(0, EFD_NONBLOCK); + if (done_fd < 0) { + pr_err("Failed to create wakeup eventfd, error: %m\n"); + status = -1; + goto out_delete_session; + } + err = evlist__add_pollfd(rec->evlist, done_fd); + if (err < 0) { + pr_err("Failed to add wakeup eventfd to poll list\n"); + status = err; + goto out_delete_session; + } +#endif // HAVE_EVENTFD_SUPPORT session->header.env.comp_type = PERF_COMP_ZSTD; session->header.env.comp_level = rec->opts.comp_level; @@ -1905,6 +1940,10 @@ out_child: } out_delete_session: +#ifdef HAVE_EVENTFD_SUPPORT + if (done_fd >= 0) + close(done_fd); +#endif zstd_fini(&session->zstd_data); perf_session__delete(session); -- 2.11.0