From 183c51637ae90652418b74588a647583caa604d5 Mon Sep 17 00:00:00 2001 From: Yibo Cai Date: Tue, 24 Sep 2019 02:34:14 +0000 Subject: [PATCH 1/3] rwlock: refine lock->recursive with C++11 atomics Setting lock->recursive publishes valid lock->writer_thread, their ordering must be enforced in both writing and reading side. Current implementation uses wmb and rmb for this purpose. This patch leverages C++11 memory model to refine it with store-release and load-acquire ordering, which is designed for this typical use case and more friendly to weak memory order platforms like Arm. Atomic operations of current code are based on GCC __sync_xxx or __atomic_xxx builtins for Linux and Solaris, and MSVC InterlockedXXX ones for Windows.[1] It's hard to add another set of atomic operations with memory order parameters and keep existing code concise at the same time(without exploding #ifdef). So I'm using C++11 std::atomic, which is supported by all compilers per mysql8.0 source installation prerequisites.[2] Added an os_atomic_t class which wraps std::atomic and does some refinements, it will also be used in later optimizations. [1] https://github.com/mysql/mysql-server/blob/8.0/storage/innobase/include/os0atomic.h [2] https://dev.mysql.com/doc/refman/8.0/en/source-installation-prerequisites.html Change-Id: I7bb8c90c9971979b6e39aa368e76ac77e5ea7428 --- storage/innobase/include/os0atomic-c11.h | 52 ++++++++++++++++++++++++++++++++ storage/innobase/include/os0atomic.h | 2 ++ storage/innobase/include/sync0rw.h | 2 +- storage/innobase/include/sync0rw.ic | 22 +++++--------- storage/innobase/sync/sync0rw.cc | 12 +++----- 5 files changed, 67 insertions(+), 23 deletions(-) create mode 100644 storage/innobase/include/os0atomic-c11.h diff --git a/storage/innobase/include/os0atomic-c11.h b/storage/innobase/include/os0atomic-c11.h new file mode 100644 index 0000000..c81eed7 --- /dev/null +++ b/storage/innobase/include/os0atomic-c11.h @@ -0,0 +1,52 @@ +/***************************************************************************** + +Copyright (c) 2019, Arm Corporation All rights reserved. + +*****************************************************************************/ + +/** @file include/os0atomic-c11.h + This file provides C++11 atomics for finer memory model controlling and + better performance on weak memory order platforms like Arm. + + Created 2019-09-09 Yibo Cai + *******************************************************/ + +#ifndef os0atomic_c11_h +#define os0atomic_c11_h + +#include + +/** Shorter names for C++11 memory order options */ +constexpr auto order_relaxed = std::memory_order_relaxed; +constexpr auto order_consume = std::memory_order_consume; +constexpr auto order_acquire = std::memory_order_acquire; +constexpr auto order_release = std::memory_order_release; +constexpr auto order_acq_rel = std::memory_order_acq_rel; +constexpr auto order_seq_cst = std::memory_order_seq_cst; + +template +struct os_atomic_t : std::atomic { + /** Disable implicit load/store operators as they enforces strongest memory + order. Use member functions with explicit memory order options instead. */ + operator T() const = delete; + T operator=(T arg) = delete; + T operator+=(T arg) = delete; + T operator-=(T arg) = delete; + T operator&=(T arg) = delete; + T operator|=(T arg) = delete; + T operator^=(T arg) = delete; + T operator++() = delete; + T operator--() = delete; + +#ifdef UNIV_DEBUG + /** std::atomic is not copyassignable, which makes rw_lock_t not assignable + as it includes std::atomic members. In some debug test code, this copying is + required. So add this dummy assignment operator for debug build. */ + os_atomic_t& operator=(const os_atomic_t& v) { + *this = v; + return *this; + } +#endif +}; + +#endif /* !os0atomic_c11_h */ diff --git a/storage/innobase/include/os0atomic.h b/storage/innobase/include/os0atomic.h index 51137f3..52989c1 100644 --- a/storage/innobase/include/os0atomic.h +++ b/storage/innobase/include/os0atomic.h @@ -344,4 +344,6 @@ amount to decrement. */ #include "os0atomic.ic" +#include "os0atomic-c11.h" + #endif /* !os0atomic_h */ diff --git a/storage/innobase/include/sync0rw.h b/storage/innobase/include/sync0rw.h index a3169ad..f333487 100644 --- a/storage/innobase/include/sync0rw.h +++ b/storage/innobase/include/sync0rw.h @@ -572,7 +572,7 @@ struct rw_lock_t If this flag is set then writer_thread MUST contain the thread id of the current x-holder or wait-x thread. This flag must be reset in x_unlock functions before incrementing the lock_word */ - volatile bool recursive; + os_atomic_t recursive; /** number of granted SX locks. */ volatile ulint sx_recursive; diff --git a/storage/innobase/include/sync0rw.ic b/storage/innobase/include/sync0rw.ic index bf34238..eb1740b 100644 --- a/storage/innobase/include/sync0rw.ic +++ b/storage/innobase/include/sync0rw.ic @@ -291,26 +291,18 @@ void rw_lock_set_writer_id_and_recursion_flag( os_thread_id_t curr_thread = os_thread_get_curr_id(); #ifdef INNODB_RW_LOCKS_USE_ATOMICS - os_thread_id_t local_thread; - ibool success; - /* Prevent Valgrind warnings about writer_thread being - uninitialized. It does not matter if writer_thread is - uninitialized, because we are comparing writer_thread against - itself, and the operation should always succeed. */ + /* Prevent Valgrind warnings about writer_thread being uninitialized. */ UNIV_MEM_VALID(&lock->writer_thread, sizeof lock->writer_thread); - local_thread = lock->writer_thread; - success = os_compare_and_swap_thread_id(&lock->writer_thread, local_thread, - curr_thread); - ut_a(success); - lock->recursive = recursive; + lock->writer_thread = curr_thread; + lock->recursive.store(recursive, order_release); #else /* INNODB_RW_LOCKS_USE_ATOMICS */ mutex_enter(&lock->mutex); lock->writer_thread = curr_thread; - lock->recursive = recursive; + lock->recursive.store(recursive, order_relaxed); mutex_exit(&lock->mutex); #endif /* INNODB_RW_LOCKS_USE_ATOMICS */ @@ -405,7 +397,7 @@ ibool rw_lock_x_lock_func_nowait( if (success) { rw_lock_set_writer_id_and_recursion_flag(lock, true); - } else if (lock->recursive && + } else if (lock->recursive.load(order_acquire) && os_thread_eq(lock->writer_thread, os_thread_get_curr_id())) { /* Relock: this lock_word modification is safe since no other threads can modify (lock, unlock, or reserve) lock_word while @@ -487,7 +479,7 @@ void rw_lock_x_unlock_func( lock_word. */ if (lock->lock_word == 0) { /* Last caller in a possible recursive chain. */ - lock->recursive = FALSE; + lock->recursive.store(FALSE, order_relaxed); } ut_d(rw_lock_remove_debug_info(lock, pass, RW_LOCK_X)); @@ -541,7 +533,7 @@ void rw_lock_sx_unlock_func( if (lock->sx_recursive == 0) { /* Last caller in a possible recursive chain. */ if (lock->lock_word > 0) { - lock->recursive = FALSE; + lock->recursive.store(FALSE, order_relaxed); UNIV_MEM_INVALID(&lock->writer_thread, sizeof lock->writer_thread); if (rw_lock_lock_word_incr(lock, X_LOCK_HALF_DECR) <= X_LOCK_HALF_DECR) { diff --git a/storage/innobase/sync/sync0rw.cc b/storage/innobase/sync/sync0rw.cc index 840019e..899be12 100644 --- a/storage/innobase/sync/sync0rw.cc +++ b/storage/innobase/sync/sync0rw.cc @@ -227,7 +227,7 @@ void rw_lock_create_func( /* We set this value to signify that lock->writer_thread contains garbage at initialization and cannot be used for recursive x-locking. */ - lock->recursive = FALSE; + lock->recursive.store(FALSE, order_relaxed); lock->sx_recursive = 0; /* Silence Valgrind when UNIV_DEBUG_VALGRIND is not enabled. */ memset((void *)&lock->writer_thread, 0, sizeof lock->writer_thread); @@ -505,7 +505,7 @@ ibool rw_lock_x_lock_low( field is stale or active. As we are going to write our own thread id in that field it must be that the current writer_thread value is not active. */ - ut_a(!lock->recursive); + ut_a(!lock->recursive.load(order_relaxed)); /* Decrement occurred: we are writer or next-writer. */ rw_lock_set_writer_id_and_recursion_flag(lock, !pass); @@ -518,8 +518,7 @@ ibool rw_lock_x_lock_low( bool locked = false; if (!pass) { - bool recursive = lock->recursive; - os_rmb; + bool recursive = lock->recursive.load(order_acquire); os_thread_id_t writer_thread = lock->writer_thread; /* Decrement failed: An X or SX lock is held by either @@ -579,7 +578,7 @@ ibool rw_lock_sx_lock_low( field is stale or active. As we are going to write our own thread id in that field it must be that the current writer_thread value is not active. */ - ut_a(!lock->recursive); + ut_a(!lock->recursive.load(order_relaxed)); /* Decrement occurred: we are the SX lock owner. */ rw_lock_set_writer_id_and_recursion_flag(lock, !pass); @@ -592,8 +591,7 @@ ibool rw_lock_sx_lock_low( bool locked = false; if (!pass) { - bool recursive = lock->recursive; - os_rmb; + bool recursive = lock->recursive.load(order_acquire); os_thread_id_t writer_thread = lock->writer_thread; /* Decrement failed: It already has an X or SX lock by this -- 2.7.4