Attachment 13852 Details for Bug 13170 – Patches for 4.7

[patch] Patches for 4.7

patches-v4-7 (text/plain), 13.58 KB, created by Christof Schmitt on 2017-12-08 16:28:23 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Christof Schmitt

Created: 2017-12-08 16:28:23 UTC

Size: 13.58 KB

patch

obsolete

>From 36c081bd45a46ba7dc58cc431660ecbd4507ade8 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Tue, 28 Nov 2017 10:49:36 -0700
>Subject: [PATCH 1/4] pthreadpool: Move creating of thread to new function
>
>No functional change, but this simplifies error handling.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit 949ccc3ea9073a3d38bff28345f644d39177256f)
>---
> lib/pthreadpool/pthreadpool.c | 79 ++++++++++++++++++++++++-------------------
> 1 file changed, 45 insertions(+), 34 deletions(-)
>
>diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
>index 23885aa..4e1e5d4 100644
>--- a/lib/pthreadpool/pthreadpool.c
>+++ b/lib/pthreadpool/pthreadpool.c
>@@ -521,14 +521,56 @@ static void *pthreadpool_server(void *arg)
> 	}
> }
> 
>-int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>-			void (*fn)(void *private_data), void *private_data)
>+static int pthreadpool_create_thread(struct pthreadpool *pool)
> {
> 	pthread_attr_t thread_attr;
> 	pthread_t thread_id;
> 	int res;
> 	sigset_t mask, omask;
> 
>+	/*
>+	 * Create a new worker thread. It should not receive any signals.
>+	 */
>+
>+	sigfillset(&mask);
>+
>+	res = pthread_attr_init(&thread_attr);
>+	if (res != 0) {
>+		return res;
>+	}
>+
>+	res = pthread_attr_setdetachstate(
>+		&thread_attr, PTHREAD_CREATE_DETACHED);
>+	if (res != 0) {
>+		pthread_attr_destroy(&thread_attr);
>+		return res;
>+	}
>+
>+	res = pthread_sigmask(SIG_BLOCK, &mask, &omask);
>+	if (res != 0) {
>+		pthread_attr_destroy(&thread_attr);
>+		return res;
>+	}
>+
>+	res = pthread_create(&thread_id, &thread_attr, pthreadpool_server,
>+			     (void *)pool);
>+
>+	assert(pthread_sigmask(SIG_SETMASK, &omask, NULL) == 0);
>+
>+	pthread_attr_destroy(&thread_attr);
>+
>+	if (res == 0) {
>+		pool->num_threads += 1;
>+	}
>+
>+	return res;
>+}
>+
>+int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
>+			void (*fn)(void *private_data), void *private_data)
>+{
>+	int res;
>+
> 	res = pthread_mutex_lock(&pool->mutex);
> 	if (res != 0) {
> 		return res;
>@@ -570,43 +612,12 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
> 		return 0;
> 	}
> 
>-	/*
>-	 * Create a new worker thread. It should not receive any signals.
>-	 */
>-
>-	sigfillset(&mask);
>-
>-	res = pthread_attr_init(&thread_attr);
>-	if (res != 0) {
>-		pthread_mutex_unlock(&pool->mutex);
>-		return res;
>-	}
>-
>-	res = pthread_attr_setdetachstate(
>-		&thread_attr, PTHREAD_CREATE_DETACHED);
>+	res = pthreadpool_create_thread(pool);
> 	if (res != 0) {
>-		pthread_attr_destroy(&thread_attr);
>-		pthread_mutex_unlock(&pool->mutex);
>-		return res;
>-	}
>-
>-        res = pthread_sigmask(SIG_BLOCK, &mask, &omask);
>-	if (res != 0) {
>-		pthread_attr_destroy(&thread_attr);
> 		pthread_mutex_unlock(&pool->mutex);
> 		return res;
> 	}
> 
>-	res = pthread_create(&thread_id, &thread_attr, pthreadpool_server,
>-			     (void *)pool);
>-	if (res == 0) {
>-		pool->num_threads += 1;
>-	}
>-
>-        assert(pthread_sigmask(SIG_SETMASK, &omask, NULL) == 0);
>-
>-	pthread_attr_destroy(&thread_attr);
>-
> 	pthread_mutex_unlock(&pool->mutex);
> 	return res;
> }
>-- 
>1.8.3.1
>
>
>From be44aaf6daa535bf225af8dec2f4847457bcef64 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Tue, 28 Nov 2017 10:59:06 -0700
>Subject: [PATCH 2/4] pthreadpool: Undo put_job when returning error
>
>When an error is returned to the caller of pthreadpool_add_job, the job
>should not be kept in the internal job array. Otherwise the caller might
>free the data structure and a later worker thread would still reference
>it.
>
>When it is not possible to create a single worker thread, the system
>might be out of resources or hitting a configured limit. In this case
>fall back to calling the job function synchronously instead of raising
>the error to the caller and possibly back to the SMB client.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Volker Lendecke <vl@samba.org>
>(cherry picked from commit 065fb5d94d25d19fc85832bb85aa9e379e8551cc)
>---
> lib/pthreadpool/pthreadpool.c | 28 ++++++++++++++++++++++++++--
> 1 file changed, 26 insertions(+), 2 deletions(-)
>
>diff --git a/lib/pthreadpool/pthreadpool.c b/lib/pthreadpool/pthreadpool.c
>index 4e1e5d4..309aba9 100644
>--- a/lib/pthreadpool/pthreadpool.c
>+++ b/lib/pthreadpool/pthreadpool.c
>@@ -429,6 +429,11 @@ static bool pthreadpool_put_job(struct pthreadpool *p,
> 	return true;
> }
> 
>+static void pthreadpool_undo_put_job(struct pthreadpool *p)
>+{
>+	p->num_jobs -= 1;
>+}
>+
> static void *pthreadpool_server(void *arg)
> {
> 	struct pthreadpool *pool = (struct pthreadpool *)arg;
>@@ -599,6 +604,9 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
> 		 * We have idle threads, wake one.
> 		 */
> 		res = pthread_cond_signal(&pool->condvar);
>+		if (res != 0) {
>+			pthreadpool_undo_put_job(pool);
>+		}
> 		pthread_mutex_unlock(&pool->mutex);
> 		return res;
> 	}
>@@ -614,8 +622,24 @@ int pthreadpool_add_job(struct pthreadpool *pool, int job_id,
> 
> 	res = pthreadpool_create_thread(pool);
> 	if (res != 0) {
>-		pthread_mutex_unlock(&pool->mutex);
>-		return res;
>+		if (pool->num_threads == 0) {
>+			/*
>+			 * No thread could be created to run job,
>+			 * fallback to sync call.
>+			 */
>+			pthreadpool_undo_put_job(pool);
>+			pthread_mutex_unlock(&pool->mutex);
>+
>+			fn(private_data);
>+			return pool->signal_fn(job_id, fn, private_data,
>+					       pool->signal_fn_private_data);
>+		}
>+
>+		/*
>+		 * At least one thread is still available, let
>+		 * that one run the queued job.
>+		 */
>+		res = 0;
> 	}
> 
> 	pthread_mutex_unlock(&pool->mutex);
>-- 
>1.8.3.1
>
>
>From a02bc6afa2786e1273a39c9b8d56ef7612218e02 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Thu, 7 Dec 2017 10:42:30 -0700
>Subject: [PATCH 3/4] wscript: Add check for --wrap linker flag
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
>
>Signed-off-by: Christof Schmitt <cs@samba.org>
>Reviewed-by: Andreas Schneider <asn@samba.org>
>(cherry picked from commit 8e17be1c3df09c238560c8a7e62c17e9f9ff9bc7)
>---
> wscript | 4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/wscript b/wscript
>index 35cb9d1..542a60c 100644
>--- a/wscript
>+++ b/wscript
>@@ -157,6 +157,10 @@ def configure(conf):
> 
>     conf.RECURSE('lib/ldb')
> 
>+    if conf.CHECK_LDFLAGS(['-Wl,--wrap=test']):
>+        conf.env['HAVE_LDWRAP'] = True
>+        conf.define('HAVE_LDWRAP', 1)
>+
>     if not (Options.options.without_ad_dc):
>         conf.DEFINE('AD_DC_BUILD_IS_ENABLED', 1)
> 
>-- 
>1.8.3.1
>
>
>From 46fb8351e5ed3ab23fc45dfb2cbc27f14434de47 Mon Sep 17 00:00:00 2001
>From: Christof Schmitt <cs@samba.org>
>Date: Wed, 6 Dec 2017 15:10:23 -0700
>Subject: [PATCH 4/4] pthreadpool: Add test for pthread_create failure
>
>This is implemented using cmocka and the __wrap override for
>pthread_create.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13170
>
>Signed-off-by: Christof Schmitt <cs@samba.org
>Reviewed-by: Andreas Schneider <asn@samba.org>
>
>Autobuild-User(master): Christof Schmitt <cs@samba.org>
>Autobuild-Date(master): Fri Dec  8 13:54:20 CET 2017 on sn-devel-144
>
>(cherry picked from commit 8cdb3995caf7a21d0c27a56a0bf0c1efd5b491e4)
>---
> lib/pthreadpool/tests_cmocka.c | 176 +++++++++++++++++++++++++++++++++++++++++
> lib/pthreadpool/wscript_build  |   7 ++
> source3/selftest/tests.py      |   7 ++
> 3 files changed, 190 insertions(+)
> create mode 100644 lib/pthreadpool/tests_cmocka.c
>
>diff --git a/lib/pthreadpool/tests_cmocka.c b/lib/pthreadpool/tests_cmocka.c
>new file mode 100644
>index 0000000..75a935f
>--- /dev/null
>+++ b/lib/pthreadpool/tests_cmocka.c
>@@ -0,0 +1,176 @@
>+/*
>+ * Unix SMB/CIFS implementation.
>+ * cmocka tests for thread pool implementation
>+ * Copyright (C) Christof Schmitt 2017
>+ *
>+ * This program is free software; you can redistribute it and/or modify
>+ * it under the terms of the GNU General Public License as published by
>+ * the Free Software Foundation; either version 3 of the License, or
>+ * (at your option) any later version.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>+ * GNU General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public License
>+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>+ */
>+
>+#include <errno.h>
>+#include <pthread.h>
>+#include <setjmp.h>
>+#include <stdlib.h>
>+#include <string.h>
>+
>+#include <talloc.h>
>+#include <tevent.h>
>+#include <pthreadpool_tevent.h>
>+
>+#include <cmocka.h>
>+
>+struct pthreadpool_tevent_test {
>+	struct tevent_context *ev;
>+	struct pthreadpool_tevent *pool;
>+};
>+
>+static int setup_pthreadpool_tevent(void **state)
>+{
>+	struct pthreadpool_tevent_test *t;
>+	int ret;
>+
>+	t = talloc(NULL, struct pthreadpool_tevent_test);
>+	assert_non_null(t);
>+
>+	t->ev = tevent_context_init(t);
>+	assert_non_null(t->ev);
>+
>+	ret = pthreadpool_tevent_init(t->ev, 0, &t->pool);
>+	assert_return_code(ret, 0);
>+
>+	*state = t;
>+
>+	return 0;
>+}
>+
>+static int teardown_pthreadpool_tevent(void **state)
>+{
>+	struct pthreadpool_tevent_test *t = *state;
>+
>+	TALLOC_FREE(t);
>+
>+	return 0;
>+}
>+
>+int __wrap_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
>+			  void *(*start_routine) (void *), void *arg);
>+int __real_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
>+			  void *(*start_routine) (void *),  void *arg);
>+
>+int __wrap_pthread_create(pthread_t *thread, const pthread_attr_t *attr,
>+			  void *(*start_routine) (void *), void *arg)
>+{
>+	int error;
>+
>+	error = mock_type(int);
>+	if (error != 0) {
>+		return error;
>+	}
>+
>+	return __real_pthread_create(thread, attr, start_routine, arg);
>+}
>+
>+static void test_job_threadid(void *ptr)
>+{
>+	pthread_t *threadid = ptr;
>+
>+	*threadid = pthread_self();
>+}
>+
>+static int test_create_do(struct tevent_context *ev,
>+			  struct pthreadpool_tevent *pool,
>+			  bool *in_main_thread)
>+{
>+	struct tevent_req *req;
>+	pthread_t main_thread, worker_thread;
>+	bool ok;
>+	int ret;
>+
>+	main_thread = pthread_self();
>+
>+	req = pthreadpool_tevent_job_send(
>+		ev, ev, pool, test_job_threadid, &worker_thread);
>+	if (req == NULL) {
>+		fprintf(stderr, "pthreadpool_tevent_job_send failed\n");
>+		TALLOC_FREE(ev);
>+		return ENOMEM;
>+	}
>+
>+	ok = tevent_req_poll(req, ev);
>+	if (!ok) {
>+		ret = errno;
>+		fprintf(stderr, "tevent_req_poll failed: %s\n",
>+			strerror(ret));
>+		TALLOC_FREE(ev);
>+		return ret;
>+	}
>+
>+
>+	ret = pthreadpool_tevent_job_recv(req);
>+	TALLOC_FREE(req);
>+	if (ret != 0) {
>+		fprintf(stderr, "tevent_req_recv failed: %s\n",
>+			strerror(ret));
>+		TALLOC_FREE(ev);
>+		return ret;
>+	}
>+	*in_main_thread = pthread_equal(worker_thread, main_thread);
>+
>+	return 0;
>+}
>+
>+static void test_create(void **state)
>+{
>+	struct pthreadpool_tevent_test *t = *state;
>+	bool in_main_thread;
>+	int ret;
>+
>+	/*
>+	 * When pthreadpool cannot create the first worker thread,
>+	 * this job will run in the sync fallback in the main thread.
>+	 */
>+	will_return(__wrap_pthread_create, EAGAIN);
>+	ret = test_create_do(t->ev, t->pool, &in_main_thread);
>+	assert_return_code(ret, 0);
>+	assert_true(in_main_thread);
>+
>+	/*
>+	 * When a thread can be created, the job will run in the worker thread.
>+	 */
>+	will_return(__wrap_pthread_create, 0);
>+	ret = test_create_do(t->ev, t->pool, &in_main_thread);
>+	assert_return_code(ret, 0);
>+	assert_false(in_main_thread);
>+
>+	/*
>+	 * Workerthread will still be active for a second; immediately
>+	 * running another job will also use the worker thread, even
>+	 * if a new thread cannot be created.
>+	 */
>+	ret = test_create_do(t->ev, t->pool, &in_main_thread);
>+	assert_return_code(ret, 0);
>+	assert_false(in_main_thread);
>+}
>+
>+int main(int argc, char **argv)
>+{
>+	const struct CMUnitTest tests[] = {
>+		cmocka_unit_test_setup_teardown(test_create,
>+						setup_pthreadpool_tevent,
>+						teardown_pthreadpool_tevent),
>+	};
>+
>+	cmocka_set_message_output(CM_OUTPUT_SUBUNIT);
>+
>+	return cmocka_run_group_tests(tests, NULL, NULL);
>+}
>diff --git a/lib/pthreadpool/wscript_build b/lib/pthreadpool/wscript_build
>index d530463..57df255 100644
>--- a/lib/pthreadpool/wscript_build
>+++ b/lib/pthreadpool/wscript_build
>@@ -21,3 +21,10 @@ bld.SAMBA_BINARY('pthreadpooltest',
>                   deps='PTHREADPOOL',
>                   enabled=bld.env.WITH_PTHREADPOOL,
>                   install=False)
>+
>+bld.SAMBA_BINARY('pthreadpooltest_cmocka',
>+                  source='tests_cmocka.c',
>+                  deps='PTHREADPOOL cmocka',
>+                  ldflags='-Wl,--wrap=pthread_create',
>+                  enabled=bld.env.WITH_PTHREADPOOL and bld.env['HAVE_LDWRAP'],
>+                  install=False)
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index ead8902..c20ff17 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -57,6 +57,9 @@ finally:
> have_libarchive = ("HAVE_LIBARCHIVE" in config_hash)
> have_linux_kernel_oplocks = ("HAVE_KERNEL_OPLOCKS_LINUX" in config_hash)
> have_inotify = ("HAVE_INOTIFY" in config_hash)
>+have_ldwrap = ("HAVE_LDWRAP" in config_hash)
>+with_pthreadpool = ("WITH_PTHREADPOOL" in config_hash)
>+
> 
> plantestsuite("samba3.blackbox.success", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/test_success.sh")])
> plantestsuite("samba3.blackbox.failure", "nt4_dc:local", [os.path.join(samba3srcdir, "script/tests/test_failure.sh")])
>@@ -325,6 +328,10 @@ plantestsuite(
>     "samba3.pthreadpool", "nt4_dc",
>     [os.path.join(samba3srcdir, "script/tests/test_pthreadpool.sh")])
> 
>+if with_pthreadpool and have_ldwrap:
>+    plantestsuite("samba3.pthreadpool_cmocka", "nt4_dc",
>+                  [os.path.join(bindir(), "pthreadpooltest_cmocka")])
>+
> plantestsuite("samba3.async_req", "nt4_dc",
>               [os.path.join(samba3srcdir, "script/tests/test_async_req.sh")])
> 
>-- 
>1.8.3.1
>

Flags:

vl: review+

Actions: View

Attachments on bug 13170: 13852 | 13853 | 13855