From fc20c8de910af56d3f1841bcf0bfa57b439d3be5 Mon Sep 17 00:00:00 2001 From: Yibo Cai Date: Tue, 24 Sep 2019 10:13:05 +0000 Subject: [PATCH 3/3] rwlock: refine lock->waiters with C++11 atomics lock->waiters is an integer(used as bool actually) to tell the lock holder that some threads are sleeping and waiting for lock released. The lock holder must check it and signal a conditional variable to wake them up. Memory order is important to make sure sleeping threads will be woken up in time. The problem can be simplified as below code: // Initial state: lockword = 0; waiters = 0; // Thread1 // Thread2 waiters = 1; lockword = 1; // free the lock mb(); mb(); if (lockword == 0) { if (waiters == 1) { sleep(); waiters = 0; } mb(); wakeup(); // wakeup thread1 } To make sure thread2 calls wakeup() if thread1 calls sleep(), full memory barrier is required for both thread, so the ordering of "waiters" and "lockword" is preserved. Besides, memory barrier is required after thread2 resets "waiters" to make sure it happens before calling wakeup(). Another instance of thread1 may set "waiters" at the same time thread2 calls wakeup(), without the barrier, that "waiters" flag may be cleared accidentally. Actual code is more complex. Instead of direct load/store instructions, TTAS is used for atomicity and to prevent cache line store contentions. x86 ignores acquire/release options, the lock/xchg instructions imply memory barrier. Arm respects acquire/release options and generates instructions with necessary memory order. // Thread1 waiters.cmpxchg(0, 1, acquire); if (lockword.cmpxchg(1, 0, acquire) == 0) { // try lock sleep(); } // Thread2 lockword.cmpxchg(0, 1, release); // unlock fence(acquire); if (waiters.load(relaxed) == 1) { waiters.cmpxchg(1, 0, acquire); wakeup(); } Change-Id: Id091cce87c64af7488121e988fca25aafe344e24 --- storage/innobase/include/sync0rw.h | 2 +- storage/innobase/include/sync0rw.ic | 12 +++++++----- storage/innobase/sync/sync0arr.cc | 3 ++- storage/innobase/sync/sync0rw.cc | 2 +- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/storage/innobase/include/sync0rw.h b/storage/innobase/include/sync0rw.h index e5db35b..65775a5 100644 --- a/storage/innobase/include/sync0rw.h +++ b/storage/innobase/include/sync0rw.h @@ -561,7 +561,7 @@ struct rw_lock_t os_atomic_t lock_word; /** 1: there are waiters */ - volatile ulint waiters; + os_atomic_t waiters; /** Default value FALSE which means the lock is non-recursive. The value is typically set to TRUE making normal rw_locks recursive. diff --git a/storage/innobase/include/sync0rw.ic b/storage/innobase/include/sync0rw.ic index 0be1204..f641a49 100644 --- a/storage/innobase/include/sync0rw.ic +++ b/storage/innobase/include/sync0rw.ic @@ -68,7 +68,7 @@ void rw_lock_remove_debug_info(rw_lock_t *lock, /*!< in: rw-lock */ UNIV_INLINE ulint rw_lock_get_waiters(const rw_lock_t *lock) /*!< in: rw-lock */ { - return (lock->waiters); + return (lock->waiters.load(order_relaxed)); } /** Sets lock->waiters to 1. It is not an error if lock->waiters is already @@ -78,7 +78,8 @@ UNIV_INLINE void rw_lock_set_waiter_flag(rw_lock_t *lock) /*!< in/out: rw-lock */ { #ifdef INNODB_RW_LOCKS_USE_ATOMICS - (void)os_compare_and_swap_ulint(&lock->waiters, 0, 1); + ulint v = 0; + lock->waiters.compare_exchange_strong(v, 1, order_acquire); #else /* INNODB_RW_LOCKS_USE_ATOMICS */ lock->waiters = 1; os_wmb; @@ -92,7 +93,8 @@ UNIV_INLINE void rw_lock_reset_waiter_flag(rw_lock_t *lock) /*!< in/out: rw-lock */ { #ifdef INNODB_RW_LOCKS_USE_ATOMICS - (void)os_compare_and_swap_ulint(&lock->waiters, 1, 0); + ulint v = 1; + lock->waiters.compare_exchange_strong(v, 0, order_acquire); #else /* INNODB_RW_LOCKS_USE_ATOMICS */ lock->waiters = 0; os_wmb; @@ -510,7 +512,7 @@ void rw_lock_x_unlock_func( We do not need to signal wait_ex waiters, since they cannot exist when there is a writer. */ std::atomic_thread_fence(order_acquire); - if (lock->waiters) { + if (lock->waiters.load(order_relaxed)) { rw_lock_reset_waiter_flag(lock); os_event_set(lock->event); sync_array_object_signalled(); @@ -560,7 +562,7 @@ void rw_lock_sx_unlock_func( since they cannot exist when there is an sx-lock holder. */ std::atomic_thread_fence(order_acquire); - if (lock->waiters) { + if (lock->waiters.load(order_relaxed)) { rw_lock_reset_waiter_flag(lock); os_event_set(lock->event); sync_array_object_signalled(); diff --git a/storage/innobase/sync/sync0arr.cc b/storage/innobase/sync/sync0arr.cc index e096e2f..73f8dbe 100644 --- a/storage/innobase/sync/sync0arr.cc +++ b/storage/innobase/sync/sync0arr.cc @@ -544,7 +544,8 @@ static void sync_array_cell_print(FILE *file, /*!< in: file where to print */ ", lock_word: %lx\n" "Last time read locked in file %s line %lu\n" "Last time write locked in file %s line %lu\n", - rw_lock_get_reader_count(rwlock), rwlock->waiters, + rw_lock_get_reader_count(rwlock), + rwlock->waiters.load(order_relaxed), static_cast(rwlock->lock_word.load(order_relaxed)), innobase_basename(rwlock->last_s_file_name), static_cast(rwlock->last_s_line), rwlock->last_x_file_name, diff --git a/storage/innobase/sync/sync0rw.cc b/storage/innobase/sync/sync0rw.cc index 4b50541..bd9be5e 100644 --- a/storage/innobase/sync/sync0rw.cc +++ b/storage/innobase/sync/sync0rw.cc @@ -222,7 +222,7 @@ void rw_lock_create_func( #endif /* INNODB_RW_LOCKS_USE_ATOMICS */ lock->lock_word.store(X_LOCK_DECR, order_relaxed); - lock->waiters = 0; + lock->waiters.store(0, order_relaxed); /* We set this value to signify that lock->writer_thread contains garbage at initialization and cannot be used for -- 2.7.4