diff options
| author | fanquake <fanquake@gmail.com> | 2022-04-08 13:28:18 +0100 |
|---|---|---|
| committer | fanquake <fanquake@gmail.com> | 2022-04-08 13:30:24 +0100 |
| commit | c1059c9fef188e1d025458fdf33eb992af4376e2 (patch) | |
| tree | 7ea28ccb5a3425c433ae0d787a2ff25f5e817ec6 /test | |
| parent | 38d3d0bfc4cae6b31c5ac30f9b0da458bd9a9e57 (diff) | |
| parent | 43947333315d07f59e1247bd76e0ba9d35a99e31 (diff) | |
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
Diffstat (limited to 'test')
| -rwxr-xr-x | test/functional/rpc_misc.py | 3 |
1 files changed, 0 insertions, 3 deletions
diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index 2f1796d7cc..f64aae7223 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -56,9 +56,6 @@ class RpcMiscTest(BitcoinTestFramework): self.log.info("test logging rpc and help") - # Test logging RPC returns the expected number of logging categories. - assert_equal(len(node.logging()), 27) - # Test toggling a logging category on/off/on with the logging RPC. assert_equal(node.logging()['qt'], True) node.logging(exclude=['qt']) |