cs_main during IBD in CChainState::IsInitialBlockDownload(), as most of its callers already hold the lock. Have these callers invoke non-locking function CChainState::IsIBD() instead. Apply thread safety analysis to the IBD functions. Retains the post-IBD optimization latch for the callers that do not hold the lock. Makes explicit which callers hold cs_main, and which do not.
I recall that the reason IsIBD was written like this:
0bool CChainState::IsInitialBlockDownload() const
1{
2 // Optimization: pre-test latch before taking the lock.
3 if (m_cached_finished_ibd.load(std::memory_order_relaxed))
4 return false;
5
6 LOCK(cs_main);
is so that in the common case – which is that we’ve already latched to false – that we can respond without ever acquiring cs_main inside this function. See #8007
Maybe there’s a better refactor that could be done so that we can reduce locking contention on cs_main further, but this patch would have all the call sites grab the lock in order to even call IsIBD(), which would add more cs_main lock acquisitions in general – and that seems counter to where we want to end up?
Is the motivation for this patch to reduce the uses of cs_main as a recursive mutex, and thereby get to a better design? If so that sounds like a good direction, but I think it’d be better if we avoided adding extra lock acquisitions (and holding cs_main longer) in order to get there. For instance it seems like with this patch some of the validation callbacks will now be invoked while holding cs_main the whole time? That seems undesirable.
Yes, the trade-off this pull makes EDIT: is no longer made, thanks to review feedback. foregoing the optimization for a half dozen callers that don’t already hold cs_main, in favor of avoiding re-taking the cs_main lock for the 18 callers that do already hold it, by moving the lock-taking to the half dozen callers (but just for the call itself, so essentially only the optimization is affected unless I am missing something).
Thanks for the link to #8007. It looks like the proportion of callers that hold the lock has since increased.
3116- // Only notify about a new block tip if the active chain was modified.
3117- if (pindex_was_in_chain) {
3118- uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), to_mark_failed->pprev);
3119+ // Only notify about a new block tip if the active chain was modified.
3120+ if (pindex_was_in_chain) {
3121+ uiInterface.NotifyBlockTip(GetSynchronizationState(IsInitialBlockDownload()), to_mark_failed->pprev);
uiInterface.NotifyBlockTip(GetSynchronizationState(WITH_LOCK(::cs_main, return IsInitialBlockDownload())), to_mark_failed->pprev);
For instance it seems like with this patch some of the validation callbacks will now be invoked while holding cs_main the whole time? That seems undesirable.
Agree with @sdaftuar; IsIBD() is called in a few performance-critical places and at the very least this change should be benchmarked before merge. I’m Concept NACK if this change is essentially just for refactoring’s sake; if there’s some overarching benefit it would be nice to hear that.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
::UnloadBlockIndex to BlockManager by dongcarl)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
potentially improving performance by not needlessly retaking the cs_main lock is the only point here
AFAIU lock acquisition is essentially free if it’s already held, so I would be surprised if there’s a performance gain from avoiding re-acquisition.
4608@@ -4608,11 +4609,11 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
4609 // MaybeSendPing may have marked peer for disconnection
4610 if (pto->fDisconnect) return true;
4611
4612- MaybeSendAddr(*pto, *peer, current_time);
4613-
4614 {
4615 LOCK(cs_main);
4616
4617+ MaybeSendAddr(*pto, *peer, current_time);
Pushed a different approach along the lines of #24220 (comment). It does not increase the scope of cs_main and maintains the optimization, while avoiding reacquiring the mutex during IBD for the majority of callers that already hold it.
This may also have a silent merge conflict with #22053?
With the latest push, if MaybeSendAddr no longer holds cs_main it should call IsIBD, otherwise clang provides a thread safety warning.
to reduce the use of cs_main as a recursive mutex.
I think this is a goal that is almost impossible to achieve with a mutex visible in the global scope/namespace. And if someone did it, it would be brittle and easily lead to UB.
See also how for a far simpler and private mutex (m_chainstate_mutex), it took two attempts to get right: #24235.
Personally, I think a better way would be to no longer hold cs_main for non-validation uses and then make cs_main private to chainman.
cs_main and which do not, now and going forward. Edit: updated the pull description.
675@@ -676,7 +676,13 @@ class CChainState
676 void UnloadBlockIndex() EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
677
678 /** Check whether we are doing an initial block download (synchronizing from disk or network) */
679- bool IsInitialBlockDownload() const;
680+ bool IsInitialBlockDownload() const EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
IsIBD the new semantics
as most of its callers already hold the cs_main lock;
have these callers invoke non-locking IsIBD() instead.
Retains the post-IBD optimization latch for the callers that
do not hold the lock.
Makes explicit which callers hold cs_main and which do not.
Ensures that callers now and going forward invoke the correct function.
681+
682+ /**
683+ * Same as `IsInitialBlockDownload()`, but also locks mutex cs_main during
684+ * initial block download. Use for callers not already holding the lock.
685+ */
686+ bool IsIBD() const;
LOCKS_EXCLUDED to prevent accidentally calling this when we know we have cs_main locked?
LOCKS_EXCLUDED thread safety annotation to the declaration along with an AssertLockNotHeld run-time assertion in the definition.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.