Attachment 13856 Details for Bug 13179 – patch for 4.7

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:

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
>

Flags:

metze: review+

Actions: View

Attachments on bug 13179: 13856 | 13867