Attachment 13531 Details for Bug 13006 – git-am back-port of patch to 4.6.x. Doesn't include test.

[patch] git-am back-port of patch to 4.6.x. Doesn't include test.

bug-13006-4.6.patch (text/plain), 4.34 KB, created by Jeremy Allison on 2017-09-01 22:39:30 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeremy Allison

Created: 2017-09-01 22:39:30 UTC

Size: 4.34 KB

patch

obsolete

>From d25f27a53d8382ee6b3fe4a009a755f5c4050777 Mon Sep 17 00:00:00 2001
>From: Volker Lendecke <vl@samba.org>
>Date: Mon, 28 Aug 2017 16:38:19 +0200
>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 f97cdcc6c15..23885aa6d11 100644
>--- a/source3/lib/pthreadpool/pthreadpool.c
>+++ b/source3/lib/pthreadpool/pthreadpool.c
>@@ -89,6 +89,13 @@ struct pthreadpool {
> 	 * Number of idle threads
> 	 */
> 	int num_idle;
>+
>+	/*
>+	 * 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;
>@@ -148,6 +155,7 @@ int pthreadpool_init(unsigned max_threads, struct pthreadpool **presult,
> 	pool->num_threads = 0;
> 	pool->max_threads = max_threads;
> 	pool->num_idle = 0;
>+	pool->prefork_cond = NULL;
> 
> 	ret = pthread_mutex_lock(&pthreadpools_mutex);
> 	if (ret != 0) {
>@@ -169,6 +177,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;
>@@ -180,8 +229,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;
> 	}
> }
>@@ -194,6 +242,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);
> 	}
>@@ -216,6 +266,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);
> 	}
>@@ -406,6 +458,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