The Samba-Bugzilla – Attachment 13856 Details for
Bug 13179
messaging can stop working in the parent smbd and winbind after a fork (pthreadpool race)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
patch for 4.7
patch-4.7.txt (text/plain), 12.21 KB, created by
Volker Lendecke
on 2017-12-13 10:50:55 UTC
(
hide
)
Description:
patch for 4.7
Filename:
MIME Type:
Creator:
Volker Lendecke
Created:
2017-12-13 10:50:55 UTC
Size:
12.21 KB
patch
obsolete
>From 7f2eb6e7a5b401d43c7b98befe6e1ead94b8d355 Mon Sep 17 00:00:00 2001 >From: Volker Lendecke <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 <vl@samba.org> >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 <vl@samba.org> >Reviewed-by: Jeremy Allison <jra@samba.org> >(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 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
metze
:
review+
Actions:
View
Attachments on
bug 13179
: 13856 |
13867