From 5d5c9322e915e306207647d47bf841af03559039 Mon Sep 17 00:00:00 2001 From: Yilong Li Date: Sat, 19 Jan 2019 00:52:19 -0800 Subject: [PATCH] Moved performance-critical SpinLock::{lock, tryLock, unlock} to the header file ... and changed the implementation to be a Test-And-Test-And-Set (TTAS) Lock. --- src/BackupMasterRecovery.h | 1 + src/InfUdDriverTest.cc | 2 +- src/SpinLock.cc | 93 +++++++++++++------------------------- src/SpinLock.h | 64 ++++++++++++++++++++++++-- src/SpinLockTest.cc | 8 ++-- 5 files changed, 96 insertions(+), 72 deletions(-) diff --git a/src/BackupMasterRecovery.h b/src/BackupMasterRecovery.h index 84369882c..34f20f930 100644 --- a/src/BackupMasterRecovery.h +++ b/src/BackupMasterRecovery.h @@ -16,6 +16,7 @@ #ifndef RAMCLOUD_BACKUPMASTERRECOVERY_H #define RAMCLOUD_BACKUPMASTERRECOVERY_H +#include "Atomic.h" #include "Common.h" #include "BackupStorage.h" #include "Log.h" diff --git a/src/InfUdDriverTest.cc b/src/InfUdDriverTest.cc index 382392b1a..dae928813 100644 --- a/src/InfUdDriverTest.cc +++ b/src/InfUdDriverTest.cc @@ -98,7 +98,7 @@ TEST_F(InfUdDriverTest, gbsOption) { ServiceLocator serverLocator2("basic+infud:gbs=1"); InfUdDriver driver2(&context, &serverLocator2, false); - EXPECT_EQ(4016u, driver2.maxTransmitQueueSize); + EXPECT_EQ(8112u, driver2.maxTransmitQueueSize); Cycles::mockCyclesPerSec = 0; } diff --git a/src/SpinLock.cc b/src/SpinLock.cc index 06f6b313d..3560c2008 100644 --- a/src/SpinLock.cc +++ b/src/SpinLock.cc @@ -13,11 +13,10 @@ * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */ -#include #include -#include "Common.h" #include "Cycles.h" +#include "Logger.h" #include "SpinLock.h" namespace RAMCloud { @@ -70,66 +69,6 @@ SpinLock::~SpinLock() SpinLockTable::allLocks()->erase(this); } -/** - * Acquire the SpinLock; blocks the thread (by continuously polling the lock) - * until the lock has been acquired. - */ -void -SpinLock::lock() -{ - uint64_t startOfContention = 0; - - while (mutex.test_and_set(std::memory_order_acquire)) { - if (startOfContention == 0) { - startOfContention = Cycles::rdtsc(); - if (logWaits) { - RAMCLOUD_TEST_LOG("Waiting on SpinLock"); - } - } else { - uint64_t now = Cycles::rdtsc(); - if (Cycles::toSeconds(now - startOfContention) > 1.0) { - RAMCLOUD_LOG(WARNING, - "%s SpinLock locked for one second; deadlock?", - name.c_str()); - contendedTicks += now - startOfContention; - startOfContention = now; - } - } - } - - if (startOfContention != 0) { - contendedTicks += (Cycles::rdtsc() - startOfContention); - contendedAcquisitions++; - } - acquisitions++; -} - -/** - * Try to acquire the SpinLock; does not block the thread and returns - * immediately. - * - * \return - * True if the lock was successfully acquired, false if it was already - * owned by some other thread. - */ -bool -SpinLock::try_lock() -{ - // test_and_set sets the flag to true and returns the previous value; - // if it's True, someone else is owning the lock. - return !mutex.test_and_set(std::memory_order_acquire); -} - -/** - * Release the SpinLock. The caller must previously have acquired the - * lock with a call to #lock or #try_lock. - */ -void -SpinLock::unlock() -{ - mutex.clear(std::memory_order_release); -} - /** * Change the name of the SpinLock. The name is intended to give some hint as * to the purpose of the lock, where it was declared, etc. @@ -179,4 +118,34 @@ SpinLock::numLocks() return downCast(SpinLockTable::allLocks()->size()); } +/** + * Log a warning if we have been stuck at acquiring the lock for too long; + * intended primarily for debugging. + * + * This method is extracted from SpinLock::lock to avoid having to include + * "Logger.h" in the header file. + * + * \param[out] startOfContention + * Time, in rdtsc ticks, when we first tried to acquire the lock. + */ +void +SpinLock::debugLongWaitAndDeadlock(uint64_t* startOfContention) +{ + if (*startOfContention == 0) { + *startOfContention = Cycles::rdtsc(); + if (logWaits) { + RAMCLOUD_TEST_LOG("Waiting on SpinLock"); + } + } else { + uint64_t now = Cycles::rdtsc(); + if (now >= *startOfContention + uint64_t(Cycles::perSecond())) { + RAMCLOUD_LOG(WARNING, + "%s SpinLock locked for one second; deadlock?", + name.c_str()); + contendedTicks += now - *startOfContention; + *startOfContention = now; + } + } +} + } // namespace RAMCloud diff --git a/src/SpinLock.h b/src/SpinLock.h index 6c16ba74a..e063420f6 100644 --- a/src/SpinLock.h +++ b/src/SpinLock.h @@ -18,8 +18,8 @@ #include #include +#include -#include "Atomic.h" #include "SpinLockStatistics.pb.h" namespace RAMCloud { @@ -39,9 +39,61 @@ class SpinLock { public: explicit SpinLock(string name); virtual ~SpinLock(); - void lock(); - bool try_lock(); - void unlock(); + + /** + * Acquire the SpinLock; blocks the thread (by continuously polling the + * lock) until the lock has been acquired. + */ + inline void + lock() { + uint64_t startOfContention = 0; + do { + // mutex.exchange() always invalidates the cache line mutex resides + // in, regardless of whether it succeeded in updating the value or + // not. To avoid bogus cache invalidation traffic, wait until we + // observe the lock to be free. This technique is usually called the + // Test-And-Test-And-Set (TTAS) optimization. + while (mutex.load(std::memory_order_relaxed)) { + debugLongWaitAndDeadlock(&startOfContention); + } + } while (mutex.exchange(1, std::memory_order_acquire)); + + if (startOfContention != 0) { + contendedTicks += (__rdtsc() - startOfContention); + contendedAcquisitions++; + } + acquisitions++; + } + + /** + * Try to acquire the SpinLock; does not block the thread and returns + * immediately. + * + * \return + * True if the lock was successfully acquired, false if it was already + * owned by some other thread. + */ + inline bool + try_lock() + { + if (mutex.load(std::memory_order_relaxed)) { + return false; + } + // exchange returns the previous value of the variable; if it's true, + // someone else is owning the lock. + return !mutex.exchange(1, std::memory_order_acquire); + } + + /** + * Release the SpinLock. The caller must previously have acquired the + * lock with a call to #lock or #try_lock. + */ + inline void + unlock() + { + mutex.store(0, std::memory_order_release); + } + void setName(string name); static void getStatistics(ProtoBuf::SpinLockStatistics* stats); static int numLocks(); @@ -53,8 +105,10 @@ class SpinLock { typedef std::lock_guard Guard; PRIVATE: + void debugLongWaitAndDeadlock(uint64_t* startOfContention); + /// Implements the lock: False means free, True means locked. - std::atomic_flag mutex; + std::atomic_bool mutex; /// Descriptive name for this SpinLock. Used to identify the purpose of /// the lock, what it protects, where it exists in the codebase, etc. diff --git a/src/SpinLockTest.cc b/src/SpinLockTest.cc index 66049d90d..37ce29b30 100644 --- a/src/SpinLockTest.cc +++ b/src/SpinLockTest.cc @@ -68,7 +68,7 @@ TEST(SpinLockTest, threadBlocks) { // available. std::thread thread(blockingChild, &lock); TestUtil::waitForLog(); - EXPECT_EQ("lock: Waiting on SpinLock", TestLog::get()); + EXPECT_EQ("debugLongWaitAndDeadlock: Waiting on SpinLock", TestLog::get()); // Make sure that the child thread eventually completes once we // release the lock. @@ -130,14 +130,14 @@ TEST(SpinLockTest, printWarning) { // Wait for a thread to block on the lock. std::thread thread(blockingChild, &lock); TestUtil::waitForLog(); - EXPECT_EQ("lock: Waiting on SpinLock", TestLog::get()); + EXPECT_EQ("debugLongWaitAndDeadlock: Waiting on SpinLock", TestLog::get()); // Advance time, make sure that the thread prints a message. TestLog::reset(); Cycles::mockTscValue += ticksPerSecond; TestUtil::waitForLog(); - EXPECT_EQ("lock: SpinLockTest SpinLock locked for one second; deadlock\?", - TestLog::get()); + EXPECT_EQ("debugLongWaitAndDeadlock: SpinLockTest SpinLock locked for one " + "second; deadlock\?", TestLog::get()); EXPECT_EQ(ticksPerSecond, lock.contendedTicks); // Release the lock, make sure the thread acquires it.