| From d967e3137724fd5993dc6fee8f881e7bfbea22ca Mon Sep 17 00:00:00 2001 |
| From: Anand K Mistry <amistry@google.com> |
| Date: Wed, 13 May 2020 12:20:23 +1000 |
| Subject: 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. |
| |
| Signed-off-by: Anand K Mistry <amistry@google.com> |
| Acked-by: Jiri Olsa <jolsa@redhat.com> |
| Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> |
| Cc: Mark Rutland <mark.rutland@arm.com> |
| Cc: Namhyung Kim <namhyung@kernel.org> |
| Cc: Peter Zijlstra <peterz@infradead.org> |
| Link: http://lore.kernel.org/lkml/20200513122012.v3.1.I4d7421c6bbb1f83ea58419082481082e19097841@changeid |
| Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> |
| --- |
| tools/perf/builtin-record.c | 29 +++++++++++++++++++++++++++++ |
| 1 file changed, 29 insertions(+) |
| |
| diff --git a/builtin-record.c b/builtin-record.c |
| index 4d4502b7fea0..47f5fec7fb45 100644 |
| --- a/builtin-record.c |
| +++ b/builtin-record.c |
| @@ -56,6 +56,7 @@ |
| #include <unistd.h> |
| #include <sched.h> |
| #include <signal.h> |
| +#include <sys/eventfd.h> |
| #include <sys/mman.h> |
| #include <sys/wait.h> |
| #include <sys/types.h> |
| @@ -538,15 +539,28 @@ static int record__pushfn(struct mmap *map, void *to, void *bf, size_t size) |
| |
| static volatile int signr = -1; |
| static volatile int child_finished; |
| +static int done_fd = -1; |
| |
| static void sig_handler(int sig) |
| { |
| + u64 tmp = 1; |
| if (sig == SIGCHLD) |
| child_finished = 1; |
| else |
| signr = sig; |
| |
| done = 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"); |
| } |
| |
| static void sigsegv_handler(int sig) |
| @@ -1548,6 +1562,19 @@ static int __cmd_record(struct record *rec, int argc, const char **argv) |
| return -1; |
| } |
| |
| + 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 = perf_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; |
| + } |
| + |
| session->header.env.comp_type = PERF_COMP_ZSTD; |
| session->header.env.comp_level = rec->opts.comp_level; |
| |
| @@ -1905,6 +1932,8 @@ out_child: |
| } |
| |
| out_delete_session: |
| + if (done_fd >= 0) |
| + close(done_fd); |
| zstd_fini(&session->zstd_data); |
| perf_session__delete(session); |
| |
| -- |
| cgit 1.2.3-1.el7 |
| |