[SCM] Samba Shared Repository - branch v4-6-test updated
Karolin Seeger
kseeger at samba.org
Fri Dec 15 14:23:02 UTC 2017
The branch, v4-6-test has been updated
via 1a8c27f pthreadpool: Add a test for the race condition fixed in the last commit
via b181b26 pthreadpool: Fix starvation after fork
from 7dcc119 winbindd: idmap_rid: error code for failing id-to-sid mapping request
https://git.samba.org/?p=samba.git;a=shortlog;h=v4-6-test
- Log -----------------------------------------------------------------
commit 1a8c27f408c84c50ca9b3573988e39c2c5db387f
Author: Volker Lendecke <vl at samba.org>
Date: Wed Nov 29 18:55:21 2017 +0100
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 at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
(cherry picked from commit 53f7bbca0451e4f57cdbe8ab4f67f601fe8d40c1)
Autobuild-User(v4-6-test): Karolin Seeger <kseeger at samba.org>
Autobuild-Date(v4-6-test): Fri Dec 15 15:22:27 CET 2017 on sn-devel-144
commit b181b26caac8678f4fe1f746f9613b9a0a9e470d
Author: Volker Lendecke <vl at samba.org>
Date: Wed Nov 29 16:45:40 2017 +0100
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 at samba.org>
Reviewed-by: Jeremy Allison <jra at samba.org>
(cherry picked from commit f6858505aec9f1004aeaffa83f21e58868749d65)
-----------------------------------------------------------------------
Summary of changes:
source3/lib/pthreadpool/pthreadpool.c | 93 ++++++++++++++++++++++++++++-------
source3/lib/pthreadpool/tests.c | 82 ++++++++++++++++++++++++++++++
2 files changed, 157 insertions(+), 18 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/lib/pthreadpool/pthreadpool.c b/source3/lib/pthreadpool/pthreadpool.c
index 309aba9..b70694a 100644
--- a/source3/lib/pthreadpool/pthreadpool.c
+++ b/source3/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) {
diff --git a/source3/lib/pthreadpool/tests.c b/source3/lib/pthreadpool/tests.c
index 9991182..0ea285d 100644
--- a/source3/lib/pthreadpool/tests.c
+++ b/source3/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;
}
--
Samba Shared Repository
More information about the samba-cvs
mailing list