From 166c0a5ebe3dcd76e7f694cfe77e9dd81020009e Mon Sep 17 00:00:00 2001 From: Fletcher Dunn Date: Wed, 28 Aug 2024 16:44:38 -0700 Subject: [PATCH] Adjust our lock hygiene checking framework Allow multiple ShortDurationLocks to be taken, as long as they are taken in a certain order. P4:8765422 --- .../clientlib/csteamnetworkingsockets.cpp | 2 +- .../steamnetworkingsockets_connections.cpp | 4 +- .../steamnetworkingsockets_connections.h | 6 +- .../steamnetworkingsockets_lowlevel.cpp | 64 ++++++++++++------- .../steamnetworkingsockets_lowlevel.h | 33 ++++++++-- .../steamnetworkingsockets_thinker.cpp | 2 +- 6 files changed, 76 insertions(+), 35 deletions(-) diff --git a/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp b/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp index 213f541..7560ac9 100644 --- a/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp +++ b/src/steamnetworkingsockets/clientlib/csteamnetworkingsockets.cpp @@ -433,7 +433,7 @@ CSteamNetworkingSockets::CSteamNetworkingSockets( CSteamNetworkingUtils *pSteamN #endif , m_bEverTriedToGetCert( false ) , m_bEverGotCert( false ) -, m_mutexPendingCallbacks( "pending_callbacks" ) +, m_mutexPendingCallbacks( "pending_callbacks", LockDebugInfo::k_nOrder_Max ) // Never take another lock while holding this { m_connectionConfig.Init( nullptr ); InternalClearIdentity(); diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp index 96b4d0b..f7f7ee3 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.cpp @@ -164,7 +164,7 @@ void CSteamNetworkingMessage::Unlink() UnlinkFromQueue( &CSteamNetworkingMessage::m_linksSecondaryQueue ); } -ShortDurationLock g_lockAllRecvMessageQueues( "all_recv_msg_queue" ); +ShortDurationLock g_lockAllRecvMessageQueues( "all_recv_msg_queue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this void SteamNetworkingMessageQueue::AssertLockHeld() const { @@ -638,7 +638,7 @@ CSteamNetworkConnectionBase::~CSteamNetworkConnectionBase() } static std::vector s_vecPendingDeleteConnections; -static ShortDurationLock s_lockPendingDeleteConnections( "connection_delete_queue" ); +static ShortDurationLock s_lockPendingDeleteConnections( "connection_delete_queue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this void CSteamNetworkConnectionBase::ConnectionQueueDestroy() { diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h index ecdbc38..1566bd6 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_connections.h @@ -248,7 +248,7 @@ private: class CSteamNetworkPollGroup; struct PollGroupLock : Lock { - PollGroupLock() : Lock( "pollgroup", LockDebugInfo::k_nFlag_PollGroup ) {} + PollGroupLock() : Lock( "pollgroup", LockDebugInfo::k_nFlag_PollGroup, LockDebugInfo::k_nOrder_ObjectOrTable ) {} }; using PollGroupScopeLock = ScopeLock; @@ -339,7 +339,7 @@ protected: ///////////////////////////////////////////////////////////////////////////// struct ConnectionLock : Lock { - ConnectionLock() : Lock( "connection", LockDebugInfo::k_nFlag_Connection ) {} + ConnectionLock() : Lock( "connection", LockDebugInfo::k_nFlag_Connection, LockDebugInfo::k_nOrder_ObjectOrTable ) {} }; struct ConnectionScopeLock : ScopeLock { @@ -1115,7 +1115,7 @@ extern CUtlHashMap, Identity { - TableLock() : Lock( "table", LockDebugInfo::k_nFlag_Table ) {} + TableLock() : Lock( "table", LockDebugInfo::k_nFlag_Table, LockDebugInfo::k_nOrder_ObjectOrTable ) {} }; using TableScopeLock = ScopeLock; extern TableLock g_tables_lock; diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp index e3b1156..4d17f6b 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.cpp @@ -83,7 +83,7 @@ static void FlushSystemSpew(); int g_cbUDPSocketBufferSize = 256*1024; /// Global lock for all local data structures -static Lock s_mutexGlobalLock( "global", 0 ); +static Lock s_mutexGlobalLock( "global", 0, LockDebugInfo::k_nOrder_Global ); #if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0 @@ -208,37 +208,50 @@ void LockDebugInfo::AboutToLock( bool bTry ) { // Remember when we started trying to lock t.m_usecOuterLockStartTime = SteamNetworkingSockets_GetLocalTimestamp(); + return; } - else + + // We already hold a lock. Check for taking locks in such a way + // that might lead to deadlocks. + const LockDebugInfo *pTopLock = t.m_arHeldLocks[ t.m_nHeldLocks-1 ]; + + // Taking locks in increasing order is always allowed + if ( likely( pTopLock->m_nOrder < m_nOrder ) ) + return; + + // Global lock *must* always be the outermost lock. (It is legal to take other locks in + // between and then lock the global lock recursively.) + const bool bHoldGlobalLock = t.m_arHeldLocks[ 0 ] == &s_mutexGlobalLock; + AssertMsg( + bHoldGlobalLock || this != &s_mutexGlobalLock, + "Taking global lock while already holding lock '%s'", t.m_arHeldLocks[ 0 ]->m_pszName + ); + + // If they are only "trying", we allow out-of-order behaviour. + if ( bTry ) + return; + + // Taking locks of equal order? This will also be true for recursive locks, which are not allowed for 'short duration' locks. + if ( likely( pTopLock->m_nOrder == m_nOrder ) ) { + if ( m_nOrder < k_nOrder_ObjectOrTable ) // OK to lock global lock recursively + return; - // We already hold a lock. Make sure it's legal for us to take another! - - // Global lock *must* always be the outermost lock. (It is legal to take other locks in - // between and then lock the global lock recursively.) - const bool bHoldGlobalLock = t.m_arHeldLocks[ 0 ] == &s_mutexGlobalLock; - AssertMsg( - bHoldGlobalLock || this != &s_mutexGlobalLock, - "Taking global lock while already holding lock '%s'", t.m_arHeldLocks[ 0 ]->m_pszName - ); - - // Check for taking locks in such a way that might lead to deadlocks. - // If they are only "trying", then we do allow out of order behaviour. - if ( !bTry ) + // Taking multiple object locks? This is allowed under certain circumstances + if ( likely( m_nOrder == k_nOrder_ObjectOrTable ) ) { - const LockDebugInfo *pTopLock = t.m_arHeldLocks[ t.m_nHeldLocks-1 ]; - // Once we take a "short duration" lock, we must not - // take any additional locks! (Including a recursive lock.) - AssertMsg( !( pTopLock->m_nFlags & LockDebugInfo::k_nFlag_ShortDuration ), "Taking lock '%s' while already holding lock '%s'", m_pszName, pTopLock->m_pszName ); + // If we hold the global lock, it's OK + if ( bHoldGlobalLock ) + return; // If the global lock isn't held, then no more than one // object lock is allowed, since two different threads // might take them in different order. constexpr int k_nObjectFlags = LockDebugInfo::k_nFlag_Connection | LockDebugInfo::k_nFlag_PollGroup; if ( - ( !bHoldGlobalLock && ( m_nFlags & k_nObjectFlags ) != 0 ) - //|| ( m_nFlags & k_nFlag_Table ) // We actually do this in one place when we know it's OK. Not wirth it right now to get this situation exempted from the checking. + ( ( m_nFlags & k_nObjectFlags ) != 0 ) + //|| ( m_nFlags & k_nFlag_Table ) // We actually do this in one place when we know it's OK. Not worth it right now to get this situation exempted from the checking. ) { // We must not already hold any existing object locks (except perhaps this one) for ( int i = 0 ; i < t.m_nHeldLocks ; ++i ) @@ -248,8 +261,13 @@ void LockDebugInfo::AboutToLock( bool bTry ) "Taking lock '%s' and then '%s', while not holding the global lock", pOtherLock->m_pszName, m_pszName ); } } + + // Usage is OK if we didn't find any problems above + return; } } + + AssertMsg( false, "Taking lock '%s' while already holding lock '%s'", m_pszName, pTopLock->m_pszName ); } void LockDebugInfo::OnLocked( const char *pszTag ) @@ -457,7 +475,7 @@ static volatile bool s_bManualPollMode; // ///////////////////////////////////////////////////////////////////////////// -ShortDurationLock s_lockTaskQueue( "TaskQueue" ); +ShortDurationLock s_lockTaskQueue( "TaskQueue", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this CTaskTarget::~CTaskTarget() { @@ -3512,7 +3530,7 @@ void CSharedSocket::RemoteHost::Close() // ///////////////////////////////////////////////////////////////////////////// -static ShortDurationLock s_systemSpewLock( "SystemSpew" ); +static ShortDurationLock s_systemSpewLock( "SystemSpew", LockDebugInfo::k_nOrder_Max ); // Never take another lock while holding this SteamNetworkingMicroseconds g_usecLastRateLimitSpew; int g_nRateLimitSpewCount; ESteamNetworkingSocketsDebugOutputType g_eAppSpewLevel = k_ESteamNetworkingSocketsDebugOutputType_Msg; // Option selected by app diff --git a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.h b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.h index dd8f90c..43b033d 100644 --- a/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.h +++ b/src/steamnetworkingsockets/clientlib/steamnetworkingsockets_lowlevel.h @@ -452,8 +452,31 @@ struct LockDebugInfo static constexpr int k_nFlag_PollGroup = (1<<2); static constexpr int k_nFlag_Table = (1<<4); + // When multiple locks are taken, there is potential for a deadlock. + // Locks taken in increasing order is always allowed, in decreasing + // order is never allowed. In certain situations we allow locks + // of the same order to be taken. + enum { + + // Global lock must always be taken first + k_nOrder_Global, + + // Object locks (table, connection, poll group) can be taken in any + // order while holding the global lock. Otherwise, you may not take + // more than one. + k_nOrder_ObjectOrTable, + + // We might need to take a lock to queue callbacks while holding this. + k_nOrder_NetworkAndCachedRoutes, + + // Short duration locks are usually the lowest priority and we usually + // should not take more than one of these at a time + k_nOrder_Max + }; + const char *const m_pszName; const int m_nFlags; + const int m_nOrder; #if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0 void _AssertHeldByCurrentThread( const char *pszFile, int line, const char *pszTag = nullptr ) const; @@ -462,7 +485,7 @@ struct LockDebugInfo #endif protected: - LockDebugInfo( const char *pszName, int nFlags ) : m_pszName( pszName ), m_nFlags( nFlags ) {} + LockDebugInfo( const char *pszName, int nFlags, int nOrder ) : m_pszName( pszName ), m_nFlags( nFlags ), m_nOrder( nOrder ) {} #if STEAMNETWORKINGSOCKETS_LOCK_DEBUG_LEVEL > 0 void AboutToLock( bool bTry ); @@ -480,7 +503,7 @@ protected: template struct Lock : LockDebugInfo { - inline Lock( const char *pszName, int nFlags ) : LockDebugInfo( pszName, nFlags ) {} + inline Lock( const char *pszName, int nFlags, int nOrder ) : LockDebugInfo( pszName, nFlags, nOrder ) {} inline void lock( const char *pszTag = nullptr ) { LockDebugInfo::AboutToLock( false ); @@ -572,11 +595,11 @@ private: // A very simple lock to protect short accesses to a small set of data. // Used when: // - We hold the lock for a brief period. -// - We don't need to take any additional locks while already holding this one. -// (Including this lock -- e.g. we don't need to lock recursively.) +// - We don't need to lock recursively. +// - For most locks, we also don't allow taking of any other locks. (In a few cases we use the order to mark which are allowed.) struct ShortDurationLock : Lock { - ShortDurationLock( const char *pszName ) : Lock( pszName, k_nFlag_ShortDuration ) {} + ShortDurationLock( const char *pszName, int nOrder ) : Lock( pszName, k_nFlag_ShortDuration, nOrder ) {} }; using ShortDurationScopeLock = ScopeLock; diff --git a/src/steamnetworkingsockets/steamnetworkingsockets_thinker.cpp b/src/steamnetworkingsockets/steamnetworkingsockets_thinker.cpp index 5e073cf..e739fd8 100644 --- a/src/steamnetworkingsockets/steamnetworkingsockets_thinker.cpp +++ b/src/steamnetworkingsockets/steamnetworkingsockets_thinker.cpp @@ -62,7 +62,7 @@ IThinker::~IThinker() struct ShortDurationLock { inline void lock() {}; inline void unlock() {}; }; static ShortDurationLock s_mutexThinkerTable; #else -static ShortDurationLock s_mutexThinkerTable( "thinker" ); +static ShortDurationLock s_mutexThinkerTable( "thinker", ShortDurationLock::k_nOrder_Max ); // We do sometimes take another lock, but it is always a "try" #endif // Base class isn't lockable