From bcabbe2c92d5da3f536a19d1d7f920c08139b3cb Mon Sep 17 00:00:00 2001 From: Norman Feske Date: Fri, 16 Nov 2012 13:53:37 +0100 Subject: [PATCH] Add 'Thread_base::join()' Using the new 'join()' function, the caller can explicitly block for the completion of the thread's 'entry()' function. The test case for this feature can be found at 'os/src/test/thread_join'. For hybrid Linux/Genode programs, the 'Thread_base::join()' does not map directly to 'pthread_join'. The latter function gets already called by the destructor of 'Thread_base'. According to the documentation, subsequent calls of 'pthread_join' for one thread may result in undefined behaviour. So we use a 'Genode::Lock' on this platform, which is in line with the other platforms. Related to #194, #501 --- base-codezero/src/base/thread/thread_start.cc | 1 + base-fiasco/src/core/thread_start.cc | 1 + base-foc/src/base/thread/thread.cc | 9 ++- base-foc/src/base/thread/thread_bootstrap.cc | 1 + base-hw/src/base/thread_support.cc | 1 + base-linux/src/base/thread/thread_linux.cc | 12 ++- base-linux/src/core/thread_linux.cc | 4 +- base-linux/src/platform/linux_syscalls.h | 2 +- base-linux/src/platform/lx_hybrid.cc | 41 ++++++---- base-mb/src/base/thread/thread_start.cc | 1 + base-mb/src/core/thread_roottask.cc | 1 + base-nova/src/base/thread/thread_nova.cc | 1 + base-okl4/src/core/thread_start.cc | 1 + base-pistachio/src/core/thread_start.cc | 1 + base/include/base/thread.h | 13 ++++ base/src/base/thread/thread.cc | 9 ++- base/src/base/thread/thread_start.cc | 1 + os/run/thread_join.run | 39 ++++++++++ os/src/test/thread_join/main.cc | 74 +++++++++++++++++++ os/src/test/thread_join/target.mk | 3 + tool/autopilot.list | 1 + 21 files changed, 193 insertions(+), 24 deletions(-) create mode 100644 os/run/thread_join.run create mode 100644 os/src/test/thread_join/main.cc create mode 100644 os/src/test/thread_join/target.mk diff --git a/base-codezero/src/base/thread/thread_start.cc b/base-codezero/src/base/thread/thread_start.cc index 4b3c233e6e..19f0c6b012 100644 --- a/base-codezero/src/base/thread/thread_start.cc +++ b/base-codezero/src/base/thread/thread_start.cc @@ -30,6 +30,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/base-fiasco/src/core/thread_start.cc b/base-fiasco/src/core/thread_start.cc index 8ea4482272..39a4a02c2b 100644 --- a/base-fiasco/src/core/thread_start.cc +++ b/base-fiasco/src/core/thread_start.cc @@ -26,6 +26,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); sleep_forever(); } diff --git a/base-foc/src/base/thread/thread.cc b/base-foc/src/base/thread/thread.cc index fa26ab1e77..0c84dffdea 100644 --- a/base-foc/src/base/thread/thread.cc +++ b/base-foc/src/base/thread/thread.cc @@ -189,10 +189,17 @@ Thread_base *Thread_base::myself() { } +void Thread_base::join() +{ + _join_lock.lock(); +} + + Thread_base::Thread_base(const char *name, size_t stack_size) : _list_element(this), - _context(_alloc_context(stack_size)) + _context(_alloc_context(stack_size)), + _join_lock(Lock::LOCKED) { strncpy(_context->name, name, sizeof(_context->name)); _init_platform_thread(); diff --git a/base-foc/src/base/thread/thread_bootstrap.cc b/base-foc/src/base/thread/thread_bootstrap.cc index 3342c889d0..f2997eb907 100644 --- a/base-foc/src/base/thread/thread_bootstrap.cc +++ b/base-foc/src/base/thread/thread_bootstrap.cc @@ -24,6 +24,7 @@ void Genode::Thread_base::_thread_start() Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); sleep_forever(); } diff --git a/base-hw/src/base/thread_support.cc b/base-hw/src/base/thread_support.cc index 4b8df4e216..12b4dad74a 100644 --- a/base-hw/src/base/thread_support.cc +++ b/base-hw/src/base/thread_support.cc @@ -42,6 +42,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/base-linux/src/base/thread/thread_linux.cc b/base-linux/src/base/thread/thread_linux.cc index 7f72dd1212..312a58194a 100644 --- a/base-linux/src/base/thread/thread_linux.cc +++ b/base-linux/src/base/thread/thread_linux.cc @@ -40,7 +40,7 @@ static Lock &startup_lock() static void thread_exit_signal_handler(int) { lx_exit(0); } -static void thread_start(void *arg) +void Thread_base::_thread_start() { /* * Set signal handler such that canceled system calls get not @@ -53,7 +53,7 @@ static void thread_start(void *arg) */ lx_sigaction(LX_SIGCHLD, (void (*)(int))1); - Thread_base *thread = (Thread_base *)arg; + Thread_base * const thread = Thread_base::myself(); /* inform core about the new thread and process ID of the new thread */ Linux_cpu_session *cpu = dynamic_cast(env()->cpu_session()); @@ -63,7 +63,11 @@ static void thread_start(void *arg) /* wakeup 'start' function */ startup_lock().unlock(); - Thread_base::myself()->entry(); + thread->entry(); + + /* unblock caller of 'join()' */ + thread->_join_lock.unlock(); + sleep_forever(); } @@ -125,7 +129,7 @@ void Thread_base::start() /* align initial stack to 16 byte boundary */ void *thread_sp = (void *)((addr_t)(_context->stack) & ~0xf); - _tid.tid = lx_create_thread(thread_start, thread_sp, this); + _tid.tid = lx_create_thread(Thread_base::_thread_start, thread_sp, this); _tid.pid = lx_getpid(); /* wait until the 'thread_start' function got entered */ diff --git a/base-linux/src/core/thread_linux.cc b/base-linux/src/core/thread_linux.cc index 8306ace179..5259ffea8c 100644 --- a/base-linux/src/core/thread_linux.cc +++ b/base-linux/src/core/thread_linux.cc @@ -24,7 +24,7 @@ using namespace Genode; static void empty_signal_handler(int) { } -static void thread_start(void *) +void Thread_base::_thread_start() { /* * Set signal handler such that canceled system calls get not @@ -52,7 +52,7 @@ void Thread_base::start() { /* align initial stack to 16 byte boundary */ void *thread_sp = (void *)((addr_t)(_context->stack) & ~0xf); - _tid.tid = lx_create_thread(thread_start, thread_sp, this); + _tid.tid = lx_create_thread(Thread_base::_thread_start, thread_sp, this); _tid.pid = lx_getpid(); } diff --git a/base-linux/src/platform/linux_syscalls.h b/base-linux/src/platform/linux_syscalls.h index 0b5832e20f..7e5d0ff98c 100644 --- a/base-linux/src/platform/linux_syscalls.h +++ b/base-linux/src/platform/linux_syscalls.h @@ -306,7 +306,7 @@ inline int lx_tgkill(int pid, int tid, int signal) } -inline int lx_create_thread(void (*entry)(void *), void *stack, void *arg) +inline int lx_create_thread(void (*entry)(), void *stack, void *arg) { int flags = CLONE_VM | CLONE_FS | CLONE_FILES | CLONE_SIGHAND | CLONE_THREAD | CLONE_SYSVSEM; diff --git a/base-linux/src/platform/lx_hybrid.cc b/base-linux/src/platform/lx_hybrid.cc index 3646bca7f4..f379b22129 100644 --- a/base-linux/src/platform/lx_hybrid.cc +++ b/base-linux/src/platform/lx_hybrid.cc @@ -130,16 +130,26 @@ namespace Genode { struct Thread_meta_data { + /** + * Lock with the initial state set to LOCKED + */ + struct Barrier : Lock { Barrier() : Lock(Lock::LOCKED) { } }; + /** * Used to block the constructor until the new thread has initialized * 'id' */ - Lock construct_lock; + Barrier construct_lock; /** * Used to block the new thread until 'start' is called */ - Lock start_lock; + Barrier start_lock; + + /** + * Used to block the 'join()' function until the 'entry()' is done + */ + Barrier join_lock; /** * Filled out by 'thread_start' function in the context of the new @@ -155,13 +165,9 @@ namespace Genode { /** * Constructor * - * \param thread_base associated 'Thread_base' object + * \param thread associated 'Thread_base' object */ - Thread_meta_data(Thread_base *thread_base) - : - construct_lock(Lock::LOCKED), start_lock(Lock::LOCKED), - thread_base(thread_base) - { } + Thread_meta_data(Thread_base *thread) : thread_base(thread) { } }; } @@ -224,6 +230,8 @@ static void *thread_start(void *arg) meta_data->start_lock.lock(); Thread_base::myself()->entry(); + + meta_data->join_lock.unlock(); return 0; } @@ -286,8 +294,15 @@ void Thread_base::start() } +void Thread_base::join() +{ + _tid.meta_data->join_lock.lock(); +} + + Thread_base::Thread_base(const char *name, size_t stack_size) -: _list_element(this) +: + _list_element(this) { _tid.meta_data = new (env()->heap()) Thread_meta_data(this); @@ -325,13 +340,9 @@ void Thread_base::cancel_blocking() Thread_base::~Thread_base() { - { - int const ret = pthread_cancel(_tid.meta_data->pt); - if (ret) - PWRN("pthread_cancel unexpectedly returned with %d", ret); - } + bool const needs_join = (pthread_cancel(_tid.meta_data->pt) == 0); - { + if (needs_join) { int const ret = pthread_join(_tid.meta_data->pt, 0); if (ret) PWRN("pthread_join unexpectedly returned with %d (errno=%d)", diff --git a/base-mb/src/base/thread/thread_start.cc b/base-mb/src/base/thread/thread_start.cc index 1ef41f9009..00249d44c8 100644 --- a/base-mb/src/base/thread/thread_start.cc +++ b/base-mb/src/base/thread/thread_start.cc @@ -27,6 +27,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/base-mb/src/core/thread_roottask.cc b/base-mb/src/core/thread_roottask.cc index 6b96f75f32..1d48c157ae 100755 --- a/base-mb/src/core/thread_roottask.cc +++ b/base-mb/src/core/thread_roottask.cc @@ -58,6 +58,7 @@ void Thread_base::_thread_start() PDBG("Thread returned, tid=%i, pid=%i", myself()->tid(), Roottask::PROTECTION_ID); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/base-nova/src/base/thread/thread_nova.cc b/base-nova/src/base/thread/thread_nova.cc index f28d8ee1f6..135cbc442e 100644 --- a/base-nova/src/base/thread/thread_nova.cc +++ b/base-nova/src/base/thread/thread_nova.cc @@ -37,6 +37,7 @@ using namespace Genode; void Thread_base::_thread_start() { Genode::Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/base-okl4/src/core/thread_start.cc b/base-okl4/src/core/thread_start.cc index 1d94f67501..15e2647f0d 100644 --- a/base-okl4/src/core/thread_start.cc +++ b/base-okl4/src/core/thread_start.cc @@ -26,6 +26,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); sleep_forever(); } diff --git a/base-pistachio/src/core/thread_start.cc b/base-pistachio/src/core/thread_start.cc index 8ea4482272..39a4a02c2b 100644 --- a/base-pistachio/src/core/thread_start.cc +++ b/base-pistachio/src/core/thread_start.cc @@ -26,6 +26,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); sleep_forever(); } diff --git a/base/include/base/thread.h b/base/include/base/thread.h index 0fc1780b21..b154ff0b82 100644 --- a/base/include/base/thread.h +++ b/base/include/base/thread.h @@ -254,6 +254,11 @@ namespace Genode { */ Native_thread _tid; + /** + * Lock used for synchronizing the finalization of the thread + */ + Genode::Lock _join_lock; + public: /** @@ -335,6 +340,14 @@ namespace Genode { * 0 when called by the main thread. */ Native_utcb *utcb(); + + /** + * Block until the thread leaves the 'entry' function + * + * Join must not be called more than once. Subsequent calls have + * undefined behaviour. + */ + void join(); }; diff --git a/base/src/base/thread/thread.cc b/base/src/base/thread/thread.cc index d8fa548605..89b77cfff3 100644 --- a/base/src/base/thread/thread.cc +++ b/base/src/base/thread/thread.cc @@ -195,10 +195,17 @@ Thread_base *Thread_base::myself() } +void Thread_base::join() +{ + _join_lock.lock(); +} + + Thread_base::Thread_base(const char *name, size_t stack_size) : _list_element(this), - _context(_alloc_context(stack_size)) + _context(_alloc_context(stack_size)), + _join_lock(Lock::LOCKED) { strncpy(_context->name, name, sizeof(_context->name)); _init_platform_thread(); diff --git a/base/src/base/thread/thread_start.cc b/base/src/base/thread/thread_start.cc index 918a4aadfe..9ccf1d6933 100644 --- a/base/src/base/thread/thread_start.cc +++ b/base/src/base/thread/thread_start.cc @@ -27,6 +27,7 @@ void Thread_base::_thread_start() { Thread_base::myself()->_thread_bootstrap(); Thread_base::myself()->entry(); + Thread_base::myself()->_join_lock.unlock(); Genode::sleep_forever(); } diff --git a/os/run/thread_join.run b/os/run/thread_join.run new file mode 100644 index 0000000000..56439e1e78 --- /dev/null +++ b/os/run/thread_join.run @@ -0,0 +1,39 @@ +build "core init drivers/timer test/thread_join" + +create_boot_directory + +install_config { + + + + + + + + + + + + + + + + + + + + + + + + + +} + +build_boot_image "core init timer test-thread_join" + +append qemu_args "-nographic -m 64" + +run_genode_until {child exited with exit value 0.*} 10 + +puts "Test succeeded" diff --git a/os/src/test/thread_join/main.cc b/os/src/test/thread_join/main.cc new file mode 100644 index 0000000000..1fde397aaa --- /dev/null +++ b/os/src/test/thread_join/main.cc @@ -0,0 +1,74 @@ +/* + * \brief Test for the 'Thread_base::join()' function + * \author Norman Feske + * \date 2012-11-16 + */ + +/* + * Copyright (C) 2012 Genode Labs GmbH + * + * This file is part of the Genode OS framework, which is distributed + * under the terms of the GNU General Public License version 2. + */ + +#include +#include +#include + +using namespace Genode; + + +struct Worker : Genode::Thread<4096> +{ + Timer::Session &timer; + unsigned const result_value; + unsigned volatile result; + + void entry() + { + PLOG("worker thread is up"); + timer.msleep(250); + + PLOG("worker is leaving the entry function with result=%u...", + result_value); + result = result_value; + } + + Worker(Timer::Session &timer, int result_value) + : + timer(timer), result_value(result_value), result(~0) + { + start(); + } +}; + + +/** + * Main program + */ +int main(int, char **) +{ + printf("--- thread join test ---\n"); + + Timer::Connection timer; + + for (unsigned i = 0; i < 10; i++) { + + /* + * A worker thread is created in each iteration. Just before + * leaving the entry function, the worker assigns the result + * to 'Worker::result' variable. By validating this value, + * we determine whether the worker has finished or not. + */ + Worker worker(timer, i); + worker.join(); + + if (worker.result != i) { + PERR("work remains unfinished after 'join()' returned"); + return -1; + } + } + + printf("--- signalling test finished ---\n"); + return 0; +} diff --git a/os/src/test/thread_join/target.mk b/os/src/test/thread_join/target.mk new file mode 100644 index 0000000000..b0f7525069 --- /dev/null +++ b/os/src/test/thread_join/target.mk @@ -0,0 +1,3 @@ +TARGET = test-thread_join +SRC_CC = main.cc +LIBS = cxx env thread diff --git a/tool/autopilot.list b/tool/autopilot.list index 9c36768b95..669857e92e 100644 --- a/tool/autopilot.list +++ b/tool/autopilot.list @@ -15,3 +15,4 @@ lx_hybrid_ctors lx_hybrid_exception lx_hybrid_pthread_ipc moon +thread_join