diff --git a/sql/log.cc b/sql/log.cc index f965f5963b3..590c062351c 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -6756,29 +6756,23 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) DBUG_ASSERT(entry != NULL); } - /* Now we need to clear the wakeup_subsequent_commits_running flags. */ + /* + Now we need to clear the wakeup_subsequent_commits_running flags. + + We need a full memory barrier between walking the list above, and clearing + the flag wakeup_subsequent_commits_running below. This barrier is needed + to ensure that no other thread will start to modify the list pointers + before we are done traversing the list. + + But wait_for_commit::wakeup(), which was called above for any other thread + that might modify the list in parallel, does a full memory barrier already + (it locks a mutex). + */ if (list) { for (;;) { - if (list->wakeup_subsequent_commits_running) - { - /* - ToDo: We should not need a full lock/unlock of LOCK_wait_commit - here. All we need is a single (full) memory barrier, to ensure that - the reads of the list above are not reordered with the write of - wakeup_subsequent_commits_running, or with the writes to the list - from other threads that is allowed to happen after - wakeup_subsequent_commits_running has been set to false. - - We do not currently have explicit memory barrier primitives in the - source tree, but if we get them the below mysql_mutex_lock() could - be replaced with a full memory barrier just before the loop. - */ - mysql_mutex_lock(&list->LOCK_wait_commit); - list->wakeup_subsequent_commits_running= false; - mysql_mutex_unlock(&list->LOCK_wait_commit); - } + list->wakeup_subsequent_commits_running= false; if (list == last) break; list= list->next_subsequent_commit; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 9a638947257..4c433638a77 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -5624,6 +5624,10 @@ wait_for_commit::wakeup() Otherwise we would need to somehow ensure that they were done waking up before we could allow this THD to be destroyed, which would be annoying and unnecessary. + + Note that wakeup_subsequent_commits2() depends on this function being a + full memory barrier (it is, because it takes a mutex lock). + */ mysql_mutex_lock(&LOCK_wait_commit); waiting_for_commit= false; @@ -5755,21 +5759,15 @@ wait_for_commit::wakeup_subsequent_commits2() } /* - ToDo: We should not need a full lock/unlock of LOCK_wait_commit here. All - we need is a (full) memory barrier, to ensure that the reads of the list - above are not reordered with the write of - wakeup_subsequent_commits_running, or with the writes to the list from - other threads that is allowed to happen after - wakeup_subsequent_commits_running has been set to false. + We need a full memory barrier between walking the list above, and clearing + the flag wakeup_subsequent_commits_running below. This barrier is needed + to ensure that no other thread will start to modify the list pointers + before we are done traversing the list. - We do not currently have explicit memory barrier primitives in the source - tree, but if we get them the below mysql_mutex_lock() could be replaced - with a full memory barrier. It is probably not important, the lock is not - contented and will likely be in the CPU cache since we took it just before. + But wait_for_commit::wakeup() does a full memory barrier already (it locks + a mutex), so no extra explicit barrier is needed here. */ - mysql_mutex_lock(&LOCK_wait_commit); wakeup_subsequent_commits_running= false; - mysql_mutex_unlock(&LOCK_wait_commit); }