Merge bitcoin/bitcoin#24770: Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive

43947333315d07f59e1247bd76e0ba9d35a99e31 Add DEBUG_LOCKCONTENTION documentation to the developer notes (Jon Atack)
39a34b6877945908759f6a2322f60852e521e2ee Put lock logging behind DEBUG_LOCKCONTENTION preprocessor directive (Jon Atack)

Pull request description:

  This is a more minimal, no-frills version of #24734 for backport. The other fixes and improvements in that pull can be done after.

  *Copy of the PR 24734 description:*

  PRs #22736, #22904 and #23223 changed lock contention logging from a `DEBUG_LOCKCONTENTION` compile-time preprocessor directive to a runtime `lock` log category and improved the logging output. This changed the locking from using `lock()` to `try_lock()`:

  - `void Mutex::UniqueLock::lock()` acquires the mutex and blocks until it gains access to it

  - `bool Mutex::UniqueLock::try_lock()` doesn't block but instead immediately returns whether it acquired the mutex; it may be used by `lock()` internally as part of the deadlock-avoidance algorithm

  In theory the cost of `try_lock` might be essentially the [same](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-697) relative to `lock`. The test-and-set logic of these calls is purported to be ~ constant time, optimised and light/quick if used carefully (i.e. no mutex convoying), compared to system calls, memory/cache coherency and fences, wait queues, and (particularly) lock contentions. See the discussion around https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-902851054 and after with respect to performance/cost aspects.  However, there are reasonable concerns (see [here](https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691277896) and [here](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-620)) that `Base::try_lock()` may be potentially [costly](https://www.erisian.com.au/bitcoin-core-dev/log-2022-03-31.html#l-700) or [risky](https://github.com/bitcoin/bitcoin/pull/22904#issuecomment-930484001) compared to `Base::lock()` in this very frequently called code.

  One alternative to keep the run-time lock logging would be to gate the `try_lock` call behind the logging conditional, for example as proposed in https://github.com/bitcoin/bitcoin/commit/ccd73de1dd969097d34634c2be2fc32b03fbd09e and ACKed [here](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-901980815). However, this would add the [cost](https://github.com/bitcoin/bitcoin/pull/22736#issuecomment-910102353) of `if (LogAcceptCategory(BCLog::LOCK))` to the hotspot, instead of replacing `lock` with `try_lock`, for the most frequent happy path (non-contention).

  It turns out we can keep the advantages of the runtime lock contention logging (the ability to turn it on/off at runtime) while out of prudence putting the `try_lock()` call and `lock` logging category behind a  `DEBUG_LOCKCONTENTION` compile-time preprocessor directive, and also still retain the lock logging enhancements of the mentioned PRs, as suggested in https://github.com/bitcoin/bitcoin/pull/24734#issuecomment-1085785480 by W. J. van der Laan, in https://github.com/bitcoin/bitcoin/pull/22736#discussion_r691280693, and in the linked IRC discussion.

  Proposed here and for backport to v23.

ACKs for top commit:
  laanwj:
    Code review ACK 43947333315d07f59e1247bd76e0ba9d35a99e31

Tree-SHA512: 89b1271cae1dca0eb251914b1a60fc5b68320aab4a3939c57eec3a33a3c8f01688f05d95dfc31f91d71a6ed80cfe2d67b77ff14742611cc206175e47b2e5d3b1

5 files changed, 18 insertions, 3 deletions

diff --git a/src/logging.cpp b/src/logging.cpp
index 764941c8ea..a5e5fdb4cd 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -160,7 +160,9 @@ const CLogCategoryDesc LogCategories[] =
     {BCLog::VALIDATION, "validation"},
     {BCLog::I2P, "i2p"},
     {BCLog::IPC, "ipc"},
+#ifdef DEBUG_LOCKCONTENTION
     {BCLog::LOCK, "lock"},
+#endif
     {BCLog::UTIL, "util"},
     {BCLog::BLOCKSTORE, "blockstorage"},
     {BCLog::ALL, "1"},
diff --git a/src/logging.h b/src/logging.h
index 710e6c4c32..ae8cad906c 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -60,7 +60,9 @@ namespace BCLog {
         VALIDATION  = (1 << 21),
         I2P         = (1 << 22),
         IPC         = (1 << 23),
+#ifdef DEBUG_LOCKCONTENTION
         LOCK        = (1 << 24),
+#endif
         UTIL        = (1 << 25),
         BLOCKSTORE  = (1 << 26),
         ALL         = ~(uint32_t)0,
diff --git a/src/net.h b/src/net.h
index db09ce564e..1bc011c748 100644
--- a/src/net.h
+++ b/src/net.h
@@ -13,6 +13,7 @@
 #include <crypto/siphash.h>
 #include <hash.h>
 #include <i2p.h>
+#include <logging.h>
 #include <net_permissions.h>
 #include <netaddress.h>
 #include <netbase.h>
@@ -32,6 +33,7 @@
 #include <cstdint>
 #include <deque>
 #include <functional>
+#include <list>
 #include <map>
 #include <memory>
 #include <optional>
diff --git a/src/sync.h b/src/sync.h
index 85000a9151..af7595e6fa 100644
--- a/src/sync.h
+++ b/src/sync.h
@@ -6,8 +6,11 @@
 #ifndef BITCOIN_SYNC_H
 #define BITCOIN_SYNC_H
 
+#ifdef DEBUG_LOCKCONTENTION
 #include <logging.h>
 #include <logging/timer.h>
+#endif
+
 #include <threadsafety.h>
 #include <util/macros.h>
 
@@ -136,8 +139,10 @@ private:
     void Enter(const char* pszName, const char* pszFile, int nLine)
     {
         EnterCritical(pszName, pszFile, nLine, Base::mutex());
+#ifdef DEBUG_LOCKCONTENTION
         if (Base::try_lock()) return;
         LOG_TIME_MICROS_WITH_CATEGORY(strprintf("lock contention %s, %s:%d", pszName, pszFile, nLine), BCLog::LOCK);
+#endif
         Base::lock();
     }
 
diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp
index 153ccd984b..0d95bd7670 100644
--- a/src/test/checkqueue_tests.cpp
+++ b/src/test/checkqueue_tests.cpp
@@ -19,13 +19,17 @@
 #include <vector>
 
 /**
- * Identical to TestingSetup but excludes lock contention logging, as some of
- * these tests are designed to be heavily contested to trigger race conditions
- * or other issues.
+ * Identical to TestingSetup but excludes lock contention logging if
+ * `DEBUG_LOCKCONTENTION` is defined, as some of these tests are designed to be
+ * heavily contested to trigger race conditions or other issues.
  */
 struct NoLockLoggingTestingSetup : public TestingSetup {
     NoLockLoggingTestingSetup()
+#ifdef DEBUG_LOCKCONTENTION
         : TestingSetup{CBaseChainParams::MAIN, /*extra_args=*/{"-debugexclude=lock"}} {}
+#else
+        : TestingSetup{CBaseChainParams::MAIN} {}
+#endif
 };
 
 BOOST_FIXTURE_TEST_SUITE(checkqueue_tests, NoLockLoggingTestingSetup)