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

1 files changed, 14 insertions, 0 deletions

diff --git a/doc/developer-notes.md b/doc/developer-notes.md
index c3ab3fa953..f06902e624 100644
--- a/doc/developer-notes.md
+++ b/doc/developer-notes.md
@@ -17,6 +17,7 @@ Developer Notes
         - [`debug.log`](#debuglog)
         - [Signet, testnet, and regtest modes](#signet-testnet-and-regtest-modes)
         - [DEBUG_LOCKORDER](#debug_lockorder)
+        - [DEBUG_LOCKCONTENTION](#debug_lockcontention)
         - [Valgrind suppressions file](#valgrind-suppressions-file)
         - [Compiling for test coverage](#compiling-for-test-coverage)
         - [Performance profiling with perf](#performance-profiling-with-perf)
@@ -362,6 +363,19 @@ configure option adds `-DDEBUG_LOCKORDER` to the compiler flags. This inserts
 run-time checks to keep track of which locks are held and adds warnings to the
 `debug.log` file if inconsistencies are detected.
 
+### DEBUG_LOCKCONTENTION
+
+Defining `DEBUG_LOCKCONTENTION` adds a "lock" logging category to the logging
+RPC that, when enabled, logs the location and duration of each lock contention
+to the `debug.log` file.
+
+To enable it, run configure with `-DDEBUG_LOCKCONTENTION` added to your
+CPPFLAGS, e.g. `CPPFLAGS="-DDEBUG_LOCKCONTENTION"`, then build and run bitcoind.
+
+You can then use the `-debug=lock` configuration option at bitcoind startup or
+`bitcoin-cli logging '["lock"]'` at runtime to turn on lock contention logging.
+It can be toggled off again with `bitcoin-cli logging [] '["lock"]'`.
+
 ### Assertions and Checks
 
 The util file `src/util/check.h` offers helpers to protect against coding and