From 31eca641ad91634e5ffcf369cd756b0506a700c1 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Tue, 31 Dec 2019 09:37:55 -0800 Subject: [PATCH] Fix a potential deadlock in helper_do_stat() pthread_mutex_lock(), pthread_cond_signal() and pthread_mutex_unlock() are not async-signal-safe and hence must not be used inside a singal handler implementation. Rework the code for communication with the helper thread such that it becomes async-signal-safe. Fixes: a47591e4923f ("Improve logging accuracy") Signed-off-by: Bart Van Assche --- helper_thread.c | 94 ++++++++++++++++++++++++++++--------------------- 1 file changed, 54 insertions(+), 40 deletions(-) diff --git a/helper_thread.c b/helper_thread.c index eba2898a..49cd4c75 100644 --- a/helper_thread.c +++ b/helper_thread.c @@ -10,48 +10,52 @@ #include "steadystate.h" #include "pshared.h" +enum action { + A_EXIT = 1, + A_RESET = 2, + A_DO_STAT = 3, +}; + static struct helper_data { volatile int exit; - volatile int reset; - volatile int do_stat; + int pipe[2]; /* 0: read end; 1: write end. */ struct sk_out *sk_out; pthread_t thread; - pthread_mutex_t lock; - pthread_cond_t cond; struct fio_sem *startup_sem; } *helper_data; void helper_thread_destroy(void) { - pthread_cond_destroy(&helper_data->cond); - pthread_mutex_destroy(&helper_data->lock); + close(helper_data->pipe[0]); + close(helper_data->pipe[1]); sfree(helper_data); } -void helper_reset(void) +static void submit_action(enum action a) { + const uint8_t data = a; + int ret; + if (!helper_data) return; - pthread_mutex_lock(&helper_data->lock); - - if (!helper_data->reset) { - helper_data->reset = 1; - pthread_cond_signal(&helper_data->cond); - } + ret = write(helper_data->pipe[1], &data, sizeof(data)); + assert(ret == 1); +} - pthread_mutex_unlock(&helper_data->lock); +void helper_reset(void) +{ + submit_action(A_RESET); } +/* + * May be invoked in signal handler context and hence must only call functions + * that are async-signal-safe. See also + * https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03. + */ void helper_do_stat(void) { - if (!helper_data) - return; - - pthread_mutex_lock(&helper_data->lock); - helper_data->do_stat = 1; - pthread_cond_signal(&helper_data->cond); - pthread_mutex_unlock(&helper_data->lock); + submit_action(A_DO_STAT); } bool helper_should_exit(void) @@ -64,14 +68,9 @@ bool helper_should_exit(void) void helper_thread_exit(void) { - void *ret; - - pthread_mutex_lock(&helper_data->lock); helper_data->exit = 1; - pthread_cond_signal(&helper_data->cond); - pthread_mutex_unlock(&helper_data->lock); - - pthread_join(helper_data->thread, &ret); + submit_action(A_EXIT); + pthread_join(helper_data->thread, NULL); } static void *helper_thread_main(void *data) @@ -79,6 +78,7 @@ static void *helper_thread_main(void *data) struct helper_data *hd = data; unsigned int msec_to_next_event, next_log, next_ss = STEADYSTATE_MSEC; struct timespec ts, last_du, last_ss; + uint8_t action; int ret = 0; sk_out_assign(hd->sk_out); @@ -96,11 +96,23 @@ static void *helper_thread_main(void *data) msec_to_next_event = DISK_UTIL_MSEC; while (!ret && !hd->exit) { uint64_t since_du, since_ss = 0; + struct timeval timeout = { + .tv_sec = DISK_UTIL_MSEC / 1000, + .tv_usec = (DISK_UTIL_MSEC % 1000) * 1000, + }; + fd_set rfds, efds; timespec_add_msec(&ts, msec_to_next_event); - pthread_mutex_lock(&hd->lock); - pthread_cond_timedwait(&hd->cond, &hd->lock, &ts); + if (read(hd->pipe[0], &action, sizeof(action)) < 0) { + FD_ZERO(&rfds); + FD_SET(hd->pipe[0], &rfds); + FD_ZERO(&efds); + FD_SET(hd->pipe[0], &efds); + select(1, &rfds, NULL, &efds, &timeout); + if (read(hd->pipe[0], &action, sizeof(action)) < 0) + action = 0; + } #ifdef CONFIG_PTHREAD_CONDATTR_SETCLOCK clock_gettime(CLOCK_MONOTONIC, &ts); @@ -108,14 +120,11 @@ static void *helper_thread_main(void *data) clock_gettime(CLOCK_REALTIME, &ts); #endif - if (hd->reset) { - memcpy(&last_du, &ts, sizeof(ts)); - memcpy(&last_ss, &ts, sizeof(ts)); - hd->reset = 0; + if (action == A_RESET) { + last_du = ts; + last_ss = ts; } - pthread_mutex_unlock(&hd->lock); - since_du = mtime_since(&last_du, &ts); if (since_du >= DISK_UTIL_MSEC || DISK_UTIL_MSEC - since_du < 10) { ret = update_io_ticks(); @@ -126,10 +135,8 @@ static void *helper_thread_main(void *data) } else msec_to_next_event = DISK_UTIL_MSEC - since_du; - if (hd->do_stat) { - hd->do_stat = 0; + if (action == A_DO_STAT) __show_running_run_stats(); - } next_log = calc_log_samples(); if (!next_log) @@ -173,10 +180,17 @@ int helper_thread_create(struct fio_sem *startup_sem, struct sk_out *sk_out) hd->sk_out = sk_out; - ret = mutex_cond_init_pshared(&hd->lock, &hd->cond); +#ifdef __linux__ + ret = pipe2(hd->pipe, O_CLOEXEC); +#else + ret = pipe(hd->pipe); +#endif if (ret) return 1; + ret = fcntl(hd->pipe[0], F_SETFL, O_NONBLOCK); + assert(ret >= 0); + hd->startup_sem = startup_sem; DRD_IGNORE_VAR(helper_data); -- 2.25.1