From 3407dba7adc8dfb6fd26f11e72f894ab33a55c1c Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Tue, 5 Jun 2018 12:54:06 +0400 Subject: MDEV-16371 - Fix for memory write order inversion my_atomic implementation for interruptable waits. --- include/my_pthread.h | 1 - mysys/my_init.c | 3 +- mysys/my_thr_init.c | 123 +++++++++++++++++++++++++++++++++++---------------- mysys/mysys_priv.h | 2 +- 4 files changed, 87 insertions(+), 42 deletions(-) diff --git a/include/my_pthread.h b/include/my_pthread.h index c9cbb27375a..8f17c1e616c 100644 --- a/include/my_pthread.h +++ b/include/my_pthread.h @@ -705,7 +705,6 @@ struct st_my_thread_var { int thr_errno; mysql_cond_t suspend; - mysql_mutex_t mutex; mysql_cond_t *current_cond; pthread_t pthread_self; my_thread_id id, dbug_id; diff --git a/mysys/my_init.c b/mysys/my_init.c index 972750da0ff..11047ef45d1 100644 --- a/mysys/my_init.c +++ b/mysys/my_init.c @@ -479,7 +479,7 @@ PSI_mutex_key key_LOCK_localtime_r; PSI_mutex_key key_BITMAP_mutex, key_IO_CACHE_append_buffer_lock, key_IO_CACHE_SHARE_mutex, key_KEY_CACHE_cache_lock, key_LOCK_alarm, key_LOCK_timer, - key_my_thread_var_mutex, key_THR_LOCK_charset, key_THR_LOCK_heap, + key_THR_LOCK_charset, key_THR_LOCK_heap, key_THR_LOCK_lock, key_THR_LOCK_malloc, key_THR_LOCK_mutex, key_THR_LOCK_myisam, key_THR_LOCK_net, key_THR_LOCK_open, key_THR_LOCK_threads, @@ -499,7 +499,6 @@ static PSI_mutex_info all_mysys_mutexes[]= { &key_KEY_CACHE_cache_lock, "KEY_CACHE::cache_lock", 0}, { &key_LOCK_alarm, "LOCK_alarm", PSI_FLAG_GLOBAL}, { &key_LOCK_timer, "LOCK_timer", PSI_FLAG_GLOBAL}, - { &key_my_thread_var_mutex, "my_thread_var::mutex", 0}, { &key_THR_LOCK_charset, "THR_LOCK_charset", PSI_FLAG_GLOBAL}, { &key_THR_LOCK_heap, "THR_LOCK_heap", PSI_FLAG_GLOBAL}, { &key_THR_LOCK_lock, "THR_LOCK_lock", PSI_FLAG_GLOBAL}, diff --git a/mysys/my_thr_init.c b/mysys/my_thr_init.c index 696355075ba..369d566b123 100644 --- a/mysys/my_thr_init.c +++ b/mysys/my_thr_init.c @@ -23,6 +23,7 @@ #include #include #include +#include pthread_key(struct st_my_thread_var*, THR_KEY_mysys); mysql_mutex_t THR_LOCK_malloc, THR_LOCK_open, @@ -107,13 +108,11 @@ void my_thread_destroy_internal_mutex(void) static void my_thread_init_thr_mutex(struct st_my_thread_var *var) { - mysql_mutex_init(key_my_thread_var_mutex, &var->mutex, MY_MUTEX_INIT_FAST); mysql_cond_init(key_my_thread_var_suspend, &var->suspend, NULL); } static void my_thread_destory_thr_mutex(struct st_my_thread_var *var) { - mysql_mutex_destroy(&var->mutex); mysql_cond_destroy(&var->suspend); } @@ -452,58 +451,109 @@ safe_mutex_t **my_thread_var_mutex_in_use() } -static int before_interruptable_wait(struct st_my_thread_var *thread_var, - mysql_cond_t *cond, mysql_mutex_t *mutex) -{ - mysql_mutex_lock(&thread_var->mutex); - if (unlikely(thread_var->abort)) - { - mysql_mutex_unlock(&thread_var->mutex); - return 0; - } - thread_var->current_cond= cond; - mysql_mutex_unlock(&thread_var->mutex); - return 1; -} +/** + Starts interruptable wait for condition variable to be signalled. + Concurrent thread may interrupt condition variable wait by calling + my_thread_interrupt_wait(). -static void after_interruptable_wait(struct st_my_thread_var *thread_var) -{ - mysql_mutex_lock(&thread_var->mutex); - thread_var->current_cond= 0; - mysql_mutex_unlock(&thread_var->mutex); -} + Condition variable wait is interruptable when thread_var->current_cond != 0. + + No wait is performed if thread_var->abort is 1. + Implementation is based on atomic operations with MY_MEMORY_ORDER_SEQ_CST. + This strong memory order is required to address famous load after store + problem: thread_var->current_cond store must happen before thread_var->abort + load. + + @sa mysql_cond_wait() + @sa pthread_cond_wait() + @sa my_thread_interruptable_timedwait() + @sa my_thread_interrupt_wait() +*/ void my_thread_interruptable_wait( struct st_my_thread_var *thread_var, mysql_cond_t *cond, mysql_mutex_t *mutex) { - if (before_interruptable_wait(thread_var, cond, mutex)) - { + my_atomic_storeptr_explicit(&thread_var->current_cond, cond, + MY_MEMORY_ORDER_SEQ_CST); + if (likely(!my_atomic_load32_explicit(&thread_var->abort, + MY_MEMORY_ORDER_SEQ_CST))) mysql_cond_wait(cond, mutex); - after_interruptable_wait(thread_var); - } + my_atomic_storeptr_explicit(&thread_var->current_cond, 0, + MY_MEMORY_ORDER_SEQ_CST); } +/** + Starts interruptable timedwait for condition variable to be signalled. + + @sa mysql_cond_timedwait() + @sa pthread_cond_timedwait() + @sa my_thread_interruptable_wait() + @sa my_thread_interrupt_wait() +*/ + int my_thread_interruptable_timedwait( struct st_my_thread_var *thread_var, mysql_cond_t *cond, mysql_mutex_t *mutex, struct timespec *wait_timeout) { - if (before_interruptable_wait(thread_var, cond, mutex)) - { - int rc= mysql_cond_timedwait(cond, mutex, wait_timeout); - after_interruptable_wait(thread_var); - return rc; - } - return 0; + int rc= 0; + my_atomic_storeptr_explicit(&thread_var->current_cond, cond, + MY_MEMORY_ORDER_SEQ_CST); + if (likely(!my_atomic_load32_explicit(&thread_var->abort, + MY_MEMORY_ORDER_SEQ_CST))) + + rc= mysql_cond_timedwait(cond, mutex, wait_timeout); + my_atomic_storeptr_explicit(&thread_var->current_cond, 0, + MY_MEMORY_ORDER_SEQ_CST); + return rc; } +/** + Interrupts condition variable wait in concurrent thread started by + my_thread_interruptable_wait() or my_thread_interruptable_timedwait(). + + Caller must protect thread_var against concurrent deinitalisation or + destruction. E.g. as of this writing thd->mysys_var is intended to be + protected by thd->LOCK_thd_kill. + + For some non-obvious reason we don't set `thread_var->abort= 1` for + delayed insert threads (and sometimes for system threads) in certain + cases. This behaviour was preserved, although it doesn't look alright. + + Compared to previous implementation, this one guarantees that condition + variable wait (if there was any) in concurrent thread is interrupted when + this function returns. + + Subsequent attempts to start new wait in concurrent thread will cause + my_thread_interruptable_wait() and my_thread_interruptable_timedwait() + to return immediately until thread_var->abort is 1. + + Compared to previous implementation, we don't limit execution time for + this function as synchronisation protocol is now trivial and concurrent + thread doesn't do anything expensive while it is in "interruptable wait" + state. + + OTOH concurrent thread has to re-acquire corresponding mutex. If this mutex + deadlocks, my_thread_interrupt_wait() would get stuck too. + + Implementation is based on atomic operations with MY_MEMORY_ORDER_SEQ_CST. + This strong memory order is required to address famous load after store + problem: thread_var->abort store must happen before thread_var->current_cond + load. + + @sa my_thread_interrupt_wait() + @sa my_thread_interrupt_timedwait() + @sa mysql_cond_broadcast() + @sa pthread_cond_broadcast() +*/ + void my_thread_interrupt_wait(struct st_my_thread_var *thread_var, my_bool do_abort) { @@ -512,17 +562,14 @@ void my_thread_interrupt_wait(struct st_my_thread_var *thread_var, if (!thread_var) return; - mysql_mutex_lock(&thread_var->mutex); if (do_abort) // Don't abort locks - thread_var->abort= 1; - while ((cond= thread_var->current_cond)) + my_atomic_store32_explicit(&thread_var->abort, 1, MY_MEMORY_ORDER_SEQ_CST); + while ((cond= my_atomic_loadptr_explicit(&thread_var->current_cond, + MY_MEMORY_ORDER_SEQ_CST))) { - mysql_mutex_unlock(&thread_var->mutex); mysql_cond_broadcast(cond); LF_BACKOFF(); - mysql_mutex_lock(&thread_var->mutex); } - mysql_mutex_unlock(&thread_var->mutex); } diff --git a/mysys/mysys_priv.h b/mysys/mysys_priv.h index 38a75120303..f7f854212ca 100644 --- a/mysys/mysys_priv.h +++ b/mysys/mysys_priv.h @@ -44,7 +44,7 @@ extern PSI_mutex_key key_LOCK_localtime_r; extern PSI_mutex_key key_BITMAP_mutex, key_IO_CACHE_append_buffer_lock, key_IO_CACHE_SHARE_mutex, key_KEY_CACHE_cache_lock, key_LOCK_alarm, - key_my_thread_var_mutex, key_THR_LOCK_charset, key_THR_LOCK_heap, + key_THR_LOCK_charset, key_THR_LOCK_heap, key_THR_LOCK_lock, key_THR_LOCK_malloc, key_THR_LOCK_mutex, key_THR_LOCK_myisam, key_THR_LOCK_net, key_THR_LOCK_open, key_THR_LOCK_threads, key_LOCK_uuid_generator, -- cgit v1.2.1