Attachment 13533 Details for Bug 13006 – git-am partial backport for 4.5.x - test depends on pthreadpool_pipe_add_job() which doesn't exist in 4.5.x.

[patch] git-am partial backport for 4.5.x - test depends on pthreadpool_pipe_add_job() which doesn't exist in 4.5.x.

bug-13006-4.5.patch (text/plain), 4.35 KB, created by Jeremy Allison on 2017-09-01 23:05:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2017-09-01 23:05:49 UTC

Size: 4.35 KB

patch

obsolete

>From 53af186b11ad6879d6c09ac8d7d301c084fc9f10 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Fri, 1 Sep 2017 15:57:26 -0700
>Subject: [PATCH] pthreadpool: Fix fork behaviour
>
>glibc's pthread_cond_wait(&c, &m) increments m.__data.__nusers, making
>pthread_mutex_destroy return EBUSY. Thus we can't allow any thread waiting for
>a job across a fork. Also, the state of the condvar itself is unclear across a
>fork. Right now to me it looks like an initialized but unused condvar can be
>used in the child. Busy worker threads don't cause any trouble here, they don't
>hold mutexes or condvars. Also, they can't reach the condvar because _prepare
>holds all mutexes.
>
>Bug: https://bugzilla.samba.org/show_bug.cgi?id=13006
>Signed-off-by: Volker Lendecke <vl@samba.org>
>Reviewed-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>(cherry picked from commit ff98e3fb666b57b56a1427aa1196948ceebdec66)
>---
> source3/lib/pthreadpool/pthreadpool.c | 67 +++++++++++++++++++++++++++++++++--
> 1 file changed, 65 insertions(+), 2 deletions(-)
>
>diff --git a/source3/lib/pthreadpool/pthreadpool.c b/source3/lib/pthreadpool/pthreadpool.c
>index 4b745e44c7f..12cf46731b8 100644
>--- a/source3/lib/pthreadpool/pthreadpool.c
>+++ b/source3/lib/pthreadpool/pthreadpool.c
>@@ -94,6 +94,13 @@ struct pthreadpool {
> 	 */
> 	int			num_exited;
> 	pthread_t		*exited; /* We alloc more */
>+
>+	/*
>+	 * Condition variable indicating that we should quickly go
>+	 * away making way for fork() without anybody waiting on
>+	 * pool->condvar.
>+	 */
>+	pthread_cond_t *prefork_cond;
> };
> 
> static pthread_mutex_t pthreadpools_mutex = PTHREAD_MUTEX_INITIALIZER;
>@@ -160,6 +167,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult)
> 	pool->exited = NULL;
> 	pool->max_threads = max_threads;
> 	pool->num_idle = 0;
>+	pool->prefork_cond = NULL;
> 
> 	ret = pthread_mutex_lock(&pthreadpools_mutex);
> 	if (ret != 0) {
>@@ -183,6 +191,47 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult)
> 	return 0;
> }
> 
>+static void pthreadpool_prepare_pool(struct pthreadpool *pool)
>+{
>+	pthread_cond_t prefork_cond = PTHREAD_COND_INITIALIZER;
>+	int ret;
>+
>+	ret = pthread_mutex_lock(&pool->mutex);
>+	assert(ret == 0);
>+
>+	while (pool->num_idle != 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
>+		 * blocks here.
>+		 */
>+		pool->prefork_cond = &prefork_cond;
>+
>+		ret = pthread_cond_signal(&pool->condvar);
>+		assert(ret == 0);
>+
>+		ret = pthread_cond_wait(&prefork_cond, &pool->mutex);
>+		assert(ret == 0);
>+
>+		pool->prefork_cond = NULL;
>+	}
>+
>+	ret = pthread_cond_destroy(&prefork_cond);
>+	assert(ret == 0);
>+
>+	/*
>+	 * Probably it's well-defined somewhere: What happens to
>+	 * condvars after a fork? The rationale of pthread_atfork only
>+	 * writes about mutexes. So better be safe than sorry and
>+	 * destroy/reinit pool->condvar across a fork.
>+	 */
>+
>+	ret = pthread_cond_destroy(&pool->condvar);
>+	assert(ret == 0);
>+}
>+
> static void pthreadpool_prepare(void)
> {
> 	int ret;
>@@ -194,8 +243,7 @@ static void pthreadpool_prepare(void)
> 	pool = pthreadpools;
> 
> 	while (pool != NULL) {
>-		ret = pthread_mutex_lock(&pool->mutex);
>-		assert(ret == 0);
>+		pthreadpool_prepare_pool(pool);
> 		pool = pool->next;
> 	}
> }
>@@ -208,6 +256,8 @@ static void pthreadpool_parent(void)
> 	for (pool = DLIST_TAIL(pthreadpools);
> 	     pool != NULL;
> 	     pool = DLIST_PREV(pool)) {
>+		ret = pthread_cond_init(&pool->condvar, NULL);
>+		assert(ret == 0);
> 		ret = pthread_mutex_unlock(&pool->mutex);
> 		assert(ret == 0);
> 	}
>@@ -241,6 +291,8 @@ static void pthreadpool_child(void)
> 		pool->head = 0;
> 		pool->num_jobs = 0;
> 
>+		ret = pthread_cond_init(&pool->condvar, NULL);
>+		assert(ret == 0);
> 		ret = pthread_mutex_unlock(&pool->mutex);
> 		assert(ret == 0);
> 	}
>@@ -516,6 +568,17 @@ static void *pthreadpool_server(void *arg)
> 				&pool->condvar, &pool->mutex, &ts);
> 			pool->num_idle -= 1;
> 
>+			if (pool->prefork_cond != NULL) {
>+				/*
>+				 * Me must allow fork() to continue
>+				 * without anybody waiting on
>+				 * &pool->condvar.
>+				 */
>+				pthread_cond_signal(pool->prefork_cond);
>+				pthreadpool_server_exit(pool);
>+				return NULL;
>+			}
>+
> 			if (res == ETIMEDOUT) {
> 
> 				if (pool->num_jobs == 0) {
>-- 
>2.14.1.581.gf28d330327-goog
>

Actions: View

Attachments on bug 13006: 13530 | 13531 | 13532 | 13533 | 13551