From 36c081bd45a46ba7dc58cc431660ecbd4507ade8 Mon Sep 17 00:00:00 2001 From: Christof Schmitt 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 Reviewed-by: Volker Lendecke (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 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 Reviewed-by: Volker Lendecke (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 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 Reviewed-by: Andreas Schneider (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 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 Autobuild-User(master): Christof Schmitt 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 . + */ + +#include +#include +#include +#include +#include + +#include +#include +#include + +#include + +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