From 3885f80f7d04453a36ff5dc6bbebbf2c981de662 Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Mon, 28 Dec 2015 13:02:12 -0500 Subject: [PATCH 1/5] A test now shows the problem --- portability/toku_pthread.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/portability/toku_pthread.h b/portability/toku_pthread.h index 84c277362..bdb0f700b 100644 --- a/portability/toku_pthread.h +++ b/portability/toku_pthread.h @@ -41,6 +41,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. #include #include #include +#include #include "toku_assert.h" @@ -246,6 +247,7 @@ toku_cond_destroy(toku_cond_t *cond) { static inline void toku_cond_wait(toku_cond_t *cond, toku_mutex_t *mutex) { #if TOKU_PTHREAD_DEBUG + if (random()%2) return; // sometimes we simply spuriously wakeup to see if we are using the cond wait correctly. invariant(mutex->locked); mutex->locked = false; mutex->owner = 0; From ddc82c4a966a56094a42302fadc4b4ab5a50d2c8 Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Mon, 28 Dec 2015 14:10:55 -0500 Subject: [PATCH 2/5] Documentation --- portability/toku_pthread.h | 26 +++++++++++++++++++++++++- util/frwlock.cc | 21 +++++++++++++++++++-- util/frwlock.h | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 78 insertions(+), 3 deletions(-) diff --git a/portability/toku_pthread.h b/portability/toku_pthread.h index bdb0f700b..2e404794c 100644 --- a/portability/toku_pthread.h +++ b/portability/toku_pthread.h @@ -247,8 +247,32 @@ toku_cond_destroy(toku_cond_t *cond) { static inline void toku_cond_wait(toku_cond_t *cond, toku_mutex_t *mutex) { #if TOKU_PTHREAD_DEBUG - if (random()%2) return; // sometimes we simply spuriously wakeup to see if we are using the cond wait correctly. invariant(mutex->locked); + + switch (random()%3) { + case 0: + // Sometimes we return immediately. This is one case of a + // spurious wakeup, which cond_wait is allowed to do. We + // want to exercise the users of this code to make sure + // they handle it. + return; + case 1: + // Sometimes we unlock the mutex and relock it. This is + // another case of a spurious wakeup. It's different from + // case 0, in that it gives someone else a chance to run, + // and then we can spuriously wake up after they have + // possibly released the mutex without setting the + // condition to the one that our caller expects. + toku_mutex_unlock(mutex); + toku_mutex_lock(mutex); + return; + default: + // The default is to simply call pthread_cond_wait, (which + // may still spuriosly wake up, but won't do it reliably + // enough to test the caller's use of the condition. + break; + } + mutex->locked = false; mutex->owner = 0; #endif diff --git a/util/frwlock.cc b/util/frwlock.cc index ce1e33e85..e86862a1f 100644 --- a/util/frwlock.cc +++ b/util/frwlock.cc @@ -60,6 +60,7 @@ void frwlock::init(toku_mutex_t *const mutex) { m_num_want_read = 0; m_num_signaled_readers = 0; m_num_expensive_want_write = 0; + m_waking_cond = nullptr; toku_cond_init(&m_wait_read, nullptr); m_queue_item_read.cond = &m_wait_read; @@ -127,7 +128,11 @@ void frwlock::write_lock(bool expensive) { m_current_writer_tid = get_local_tid(); m_blocking_writer_context_id = toku_thread_get_context()->get_id(); } - toku_cond_wait(&cond, m_mutex); + while (m_waking_cond != &cond) { + // Wait until *this* cond variable is woken up. + toku_cond_wait(&cond, m_mutex); + } + m_waking_cond = nullptr; toku_cond_destroy(&cond); // Now it's our turn. @@ -186,7 +191,16 @@ void frwlock::read_lock(void) { // Wait for our turn. ++m_num_want_read; - toku_cond_wait(&m_wait_read, m_mutex); + while (m_num_writers > 0 || m_num_want_read == 0 || m_num_signaled_readers == 0) { + // Must put this in a loop, since we could get a spurious + // wakeup from toku_cond_wait. A spurious wakeup could + // result in an unfair scheduling (since we might get + // woken up just before a writer ahead of us gets the + // write lock), but it appears that at least we will + // correctly obtain a read lock in this situation. + toku_cond_wait(&m_wait_read, m_mutex); + } + m_waking_cond = nullptr; // Now it's our turn. paranoid_invariant_zero(m_num_writers); @@ -218,6 +232,7 @@ void frwlock::maybe_signal_next_writer(void) { paranoid_invariant(cond != &m_wait_read); // Grant write lock to waiting writer. paranoid_invariant(m_num_want_write > 0); + m_waking_cond = cond; toku_cond_signal(cond); } } @@ -257,11 +272,13 @@ void frwlock::maybe_signal_or_broadcast_next(void) { m_num_signaled_readers = m_num_want_read; m_wait_read_is_in_queue = false; m_read_wait_expensive = false; + m_waking_cond = nullptr; toku_cond_broadcast(cond); } else { // Grant write lock to waiting writer. paranoid_invariant(m_num_want_write > 0); + m_waking_cond = cond; toku_cond_signal(cond); } } diff --git a/util/frwlock.h b/util/frwlock.h index 52b98a8d7..ba5b7ed71 100644 --- a/util/frwlock.h +++ b/util/frwlock.h @@ -51,6 +51,11 @@ namespace toku { class frwlock { public: + // This is a fair readers-writers lock. It has one extra property + // beyond a vanilla fair rw lock: it can tell you whether waiting + // on a lock may be "expensive". This is done by requiring anyone + // who obtains a write lock to say whether it's expensive. + void init(toku_mutex_t *const mutex); void deinit(void); @@ -85,13 +90,42 @@ class frwlock { void maybe_signal_or_broadcast_next(void); void maybe_signal_next_writer(void); + // Discussion of the frwlock (especially of the condition + // variables). This implementation seems needlessly complex, so + // it needs some documentation here. We implements a fair + // readers-writer lock by keeping a queue. We employ condition + // variables to wake up threads when they get to the front of the + // queue. + // + // For a read: If there is a writer (holding the lock, or in the + // queue), then the reader must wait (that makes it fair). + // The way it waits: If there are any other readers in the queue, put this reader into the queue. + // We then wait on the condition variable. + // Bradley says: This looks pretty buggy. A reader could end up in the queue and then get spuriously woken up + // I think this code is still not safe against spurious wakeups. + toku_mutex_t *m_mutex; + // How many readers currently hold the lock (any nonnegative + // number). uint32_t m_num_readers; + + // How many writers currently hold the lock (must be zero or one). uint32_t m_num_writers; + + // Number of writers in the queue. uint32_t m_num_want_write; + + // Number of readers in the queue. uint32_t m_num_want_read; + // Number of readers that we have woken up (but they haven't + // woken up yet and, for example, incremented the m_num_readers.) uint32_t m_num_signaled_readers; + + // When we signal a writer to wake up, it must be waiting on this + // cond variable to avoid spurious wakeups. + toku_cond_t *m_waking_cond; + // number of writers waiting that are expensive // MUST be < m_num_want_write uint32_t m_num_expensive_want_write; From 28e20979df320590251c5c1ba32df08583ba45bd Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Mon, 28 Dec 2015 15:17:08 -0500 Subject: [PATCH 3/5] Simpler fair rwlock implementation. --- util/frwlock.cc | 176 +++++++++++------------------------------------- util/frwlock.h | 79 +++++++--------------- 2 files changed, 64 insertions(+), 191 deletions(-) diff --git a/util/frwlock.cc b/util/frwlock.cc index e86862a1f..f7346a358 100644 --- a/util/frwlock.cc +++ b/util/frwlock.cc @@ -52,57 +52,22 @@ static int get_local_tid() { } void frwlock::init(toku_mutex_t *const mutex) { + m_queue = new std::queue>(); m_mutex = mutex; - m_num_readers = 0; m_num_writers = 0; - m_num_want_write = 0; m_num_want_read = 0; - m_num_signaled_readers = 0; + m_num_want_write = 0; m_num_expensive_want_write = 0; - m_waking_cond = nullptr; - - toku_cond_init(&m_wait_read, nullptr); - m_queue_item_read.cond = &m_wait_read; - m_queue_item_read.next = nullptr; - m_wait_read_is_in_queue = false; - m_current_writer_expensive = false; - m_read_wait_expensive = false; + + // Do we really want these? m_current_writer_tid = -1; m_blocking_writer_context_id = CTX_INVALID; - - m_wait_head = nullptr; - m_wait_tail = nullptr; } void frwlock::deinit(void) { - toku_cond_destroy(&m_wait_read); -} - -bool frwlock::queue_is_empty(void) const { - return m_wait_head == nullptr; -} - -void frwlock::enq_item(queue_item *const item) { - paranoid_invariant_null(item->next); - if (m_wait_tail != nullptr) { - m_wait_tail->next = item; - } else { - paranoid_invariant_null(m_wait_head); - m_wait_head = item; - } - m_wait_tail = item; -} - -toku_cond_t *frwlock::deq_item(void) { - paranoid_invariant_notnull(m_wait_head); - paranoid_invariant_notnull(m_wait_tail); - queue_item *item = m_wait_head; - m_wait_head = m_wait_head->next; - if (m_wait_tail == item) { - m_wait_tail = nullptr; - } - return item->cond; + assert(m_queue->empty()); + delete m_queue; } // Prerequisite: Holds m_mutex. @@ -113,14 +78,12 @@ void frwlock::write_lock(bool expensive) { } toku_cond_t cond = TOKU_COND_INITIALIZER; - queue_item item = { .cond = &cond, .next = nullptr }; - this->enq_item(&item); - - // Wait for our turn. + m_queue->push(std::pair(&cond, false)); ++m_num_want_write; if (expensive) { ++m_num_expensive_want_write; } + if (m_num_writers == 0 && m_num_want_write == 1) { // We are the first to want a write lock. No new readers can get the lock. // Set our thread id and context for proper instrumentation. @@ -128,18 +91,17 @@ void frwlock::write_lock(bool expensive) { m_current_writer_tid = get_local_tid(); m_blocking_writer_context_id = toku_thread_get_context()->get_id(); } - while (m_waking_cond != &cond) { - // Wait until *this* cond variable is woken up. + while (m_num_writers || m_num_readers || m_queue->front().first != &cond) { + // Wait until this cond variable is woken up. We could get a spurious wakeup. toku_cond_wait(&cond, m_mutex); } - m_waking_cond = nullptr; + m_queue->pop(); toku_cond_destroy(&cond); // Now it's our turn. paranoid_invariant(m_num_want_write > 0); paranoid_invariant_zero(m_num_readers); paranoid_invariant_zero(m_num_writers); - paranoid_invariant_zero(m_num_signaled_readers); // Not waiting anymore; grab the lock. --m_num_want_write; @@ -154,7 +116,7 @@ void frwlock::write_lock(bool expensive) { bool frwlock::try_write_lock(bool expensive) { toku_mutex_assert_locked(m_mutex); - if (m_num_readers > 0 || m_num_writers > 0 || m_num_signaled_readers > 0 || m_num_want_write > 0) { + if (m_num_readers > 0 || m_num_writers > 0 || !m_queue->empty()) { return false; } // No one holds the lock. Grant the write lock. @@ -169,54 +131,32 @@ bool frwlock::try_write_lock(bool expensive) { void frwlock::read_lock(void) { toku_mutex_assert_locked(m_mutex); - if (m_num_writers > 0 || m_num_want_write > 0) { - if (!m_wait_read_is_in_queue) { - // Throw the read cond_t onto the queue. - paranoid_invariant(m_num_signaled_readers == m_num_want_read); - m_queue_item_read.next = nullptr; - this->enq_item(&m_queue_item_read); - m_wait_read_is_in_queue = true; - paranoid_invariant(!m_read_wait_expensive); - m_read_wait_expensive = ( - m_current_writer_expensive || - (m_num_expensive_want_write > 0) - ); - } - - // Note this contention event in engine status. + if (this->try_read_lock()) return; + + toku_cond_t cond = TOKU_COND_INITIALIZER; + m_queue->push(std::pair(&cond, true)); + ++m_num_want_read; + while (m_num_writers || m_queue->front().first != &cond) { toku_context_note_frwlock_contention( toku_thread_get_context()->get_id(), - m_blocking_writer_context_id - ); - - // Wait for our turn. - ++m_num_want_read; - while (m_num_writers > 0 || m_num_want_read == 0 || m_num_signaled_readers == 0) { - // Must put this in a loop, since we could get a spurious - // wakeup from toku_cond_wait. A spurious wakeup could - // result in an unfair scheduling (since we might get - // woken up just before a writer ahead of us gets the - // write lock), but it appears that at least we will - // correctly obtain a read lock in this situation. - toku_cond_wait(&m_wait_read, m_mutex); - } - m_waking_cond = nullptr; - - // Now it's our turn. - paranoid_invariant_zero(m_num_writers); - paranoid_invariant(m_num_want_read > 0); - paranoid_invariant(m_num_signaled_readers > 0); - - // Not waiting anymore; grab the lock. - --m_num_want_read; - --m_num_signaled_readers; + m_blocking_writer_context_id); + toku_cond_wait(&cond, m_mutex); } + m_queue->pop(); + toku_cond_destroy(&cond); + paranoid_invariant_zero(m_num_writers); + paranoid_invariant(m_num_want_read > 0); + --m_num_want_read; ++m_num_readers; + if (!m_queue->empty() && m_queue->front().second) { + // The next guy is a reader, so wake him up too. + toku_cond_signal(m_queue->front().first); + } } bool frwlock::try_read_lock(void) { toku_mutex_assert_locked(m_mutex); - if (m_num_writers > 0 || m_num_want_write > 0) { + if (m_num_writers > 0 || !m_queue->empty()) { return false; } // No writer holds the lock. @@ -226,61 +166,19 @@ bool frwlock::try_read_lock(void) { return true; } -void frwlock::maybe_signal_next_writer(void) { - if (m_num_want_write > 0 && m_num_signaled_readers == 0 && m_num_readers == 0) { - toku_cond_t *cond = this->deq_item(); - paranoid_invariant(cond != &m_wait_read); - // Grant write lock to waiting writer. - paranoid_invariant(m_num_want_write > 0); - m_waking_cond = cond; - toku_cond_signal(cond); - } -} - void frwlock::read_unlock(void) { toku_mutex_assert_locked(m_mutex); paranoid_invariant(m_num_writers == 0); paranoid_invariant(m_num_readers > 0); --m_num_readers; - this->maybe_signal_next_writer(); + if (m_num_readers == 0 && !m_queue->empty()) { + toku_cond_signal(m_queue->front().first); + } } bool frwlock::read_lock_is_expensive(void) { toku_mutex_assert_locked(m_mutex); - if (m_wait_read_is_in_queue) { - return m_read_wait_expensive; - } - else { - return m_current_writer_expensive || (m_num_expensive_want_write > 0); - } -} - - -void frwlock::maybe_signal_or_broadcast_next(void) { - paranoid_invariant(m_num_signaled_readers == 0); - - if (this->queue_is_empty()) { - paranoid_invariant(m_num_want_write == 0); - paranoid_invariant(m_num_want_read == 0); - return; - } - toku_cond_t *cond = this->deq_item(); - if (cond == &m_wait_read) { - // Grant read locks to all waiting readers - paranoid_invariant(m_wait_read_is_in_queue); - paranoid_invariant(m_num_want_read > 0); - m_num_signaled_readers = m_num_want_read; - m_wait_read_is_in_queue = false; - m_read_wait_expensive = false; - m_waking_cond = nullptr; - toku_cond_broadcast(cond); - } - else { - // Grant write lock to waiting writer. - paranoid_invariant(m_num_want_write > 0); - m_waking_cond = cond; - toku_cond_signal(cond); - } + return m_num_expensive_want_write > 0 || m_current_writer_expensive; } void frwlock::write_unlock(void) { @@ -290,11 +188,13 @@ void frwlock::write_unlock(void) { m_current_writer_expensive = false; m_current_writer_tid = -1; m_blocking_writer_context_id = CTX_INVALID; - this->maybe_signal_or_broadcast_next(); + if (!m_queue->empty()) { + toku_cond_signal(m_queue->front().first); + } } bool frwlock::write_lock_is_expensive(void) { toku_mutex_assert_locked(m_mutex); - return (m_num_expensive_want_write > 0) || (m_current_writer_expensive); + return (m_num_expensive_want_write > 0) || m_current_writer_expensive; } diff --git a/util/frwlock.h b/util/frwlock.h index ba5b7ed71..fe2d4c334 100644 --- a/util/frwlock.h +++ b/util/frwlock.h @@ -43,6 +43,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. #include #include #include +#include //TODO: update comment, this is from rwlock.h @@ -71,84 +72,56 @@ class frwlock { // returns true if acquiring a read lock will be expensive bool read_lock_is_expensive(void); + // How many threads are holding or waiting on the lock ? uint32_t users(void) const; + + // How many are waiting on the lock? uint32_t blocked_users(void) const; + + // How many writer therads are holding the lock (0 or 1)? uint32_t writers(void) const; + + // How many writers are waiting on the lock? uint32_t blocked_writers(void) const; + + // How many readers currently hold the lock? uint32_t readers(void) const; + + // How many readers currently wait on the lock? uint32_t blocked_readers(void) const; private: - struct queue_item { - toku_cond_t *cond; - struct queue_item *next; - }; - - bool queue_is_empty(void) const; - void enq_item(queue_item *const item); - toku_cond_t *deq_item(void); - void maybe_signal_or_broadcast_next(void); - void maybe_signal_next_writer(void); - - // Discussion of the frwlock (especially of the condition - // variables). This implementation seems needlessly complex, so - // it needs some documentation here. We implements a fair - // readers-writer lock by keeping a queue. We employ condition - // variables to wake up threads when they get to the front of the - // queue. - // - // For a read: If there is a writer (holding the lock, or in the - // queue), then the reader must wait (that makes it fair). - // The way it waits: If there are any other readers in the queue, put this reader into the queue. - // We then wait on the condition variable. - // Bradley says: This looks pretty buggy. A reader could end up in the queue and then get spuriously woken up - // I think this code is still not safe against spurious wakeups. + // the pair is the condition variable and true for read, false for write + std::queue> *m_queue; toku_mutex_t *m_mutex; - // How many readers currently hold the lock (any nonnegative - // number). + // How many readers hold the lock? uint32_t m_num_readers; - - // How many writers currently hold the lock (must be zero or one). + + // How many writers hold the lock? uint32_t m_num_writers; - // Number of writers in the queue. - uint32_t m_num_want_write; - - // Number of readers in the queue. + // How many readers in the queue? uint32_t m_num_want_read; - // Number of readers that we have woken up (but they haven't - // woken up yet and, for example, incremented the m_num_readers.) - uint32_t m_num_signaled_readers; - // When we signal a writer to wake up, it must be waiting on this - // cond variable to avoid spurious wakeups. - toku_cond_t *m_waking_cond; + // How many writers in the queue? + uint32_t m_num_want_write; - // number of writers waiting that are expensive + // Number of writers that are expensive (not including the writer that holds the lock, if any) // MUST be < m_num_want_write uint32_t m_num_expensive_want_write; - // bool that states if the current writer is expensive - // if there is no current writer, then is false - bool m_current_writer_expensive; - // bool that states if waiting for a read - // is expensive - // if there are currently no waiting readers, then set to false - bool m_read_wait_expensive; + + // Is the current writer expensive (we must store this separately + // from m_num_expensive_want_write) + bool m_current_writer_expensive; + // thread-id of the current writer int m_current_writer_tid; // context id describing the context of the current writer blocking // new readers (either because this writer holds the write lock or // is the first to want the write lock). context_id m_blocking_writer_context_id; - - toku_cond_t m_wait_read; - queue_item m_queue_item_read; - bool m_wait_read_is_in_queue; - - queue_item *m_wait_head; - queue_item *m_wait_tail; }; ENSURE_POD(frwlock); From 6a721bbac23ea96f11d78c33ef116249d410a517 Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Mon, 28 Dec 2015 16:12:46 -0500 Subject: [PATCH 4/5] Clean up the frwlock code --- util/frwlock.cc | 80 +++++++++++++++++++++-------------------------- util/frwlock.h | 83 ++++++++++++++++++++++++++++++++++--------------- 2 files changed, 93 insertions(+), 70 deletions(-) diff --git a/util/frwlock.cc b/util/frwlock.cc index f7346a358..6b8b4cbf8 100644 --- a/util/frwlock.cc +++ b/util/frwlock.cc @@ -52,15 +52,12 @@ static int get_local_tid() { } void frwlock::init(toku_mutex_t *const mutex) { - m_queue = new std::queue>(); + m_queue = new std::queue(); m_mutex = mutex; m_num_readers = 0; m_num_writers = 0; - m_num_want_read = 0; - m_num_want_write = 0; m_num_expensive_want_write = 0; - // Do we really want these? m_current_writer_tid = -1; m_blocking_writer_context_id = CTX_INVALID; } @@ -78,20 +75,14 @@ void frwlock::write_lock(bool expensive) { } toku_cond_t cond = TOKU_COND_INITIALIZER; - m_queue->push(std::pair(&cond, false)); - ++m_num_want_write; + m_queue->push(frwlock_queueitem(&cond, + get_local_tid(), + toku_thread_get_context()->get_id())); if (expensive) { ++m_num_expensive_want_write; } - if (m_num_writers == 0 && m_num_want_write == 1) { - // We are the first to want a write lock. No new readers can get the lock. - // Set our thread id and context for proper instrumentation. - // see: toku_context_note_frwlock_contention() - m_current_writer_tid = get_local_tid(); - m_blocking_writer_context_id = toku_thread_get_context()->get_id(); - } - while (m_num_writers || m_num_readers || m_queue->front().first != &cond) { + while (m_num_writers || m_num_readers || m_queue->front().m_cond != &cond) { // Wait until this cond variable is woken up. We could get a spurious wakeup. toku_cond_wait(&cond, m_mutex); } @@ -99,12 +90,10 @@ void frwlock::write_lock(bool expensive) { toku_cond_destroy(&cond); // Now it's our turn. - paranoid_invariant(m_num_want_write > 0); paranoid_invariant_zero(m_num_readers); paranoid_invariant_zero(m_num_writers); // Not waiting anymore; grab the lock. - --m_num_want_write; if (expensive) { --m_num_expensive_want_write; } @@ -120,8 +109,6 @@ bool frwlock::try_write_lock(bool expensive) { return false; } // No one holds the lock. Grant the write lock. - paranoid_invariant_zero(m_num_want_write); - paranoid_invariant_zero(m_num_want_read); m_num_writers = 1; m_current_writer_expensive = expensive; m_current_writer_tid = get_local_tid(); @@ -134,9 +121,10 @@ void frwlock::read_lock(void) { if (this->try_read_lock()) return; toku_cond_t cond = TOKU_COND_INITIALIZER; - m_queue->push(std::pair(&cond, true)); - ++m_num_want_read; - while (m_num_writers || m_queue->front().first != &cond) { + m_queue->push(frwlock_queueitem(&cond)); + while (m_num_writers || m_queue->front().m_cond != &cond) { + // We know the queue isn't empty (since we are in it), so it's + // safe to all m_queue->front(). toku_context_note_frwlock_contention( toku_thread_get_context()->get_id(), m_blocking_writer_context_id); @@ -145,12 +133,17 @@ void frwlock::read_lock(void) { m_queue->pop(); toku_cond_destroy(&cond); paranoid_invariant_zero(m_num_writers); - paranoid_invariant(m_num_want_read > 0); - --m_num_want_read; ++m_num_readers; - if (!m_queue->empty() && m_queue->front().second) { - // The next guy is a reader, so wake him up too. - toku_cond_signal(m_queue->front().first); + if (!m_queue->empty()) { + const frwlock_queueitem &qi = m_queue->front(); + if (qi.m_is_read) { + // The next guy is a reader, so wake him up too. + toku_cond_signal(qi.m_cond); + } else { + // The next guy is a writer, so he's the one whose context should be put. + m_current_writer_tid = qi.m_writer_tid; + m_blocking_writer_context_id = qi.m_writer_context_id; + } } } @@ -172,7 +165,7 @@ void frwlock::read_unlock(void) { paranoid_invariant(m_num_readers > 0); --m_num_readers; if (m_num_readers == 0 && !m_queue->empty()) { - toku_cond_signal(m_queue->front().first); + toku_cond_signal(m_queue->front().m_cond); } } @@ -189,7 +182,7 @@ void frwlock::write_unlock(void) { m_current_writer_tid = -1; m_blocking_writer_context_id = CTX_INVALID; if (!m_queue->empty()) { - toku_cond_signal(m_queue->front().first); + toku_cond_signal(m_queue->front().m_cond); } } bool frwlock::write_lock_is_expensive(void) { @@ -197,32 +190,29 @@ bool frwlock::write_lock_is_expensive(void) { return (m_num_expensive_want_write > 0) || m_current_writer_expensive; } - uint32_t frwlock::users(void) const { toku_mutex_assert_locked(m_mutex); - return m_num_readers + m_num_writers + m_num_want_read + m_num_want_write; -} -uint32_t frwlock::blocked_users(void) const { - toku_mutex_assert_locked(m_mutex); - return m_num_want_read + m_num_want_write; + return m_num_readers + m_num_writers + m_queue->size(); } + uint32_t frwlock::writers(void) const { - // this is sometimes called as "assert(lock->writers())" when we - // assume we have the write lock. if that's the assumption, we may - // not own the mutex, so we don't assert_locked here - return m_num_writers; -} -uint32_t frwlock::blocked_writers(void) const { + // The following comment was found, and if true means the code is + // incorrect (we would at least have to make m_num_writers be an + // atomic variable). Fortunately, the following comment appears + // to be completely out of date. + // + // this is sometimes called as "assert(lock->writers())" when we + // assume we have the write lock. if that's the assumption, we may + // not own the mutex, so we don't assert_locked here + // + // So we go ahead and assert it. toku_mutex_assert_locked(m_mutex); - return m_num_want_write; + return m_num_writers; } + uint32_t frwlock::readers(void) const { toku_mutex_assert_locked(m_mutex); return m_num_readers; } -uint32_t frwlock::blocked_readers(void) const { - toku_mutex_assert_locked(m_mutex); - return m_num_want_read; -} } // namespace toku diff --git a/util/frwlock.h b/util/frwlock.h index fe2d4c334..d73aae5d9 100644 --- a/util/frwlock.h +++ b/util/frwlock.h @@ -49,13 +49,60 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. namespace toku { +class frwlock_queueitem { + public: + toku_cond_t *m_cond; + bool m_is_read; + int m_writer_tid; + context_id m_writer_context_id; + frwlock_queueitem(toku_cond_t *cond) + : m_cond(cond) + , m_is_read(true) + , m_writer_tid(-1) + , m_writer_context_id(CTX_INVALID) + {} + frwlock_queueitem(toku_cond_t *cond, int writer_tid, context_id writer_context_id) + : m_cond(cond) + , m_is_read(false) + , m_writer_tid(writer_tid) + , m_writer_context_id(writer_context_id) + {} +}; + class frwlock { public: - // This is a fair readers-writers lock. It has one extra property - // beyond a vanilla fair rw lock: it can tell you whether waiting - // on a lock may be "expensive". This is done by requiring anyone - // who obtains a write lock to say whether it's expensive. + // This is a fair readers-writers lock. It has two extra properties + // beyond a vanilla fair rw lock: + // + // 1) It can tell you whether waiting on a lock may be + // "expensive". This is done by requiring anyone who obtains + // a write lock to say whether it's expensive, and keeping + // track of whether any expensive write request is either + // holding the lock or in the queue waiting. + // + // 2) It records the context and thread id of the writer who is + // currently blocking any other thread from getting the lock. + // It does this by recording the context and thread id when a + // writer gets the lock, or when a reader gets the lock and + // the next item in the queue is a writer. + // + // The implementation employs a std::queue of frwlock_queueitems + // (which contain a condition variable, a bool indicatin whether + // the request is a reader or a writer, and the threadid and + // context of the requesting thread. + // + // When a reader or writer tries, and cannot get, the lock, it + // places a condition variable into the queue. + // + // When a reader releases a lock, it checks to see if there are + // any other readers still holding the lock, and if not, it + // signals the next item in the queue (which is responsible for + // fremoving itself from the queue, and if it is a reader, for + // signaling the next item in the queue if it is a reader). + // + // When a writer releases a lock, it signals the next item in the + // queue. void init(toku_mutex_t *const mutex); void deinit(void); @@ -72,27 +119,18 @@ class frwlock { // returns true if acquiring a read lock will be expensive bool read_lock_is_expensive(void); - // How many threads are holding or waiting on the lock ? + // How many threads are holding or waiting on the lock ? (You must hold the lock to call this.) uint32_t users(void) const; - // How many are waiting on the lock? - uint32_t blocked_users(void) const; - - // How many writer therads are holding the lock (0 or 1)? + // How many writer therads are holding the lock (0 or 1)? (You must hold the lock to hold this). uint32_t writers(void) const; - // How many writers are waiting on the lock? - uint32_t blocked_writers(void) const; - - // How many readers currently hold the lock? + // How many readers currently hold the lock? (You must hold the lock to call this.) uint32_t readers(void) const; - // How many readers currently wait on the lock? - uint32_t blocked_readers(void) const; - private: // the pair is the condition variable and true for read, false for write - std::queue> *m_queue; + std::queue *m_queue; toku_mutex_t *m_mutex; @@ -102,14 +140,8 @@ class frwlock { // How many writers hold the lock? uint32_t m_num_writers; - // How many readers in the queue? - uint32_t m_num_want_read; - - // How many writers in the queue? - uint32_t m_num_want_write; - // Number of writers that are expensive (not including the writer that holds the lock, if any) - // MUST be < m_num_want_write + // MUST be < the number in the queue that want to write. uint32_t m_num_expensive_want_write; // Is the current writer expensive (we must store this separately @@ -118,9 +150,10 @@ class frwlock { // thread-id of the current writer int m_current_writer_tid; + // context id describing the context of the current writer blocking // new readers (either because this writer holds the write lock or - // is the first to want the write lock). + // is one of the ones that wants the write lock). context_id m_blocking_writer_context_id; }; From 2a967ea942d42ae2f0fc77d6588a1e793635bac7 Mon Sep 17 00:00:00 2001 From: "Bradley C. Kuszmaul" Date: Mon, 28 Dec 2015 16:28:17 -0500 Subject: [PATCH 5/5] Make the frwlock::writers() call work without a lock being held. --- util/frwlock.cc | 13 +++++-------- util/frwlock.h | 5 +++-- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/util/frwlock.cc b/util/frwlock.cc index 6b8b4cbf8..bbb1816aa 100644 --- a/util/frwlock.cc +++ b/util/frwlock.cc @@ -198,15 +198,12 @@ uint32_t frwlock::users(void) const { uint32_t frwlock::writers(void) const { // The following comment was found, and if true means the code is // incorrect (we would at least have to make m_num_writers be an - // atomic variable). Fortunately, the following comment appears - // to be completely out of date. - // - // this is sometimes called as "assert(lock->writers())" when we - // assume we have the write lock. if that's the assumption, we may - // not own the mutex, so we don't assert_locked here + // atomic variable). Unfortunately, such code actually exists, so I made the m_num_writer be atomic. + + // This is sometimes called as "assert(lock->writers())" when we + // assume we have the write lock. if that's the assumption, we may + // not own the mutex, so we don't assert_locked here // - // So we go ahead and assert it. - toku_mutex_assert_locked(m_mutex); return m_num_writers; } diff --git a/util/frwlock.h b/util/frwlock.h index d73aae5d9..cc34f1a6d 100644 --- a/util/frwlock.h +++ b/util/frwlock.h @@ -44,6 +44,7 @@ Copyright (c) 2006, 2015, Percona and/or its affiliates. All rights reserved. #include #include #include +#include //TODO: update comment, this is from rwlock.h @@ -122,7 +123,7 @@ class frwlock { // How many threads are holding or waiting on the lock ? (You must hold the lock to call this.) uint32_t users(void) const; - // How many writer therads are holding the lock (0 or 1)? (You must hold the lock to hold this). + // How many writer therads are holding the lock (0 or 1)? (You need not hold the lock to call this.) uint32_t writers(void) const; // How many readers currently hold the lock? (You must hold the lock to call this.) @@ -138,7 +139,7 @@ class frwlock { uint32_t m_num_readers; // How many writers hold the lock? - uint32_t m_num_writers; + std::atomic m_num_writers; // this is accessed without a lock, so we make it atomic. // Number of writers that are expensive (not including the writer that holds the lock, if any) // MUST be < the number in the queue that want to write.