Add MemoryBarrier to SyncBlock Lock upgrade path

src/coreclr/vm/syncblk.cpp

@@ -1307,117 +1307,161 @@ void DumpSyncBlockCache()
 
 // ***************************************************************************
 //
-//              ObjHeader class implementation
+//              SpinLock implementation
 //
 // ***************************************************************************
 
-#ifdef MP_LOCKS
-DEBUG_NOINLINE void ObjHeader::EnterSpinLock()
+namespace
 {
-    // NOTE: This function cannot have a dynamic contract.  If it does, the contract's
-    // destructor will reset the CLR debug state to what it was before entering the
-    // function, which will undo the BeginNoTriggerGC() call below.
-    STATIC_CONTRACT_GC_NOTRIGGER;
-
-#ifdef _DEBUG
-    int i = 0;
-#endif
+#ifdef MP_LOCKS
+    void EnterSpinLock(Volatile<DWORD>* pLock)
+    {
+        STATIC_CONTRACT_GC_NOTRIGGER;
 
-    DWORD dwSwitchCount = 0;
+    #ifdef _DEBUG
+        int i = 0;
+    #endif
 
-    while (TRUE)
-    {
-#ifdef _DEBUG
-#ifdef HOST_64BIT
-        // Give 64bit more time because there isn't a remoting fast path now, and we've hit this assert
-        // needlessly in CLRSTRESS.
-        if (i++ > 30000)
-#else
-        if (i++ > 10000)
-#endif // HOST_64BIT
-            _ASSERTE(!"ObjHeader::EnterLock timed out");
-#endif
-        // get the value so that it doesn't get changed under us.
-        LONG curValue = m_SyncBlockValue.LoadWithoutBarrier();
+        DWORD dwSwitchCount = 0;
 
-        // check if lock taken
-        if (! (curValue & BIT_SBLK_SPIN_LOCK))
-        {
-            // try to take the lock
-            LONG newValue = curValue | BIT_SBLK_SPIN_LOCK;
-            LONG result = InterlockedCompareExchange((LONG*)&m_SyncBlockValue, newValue, curValue);
-            if (result == curValue)
-                break;
-        }
-        if  (g_SystemInfo.dwNumberOfProcessors > 1)
+        while (TRUE)
         {
-            for (int spinCount = 0; spinCount < BIT_SBLK_SPIN_COUNT; spinCount++)
+    #ifdef _DEBUG
+    #ifdef HOST_64BIT
+            // Give 64bit more time because there isn't a remoting fast path now, and we've hit this assert
+            // needlessly in CLRSTRESS.
+            if (i++ > 30000)
+    #else
+            if (i++ > 10000)
+    #endif // HOST_64BIT
+                _ASSERTE(!"ObjHeader::EnterLock timed out");
+    #endif
+            // get the value so that it doesn't get changed under us.
+            LONG curValue = pLock->LoadWithoutBarrier();
+
+            // check if lock taken
+            if (! (curValue & BIT_SBLK_SPIN_LOCK))
             {
-                if  (! (m_SyncBlockValue & BIT_SBLK_SPIN_LOCK))
+                // try to take the lock
+                LONG newValue = curValue | BIT_SBLK_SPIN_LOCK;
+                LONG result = InterlockedCompareExchange((LONG*)pLock, newValue, curValue);
+                if (result == curValue)
                     break;
-                YieldProcessorNormalized(); // indicate to the processor that we are spinning
             }
-            if  (m_SyncBlockValue & BIT_SBLK_SPIN_LOCK)
+            if  (g_SystemInfo.dwNumberOfProcessors > 1)
+            {
+                for (int spinCount = 0; spinCount < BIT_SBLK_SPIN_COUNT; spinCount++)
+                {
+                    if  (! (*pLock & BIT_SBLK_SPIN_LOCK))
+                        break;
+                    YieldProcessorNormalized(); // indicate to the processor that we are spinning
+                }
+                if  (*pLock & BIT_SBLK_SPIN_LOCK)
+                    __SwitchToThread(0, ++dwSwitchCount);
+            }
+            else
                 __SwitchToThread(0, ++dwSwitchCount);
         }
-        else
-            __SwitchToThread(0, ++dwSwitchCount);
     }
-
-    INCONTRACT(Thread* pThread = GetThreadNULLOk());
-    INCONTRACT(if (pThread != NULL) pThread->BeginNoTriggerGC(__FILE__, __LINE__));
-}
 #else
-DEBUG_NOINLINE void ObjHeader::EnterSpinLock()
-{
-    STATIC_CONTRACT_GC_NOTRIGGER;
+    void EnterSpinLock(Volatile<DWORD>* pLock)
+    {
+        STATIC_CONTRACT_GC_NOTRIGGER;
 
 #ifdef _DEBUG
-    int i = 0;
+        int i = 0;
 #endif
 
-    DWORD dwSwitchCount = 0;
+        DWORD dwSwitchCount = 0;
 
-    while (TRUE)
-    {
+        while (TRUE)
+        {
 #ifdef _DEBUG
-        if (i++ > 10000)
-            _ASSERTE(!"ObjHeader::EnterLock timed out");
+            if (i++ > 10000)
+                _ASSERTE(!"ObjHeader::EnterLock timed out");
 #endif
-        // get the value so that it doesn't get changed under us.
-        LONG curValue = m_SyncBlockValue.LoadWithoutBarrier();
+            // get the value so that it doesn't get changed under us.
+            LONG curValue = pLock->LoadWithoutBarrier();
 
-        // check if lock taken
-        if (! (curValue & BIT_SBLK_SPIN_LOCK))
+            // check if lock taken
+            if (! (curValue & BIT_SBLK_SPIN_LOCK))
+            {
+                // try to take the lock
+                LONG newValue = curValue | BIT_SBLK_SPIN_LOCK;
+                LONG result = InterlockedCompareExchange((LONG*)pLock, newValue, curValue);
+                if (result == curValue)
+                    break;
+            }
+            __SwitchToThread(0, ++dwSwitchCount);
+        }
+    }
+#endif //MP_LOCKS
+
+    void ReleaseSpinLock(Volatile<DWORD>* pLock)
+    {
+        LIMITED_METHOD_CONTRACT;
+
+        InterlockedAnd((LONG*)pLock, ~BIT_SBLK_SPIN_LOCK);
+    }
+
+    struct HeaderSpinLockHolder
+    {
+        Volatile<DWORD>*   m_pLock;
+
+        HeaderSpinLockHolder(Volatile<DWORD>* pLock)
+            : m_pLock(pLock)
         {
-            // try to take the lock
-            LONG newValue = curValue | BIT_SBLK_SPIN_LOCK;
-            LONG result = InterlockedCompareExchange((LONG*)&m_SyncBlockValue, newValue, curValue);
-            if (result == curValue)
-                break;
+            // Acquire the spin-lock in preemptive mode with GC_NOTRIGGER
+            // to avoid deadlocks with the GC.
+            CONTRACTL
+            {
+                GC_NOTRIGGER;
+                NOTHROW;
+                MODE_PREEMPTIVE;
+            }
+            CONTRACTL_END;
+            EnterSpinLock(m_pLock);
+        }
+
+        ~HeaderSpinLockHolder()
+        {
+            LIMITED_METHOD_CONTRACT;
+            ReleaseSpinLock(m_pLock);
         }
-        __SwitchToThread(0, ++dwSwitchCount);
     }
+}
+#endif //!DACCESS_COMPILE
+
+
+// ***************************************************************************
+//
+//              ObjHeader class implementation
+//
+// ***************************************************************************
+
+#ifndef DACCESS_COMPILE
+
+
+DEBUG_NOINLINE void ObjHeader::EnterSpinLock()
+{
+    // NOTE: This function cannot have a dynamic contract.  If it does, the contract's
+    // destructor will reset the CLR debug state to what it was before entering the
+    // function, which will undo the BeginNoTriggerGC() call below.
+    STATIC_CONTRACT_GC_NOTRIGGER;
+
+    ::EnterSpinLock(std::addressof(m_SyncBlockValue));
 
     INCONTRACT(Thread* pThread = GetThreadNULLOk());
     INCONTRACT(if (pThread != NULL) pThread->BeginNoTriggerGC(__FILE__, __LINE__));
 }
-#endif //MP_LOCKS
-
 DEBUG_NOINLINE void ObjHeader::ReleaseSpinLock()
 {
-    LIMITED_METHOD_CONTRACT;
-
     INCONTRACT(Thread* pThread = GetThreadNULLOk());
     INCONTRACT(if (pThread != NULL) pThread->EndNoTriggerGC());
 
-    InterlockedAnd((LONG*)&m_SyncBlockValue, ~BIT_SBLK_SPIN_LOCK);
+    ::ReleaseSpinLock(std::addressof(m_SyncBlockValue));
 }
 
-#endif //!DACCESS_COMPILE
-
-#ifndef DACCESS_COMPILE
-
 DWORD ObjHeader::GetSyncBlockIndex()
 {
     CONTRACTL
@@ -1736,41 +1780,58 @@ OBJECTHANDLE SyncBlock::GetOrCreateLock(OBJECTREF lockObj)
 
     SetPrecious();
 
-    // We need to create a new lock
-    DWORD thinLock = m_thinLock;
+    // We'll likely need to put this lock object into the sync block.
+    // Create the handle here.
     OBJECTHANDLEHolder lockHandle = GetAppDomain()->CreateHandle(lockObj);
 
+    // Switch to preemptive so we can grab the spin-lock.
+    // Use the NO_DTOR version so we don't do a coop->preemptive->coop transition on return.
+    GCX_PREEMP_NO_DTOR();
+
+    HeaderSpinLockHolder lock(std::addressof(m_thinLock));
+
+    // We don't need to be in preemptive any more here.
+    GCX_PREEMP_NO_DTOR_END();
+
+    // Check again now that we hold the spin-lock
+    OBJECTHANDLE existingLock = VolatileLoad(&m_Lock);
+    if (existingLock != (OBJECTHANDLE)NULL)
+    {
+        return existingLock;
+    }
+
+    // We need to create a new lock
+    // Grab the bits that are interesting for thin-lock info.
+    // This way we only call back into managed code
+    // to initialize the lock when necessary.
+    DWORD thinLock = (m_thinLock & ((SBLK_MASK_LOCK_THREADID) | (SBLK_MASK_LOCK_RECLEVEL)));
+
     if (thinLock != 0)
     {
-        GCPROTECT_BEGIN(lockObj);
 
         // We have thin-lock info that needs to be transferred to the lock object.
         DWORD lockThreadId = thinLock & SBLK_MASK_LOCK_THREADID;
         DWORD recursionLevel = (thinLock & SBLK_MASK_LOCK_RECLEVEL) >> SBLK_RECLEVEL_SHIFT;
         _ASSERTE(lockThreadId != 0);
         PREPARE_NONVIRTUAL_CALLSITE(METHOD__LOCK__INITIALIZE_FOR_MONITOR);
         DECLARE_ARGHOLDER_ARRAY(args, 3);
-        args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(lockObj);
+        args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(ObjectFromHandle(lockHandle));
         args[ARGNUM_1] = DWORD_TO_ARGHOLDER(lockThreadId);
         args[ARGNUM_2] = DWORD_TO_ARGHOLDER(recursionLevel);
         CALL_MANAGED_METHOD_NORET(args);
-
-        GCPROTECT_END();
     }
 
-    OBJECTHANDLE existingHandle = InterlockedCompareExchangeT(&m_Lock, lockHandle.GetValue(), NULL);
-
-    if (existingHandle != NULL)
-    {
-        return existingHandle;
-    }
+    VolatileStore(&m_Lock, lockHandle.GetValue());
 
     // Our lock instance is in the sync block now.
     // Don't release it.
     lockHandle.SuppressRelease();
+
     // Also, clear the thin lock info.
     // It won't be used any more, but it will look out of date.
-    m_thinLock = 0u;
+    // Only clear the relevant bits, as the spin-lock bit is used to lock this method.
+    // That bit will be reset upon return.
+    m_thinLock &= ~((SBLK_MASK_LOCK_THREADID) | (SBLK_MASK_LOCK_RECLEVEL));
 
     return lockHandle;
 }