From cae065f0165ccf9f5fd6d5f373b16d74e9260c38 Mon Sep 17 00:00:00 2001 From: Matthias Maier Date: Fri, 12 Jun 2020 15:53:31 -0500 Subject: [PATCH] Thread/Task: Fix Schmidt's double checking We have to use a std::atomic_bool instead of a bool to ensure proper memory fencing, see [1] for an in-detail discussion. [1] https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/ --- include/deal.II/base/thread_management.h | 30 ++++++++++++++++++++---- 1 file changed, 25 insertions(+), 5 deletions(-) diff --git a/include/deal.II/base/thread_management.h b/include/deal.II/base/thread_management.h index a3d92a2dfa..2d25c1d9ff 100644 --- a/include/deal.II/base/thread_management.h +++ b/include/deal.II/base/thread_management.h @@ -24,6 +24,7 @@ # include # include +# include # include # include # include @@ -429,9 +430,9 @@ namespace Threads std::shared_ptr> ret_val; /** - * A bool variable that is initially false, is set to true when a new - * thread is started, and is set back to false once join() has been - * called. + * An atomic bool variable that is initially false, is set to true + * when a new thread is started, and is set back to false once join() + * has been called. * * We use this variable to make sure we can call join() twice on the * same thread. For some reason, the C++ standard library throws a @@ -458,8 +459,12 @@ namespace Threads * thread. Neither does `pthread_join` appear to have this requirement any * more. Consequently, we can in fact join from different threads and * we test this in base/thread_validity_07. + * + * @note The reason why we need to use an std::atomic_bool is + * discussed in detail in the documentation of + * Task::task_has_finished. */ - bool thread_is_active; + std::atomic_bool thread_is_active; /** * Mutex guarding access to the previous variable. @@ -1255,8 +1260,23 @@ namespace Threads /** * A boolean indicating whether the task in question has finished. + * + * @note We are using a `std::atomic_bool` here because we have + * to make sure that concurrent reads and stores between threads are + * properly synchronized, and that sequential reads on a given thread + * are not reordered or optimized away. A std::atomic [1] achieves + * this because (if not otherwise annotated) reads and stores to the + * boolean are subject to the std::memory_order_seq_cst memory + * ordering [2]. This ensures that Schmidt's double checking does + * indeed work. For additional information (and a potentially more + * efficient implementation) see [3]. + * + * [1] https://en.cppreference.com/w/cpp/atomic/atomic + * [2] https://en.cppreference.com/w/cpp/atomic/memory_order + * [3] + * https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/ */ - bool task_has_finished; + std::atomic_bool task_has_finished; /** * The place where the returned value is moved to once the std::future -- 2.39.5