From 1a1749e38c774ce3a3493da2410b19ebe71eccb5 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Thu, 24 Nov 2016 18:18:00 +0400 Subject: MDEV-11296 - InnoDB stalls under OLTP RW on P8 Threads may fall asleep forever while acquiring InnoDB rw-lock on Power8. This regression was introduced along with InnoDB atomic operations fixes. The problem was that proper memory order wasn't enforced between "writers" store and "lock_word" load: my_atomic_store32((int32*) &lock->waiters, 1); ... local_lock_word = lock->lock_word; Locking protocol is such that store to "writers" must be completed before load of "lock_word". my_atomic_store32() was expected to enforce memory order because it issues strongest MY_MEMORY_ORDER_SEQ_CST memory barrier. According to C11: - any operation with this memory order is both an acquire operation and a release operation - for atomic store order must be one of memory_order_relaxed memory_order_release or memory_order_seq_cst. Otherwise the behavior is undefined. That is it doesn't say explicitly that this expectation is wrong, but there are indications that acquire (which is actually supposed to guarantee memory order in this case) may not be issued along with MY_MEMORY_ORDER_SEQ_CST. A good fix for this is to encode waiters into lock_word, but it is a bit too intrusive. Instead we change atomic store to atomic fetch-and-store, which does memory load and is guaranteed to issue acquire. --- storage/innobase/sync/sync0rw.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'storage/innobase/sync') diff --git a/storage/innobase/sync/sync0rw.cc b/storage/innobase/sync/sync0rw.cc index 77a9102112f..5f617ae22f5 100644 --- a/storage/innobase/sync/sync0rw.cc +++ b/storage/innobase/sync/sync0rw.cc @@ -358,7 +358,7 @@ lock_loop: /* Set waiters before checking lock_word to ensure wake-up signal is sent. This may lead to some unnecessary signals. */ - my_atomic_store32((int32*) &lock->waiters, 1); + my_atomic_fas32((int32*) &lock->waiters, 1); if (rw_lock_s_lock_low(lock, pass, file_name, line)) { @@ -750,7 +750,7 @@ lock_loop: /* Waiters must be set before checking lock_word, to ensure signal is sent. This could lead to a few unnecessary wake-up signals. */ - my_atomic_store32((int32*) &lock->waiters, 1); + my_atomic_fas32((int32*) &lock->waiters, 1); if (rw_lock_x_lock_low(lock, pass, file_name, line)) { sync_array_free_cell(sync_arr, cell); @@ -855,7 +855,7 @@ lock_loop: /* Waiters must be set before checking lock_word, to ensure signal is sent. This could lead to a few unnecessary wake-up signals. */ - my_atomic_store32((int32*) &lock->waiters, 1); + my_atomic_fas32((int32*) &lock->waiters, 1); if (rw_lock_sx_lock_low(lock, pass, file_name, line)) { -- cgit v1.2.1