From 7f2eb6e7a5b401d43c7b98befe6e1ead94b8d355 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 29 Nov 2017 16:45:40 +0100 Subject: [PATCH 1/2] pthreadpool: Fix starvation after fork After the race is before the race: 1) Create an idle thread 2) Add a job: This won't create a thread anymore 3) Immediately fork The idle thread will be woken twice before it's actually woken up: Both pthreadpool_add_job and pthreadpool_prepare_pool call cond_signal, for different reasons. We must look at pool->prefork_cond first because otherwise we will end up in a blocking job deep within a fork call, the helper thread must take its fingers off the condvar as quickly as possible. This means that after the fork there's no idle thread around anymore that would pick up the job submitted in 2). So we must keep the idle threads around across the fork. The quick solution to re-create one helper thread in pthreadpool_parent has a fatal flaw: What do we do if that pthread_create call fails? We're deep in an application calling fork(), and doing fancy signalling from there is really something we must avoid. This has one potential performance issue: If we have hundreds of idle threads (do we ever have that) during the fork, the call to pthread_mutex_lock on the fork_mutex from pthreadpool_server (the helper thread) will probably cause a thundering herd when the _parent call unlocks the fork_mutex. The solution for this to just keep one idle thread around. But this adds code that is not strictly required functionally for now. More detailed explanation from Jeremy: First, understanding the problem the test reproduces: add a job (num_jobs = 1) -> creates thread to run it. job finishes, thread sticks around (num_idle = 1). num_jobs is now zero (initial job finished). a) Idle thread is now waiting on pool->condvar inside pthreadpool_server() in pthread_cond_timedwait(). Now, add another job -> pthreadpool_add_job() -> pthreadpool_put_job() This adds the job to the queue. Oh, there is an idle thread so don't create one, do: pthread_cond_signal(&pool->condvar); and return. Now call fork *before* idle thread in (a) wakes from the signaling of pool->condvar. In the parent (child is irrelevent): Go into: pthreadpool_prepare() -> pthreadpool_prepare_pool() Set the variable to tell idle threads to exit: pool->prefork_cond = &prefork_cond; then wake them up with: pthread_cond_signal(&pool->condvar); This does nothing as the idle thread is already awoken. b) Idle thread wakes up and does: Reduce idle thread count (num_idle = 0) pool->num_idle -= 1; Check if we're in the middle of a fork. if (pool->prefork_cond != NULL) { Yes we are, tell pthreadpool_prepare() we are exiting. pthread_cond_signal(pool->prefork_cond); And exit. pthreadpool_server_exit(pool); return NULL; } So we come back from the fork in the parent with num_jobs = 1, a job on the queue but no idle threads - and the code that creates a new thread on job submission was skipped because an idle thread existed at point (a). OK, assuming that the previous explaination is correct, the fix is to create a new pthreadpool context mutex: pool->fork_mutex and in pthreadpool_server(), when an idle thread wakes up and notices we're in the prepare fork state, it puts itself to sleep by waiting on the new pool->fork_mutex. And in pthreadpool_prepare_pool(), instead of waiting for the idle threads to exit, hold the pool->fork_mutex and signal each idle thread in turn, and wait for the pool->num_idle to go to zero - which means they're all blocked waiting on pool->fork_mutex. When the parent continues, pthreadpool_parent() unlocks the pool->fork_mutex and all the previously 'idle' threads wake up (and you mention the thundering herd problem, which is as you say vanishingly small :-) and pick up any remaining job. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit f6858505aec9f1004aeaffa83f21e58868749d65) --- lib/pthreadpool/pthreadpool.c | 93 ++++++++++++++++++++++++++++++++++--------- 1 file changed, 75 insertions(+), 18 deletions(-) diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c index 309aba98866..b70694a6a1b 100644 --- a/lib/pthreadpool/pthreadpool.c +++ b/lib/pthreadpool/pthreadpool.c @@ -91,11 +91,19 @@ struct pthreadpool { int num_idle; /* - * Condition variable indicating that we should quickly go - * away making way for fork() without anybody waiting on - * pool->condvar. + * Condition variable indicating that helper threads should + * quickly go away making way for fork() without anybody + * waiting on pool->condvar. */ pthread_cond_t *prefork_cond; + + /* + * Waiting position for helper threads while fork is + * running. The forking thread will have locked it, and all + * idle helper threads will sit here until after the fork, + * where the forking thread will unlock it again. + */ + pthread_mutex_t fork_mutex; }; static pthread_mutex_t pthreadpools_mutex = PTHREAD_MUTEX_INITIALIZER; @@ -151,6 +159,15 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult, return ret; } + ret = pthread_mutex_init(&pool->fork_mutex, NULL); + if (ret != 0) { + pthread_cond_destroy(&pool->condvar); + pthread_mutex_destroy(&pool->mutex); + free(pool->jobs); + free(pool); + return ret; + } + pool->shutdown = false; pool->num_threads = 0; pool->max_threads = max_threads; @@ -159,6 +176,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult, ret = pthread_mutex_lock(&pthreadpools_mutex); if (ret != 0) { + pthread_mutex_destroy(&pool->fork_mutex); pthread_cond_destroy(&pool->condvar); pthread_mutex_destroy(&pool->mutex); free(pool->jobs); @@ -179,18 +197,26 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult, static void pthreadpool_prepare_pool(struct pthreadpool *pool) { - pthread_cond_t prefork_cond = PTHREAD_COND_INITIALIZER; int ret; + ret = pthread_mutex_lock(&pool->fork_mutex); + assert(ret == 0); + ret = pthread_mutex_lock(&pool->mutex); assert(ret == 0); while (pool->num_idle != 0) { + int num_idle = pool->num_idle; + pthread_cond_t prefork_cond; + + ret = pthread_cond_init(&prefork_cond, NULL); + assert(ret == 0); + /* - * Exit all idle threads, which are all blocked in - * pool->condvar. In the child we can destroy the - * pool, which would result in undefined behaviour in - * the pthread_cond_destroy(pool->condvar). glibc just + * Push all idle threads off pool->condvar. In the + * child we can destroy the pool, which would result + * in undefined behaviour in the + * pthread_cond_destroy(pool->condvar). glibc just * blocks here. */ pool->prefork_cond = &prefork_cond; @@ -198,14 +224,16 @@ static void pthreadpool_prepare_pool(struct pthreadpool *pool) ret = pthread_cond_signal(&pool->condvar); assert(ret == 0); - ret = pthread_cond_wait(&prefork_cond, &pool->mutex); - assert(ret == 0); + while (pool->num_idle == num_idle) { + ret = pthread_cond_wait(&prefork_cond, &pool->mutex); + assert(ret == 0); + } pool->prefork_cond = NULL; - } - ret = pthread_cond_destroy(&prefork_cond); - assert(ret == 0); + ret = pthread_cond_destroy(&prefork_cond); + assert(ret == 0); + } /* * Probably it's well-defined somewhere: What happens to @@ -246,6 +274,8 @@ static void pthreadpool_parent(void) assert(ret == 0); ret = pthread_mutex_unlock(&pool->mutex); assert(ret == 0); + ret = pthread_mutex_unlock(&pool->fork_mutex); + assert(ret == 0); } ret = pthread_mutex_unlock(&pthreadpools_mutex); @@ -268,8 +298,12 @@ static void pthreadpool_child(void) ret = pthread_cond_init(&pool->condvar, NULL); assert(ret == 0); + ret = pthread_mutex_unlock(&pool->mutex); assert(ret == 0); + + ret = pthread_mutex_unlock(&pool->fork_mutex); + assert(ret == 0); } ret = pthread_mutex_unlock(&pthreadpools_mutex); @@ -284,7 +318,7 @@ static void pthreadpool_prep_atfork(void) static int pthreadpool_free(struct pthreadpool *pool) { - int ret, ret1; + int ret, ret1, ret2; ret = pthread_mutex_lock(&pthreadpools_mutex); if (ret != 0) { @@ -296,6 +330,7 @@ static int pthreadpool_free(struct pthreadpool *pool) ret = pthread_mutex_destroy(&pool->mutex); ret1 = pthread_cond_destroy(&pool->condvar); + ret2 = pthread_mutex_destroy(&pool->fork_mutex); if (ret != 0) { return ret; @@ -303,6 +338,9 @@ static int pthreadpool_free(struct pthreadpool *pool) if (ret1 != 0) { return ret1; } + if (ret2 != 0) { + return ret2; + } free(pool->jobs); free(pool); @@ -467,11 +505,30 @@ static void *pthreadpool_server(void *arg) /* * Me must allow fork() to continue * without anybody waiting on - * &pool->condvar. + * &pool->condvar. Tell + * pthreadpool_prepare_pool that we + * got that message. */ - pthread_cond_signal(pool->prefork_cond); - pthreadpool_server_exit(pool); - return NULL; + + res = pthread_cond_signal(pool->prefork_cond); + assert(res == 0); + + res = pthread_mutex_unlock(&pool->mutex); + assert(res == 0); + + /* + * pthreadpool_prepare_pool has + * already locked this mutex across + * the fork. This makes us wait + * without sitting in a condvar. + */ + res = pthread_mutex_lock(&pool->fork_mutex); + assert(res == 0); + res = pthread_mutex_unlock(&pool->fork_mutex); + assert(res == 0); + + res = pthread_mutex_lock(&pool->mutex); + assert(res == 0); } if (res == ETIMEDOUT) { -- 2.11.0 From 40dcc47867c82914f15f3e0fd8dadadd2138b10e Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Wed, 29 Nov 2017 18:55:21 +0100 Subject: [PATCH 2/2] pthreadpool: Add a test for the race condition fixed in the last commit Bug: https://bugzilla.samba.org/show_bug.cgi?id=13179 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 53f7bbca0451e4f57cdbe8ab4f67f601fe8d40c1) --- lib/pthreadpool/tests.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) diff --git a/lib/pthreadpool/tests.c b/lib/pthreadpool/tests.c index 999118286eb..0ea285d970e 100644 --- a/lib/pthreadpool/tests.c +++ b/lib/pthreadpool/tests.c @@ -300,6 +300,82 @@ static int test_busyfork(void) return 0; } +static int test_busyfork2(void) +{ + struct pthreadpool_pipe *p; + pid_t child; + int ret, jobnum; + struct pollfd pfd; + + ret = pthreadpool_pipe_init(1, &p); + if (ret != 0) { + fprintf(stderr, "pthreadpool_pipe_init failed: %s\n", + strerror(ret)); + return -1; + } + + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL); + if (ret != 0) { + fprintf(stderr, "pthreadpool_add_job failed: %s\n", + strerror(ret)); + return -1; + } + + ret = pthreadpool_pipe_finished_jobs(p, &jobnum, 1); + if (ret != 1) { + fprintf(stderr, "pthreadpool_pipe_finished_jobs failed\n"); + return -1; + } + + ret = poll(NULL, 0, 10); + if (ret == -1) { + perror("poll failed"); + return -1; + } + + ret = pthreadpool_pipe_add_job(p, 1, busyfork_job, NULL); + if (ret != 0) { + fprintf(stderr, "pthreadpool_add_job failed: %s\n", + strerror(ret)); + return -1; + } + + /* + * Do the fork right after the add_job. This tests a race + * where the atfork prepare handler gets all idle threads off + * the condvar. If we are faster doing the fork than the + * existing idle thread could get out of idle and take the + * job, after the fork we end up with no threads to take care + * of the job. + */ + + child = fork(); + if (child < 0) { + perror("fork failed"); + return -1; + } + + if (child == 0) { + exit(0); + } + + pfd = (struct pollfd) { + .fd = pthreadpool_pipe_signal_fd(p), + .events = POLLIN|POLLERR + }; + + do { + ret = poll(&pfd, 1, 5000); + } while ((ret == -1) && (errno == EINTR)); + + if (ret == 0) { + fprintf(stderr, "job unfinished after 5 seconds\n"); + return -1; + } + + return 0; +} + static void test_tevent_wait(void *private_data) { int *timeout = private_data; @@ -415,6 +491,12 @@ int main(void) return 1; } + ret = test_busyfork2(); + if (ret != 0) { + fprintf(stderr, "test_busyfork2 failed\n"); + return 1; + } + printf("success\n"); return 0; } -- 2.11.0