Moved performance-critical SpinLock::{lock, tryLock, unlock} to the h…

…eader file

... and changed the implementation to be a Test-And-Test-And-Set (TTAS) Lock.

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"

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;
 }
 

src/SpinLock.cc

@@ -13,11 +13,10 @@
  * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
  */
 
-#include <mutex>
 #include <unordered_set>
 
-#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<int>(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

src/SpinLock.h

@@ -18,8 +18,8 @@
 
 #include <mutex>
 #include <atomic>
+#include <x86intrin.h>
 
-#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<SpinLock> 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.

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.