Stop Using cs_main for CNodeState/State()

TheBlueMatt commented at 4:20 pm on December 24, 2016: member

Based on #9243 and the top commit off of #9375 (“Make CBlockIndex*es in net_processing const”), this no longer requires cs_main to call State() and removes a few cs_main’s around Misbehaving() for an easy win.

TheBlueMatt force-pushed on Dec 24, 2016

fanquake added the label Refactoring on Dec 24, 2016

TheBlueMatt force-pushed on Dec 25, 2016

in src/util.cpp: in 67bc69b159 outdated

239@@ -238,9 +240,12 @@ bool LogAcceptCategory(const char* category)
240         static boost::thread_specific_ptr<set<string> > ptrCategory;
241         if (ptrCategory.get() == NULL)
242         {
243-            const vector<string>& categories = mapMultiArgs["-debug"];
244-            ptrCategory.reset(new set<string>(categories.begin(), categories.end()));
245-            // thread_specific_ptr automatically deletes the set when the thread ends.
246+            if (mapMultiArgs.count("-debug")) {

dcousens commented at 8:50 am on December 27, 2016:

Why not IsMultiArgSet?

TheBlueMatt commented at 12:51 pm on December 27, 2016:

Prefer less diff? (note this is really from #9243, not strictly this PR).

Make CBlockIndex*es in net_processing const b37bcbc04e

Split CNode::cs_vSend: message processing and message sending

cs_vSend is used for two purposes - to lock the datastructures used
to queue messages to place on the wire and to only call
SendMessages once at a time per-node. I believe SendMessages used
to access some of the vSendMsg stuff, but it doesn't anymore, so
these locks do not need to be on the same mutex, and also make
deadlocking much more likely.

c214d120a3

in src/util.cpp: in 67bc69b159 outdated

427@@ -412,6 +428,13 @@ bool SoftSetBoolArg(const std::string& strArg, bool fValue)
428         return SoftSetArg(strArg, std::string("0"));
429 }
430 
431+void ForceSetArg(const std::string& strArg, const std::string& strValue)
432+{
433+    mapArgs[strArg] = strValue;

dcousens commented at 8:51 am on December 27, 2016:

Why is this not LOCK(cs_args)? (maybe I misinterpreted the meaning of force)

TheBlueMatt commented at 12:52 pm on December 27, 2016:

Fixed in #9243

TheBlueMatt commented at 6:21 pm on December 27, 2016: member

Rebased after #9243 merge.

TheBlueMatt force-pushed on Dec 27, 2016

sipa commented at 6:46 pm on December 27, 2016: member

Wouldn’t it be easier to use a shared_ptr wrapping for cleanup-after-last-use, instead of implementing refcounting yourself? You can still use a manual wrapper to lock/unlock.

TheBlueMatt commented at 7:28 pm on December 27, 2016: member

Hmm, I kinda prefer to not introduce shared_ptr abstractions unless they’re optimizing a copy that we don’t want to do. I don’t think the manual refcounting is that bad.

On December 27, 2016 7:46:52 PM GMT+01:00, Pieter Wuille notifications@github.com wrote:

Wouldn’t it be easier to use a shared_ptr wrapping for cleanup-after-last-use, instead of implementing refcounting yourself? You can still use a manual wrapper to lock/unlock.

– You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/9419#issuecomment-269366047

TheBlueMatt force-pushed on Dec 28, 2016

TheBlueMatt commented at 1:42 pm on December 28, 2016: member

OK, @sipa found a better way to use shared_ptrs here so I went ahead and did that. It means removing some asserts, but I think its worth it.

in src/net_processing.cpp: in 1ee9f54ff1 outdated

235+        return &it->second;
236+    }
237+
238+    void AddStateForNode(NodeId nodeid, const CAddress& addr, const std::string& addrName) {
239+        LOCK(cs_main);
240+        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName)));

sipa commented at 7:57 pm on December 28, 2016:

The std::move(addrMany) here won’t do what you want, as addrName is a const reference. You can’t destroy addrName anyway, so just remove the std::move.

in src/net_processing.cpp: in 85aec47cf9 outdated

268+            auto it = mapNodeState.find(nodeid);
269+            if (it == mapNodeState.end())
270+                return CNodeStateAccessor();
271+            pstate = it->second;
272+        }
273+        return CNodeStateAccessor(pstate);

sipa commented at 8:19 pm on December 28, 2016:

You should add an std::move here if you want to avoid a copy.

in src/net_processing.cpp: in 85aec47cf9 outdated

227+class CNodeStateAccessor {
228+private:
229+    std::shared_ptr<CNodeState> pstate;
230+
231+    CNodeStateAccessor() : pstate() { }
232+    CNodeStateAccessor(std::shared_ptr<CNodeState> pstateIn) : pstate(pstateIn) { ENTER_CRITICAL_SECTION(pstate->cs); }

sipa commented at 8:21 pm on December 28, 2016:

Add a std::move around pstateIn to avoid a copy.

TheBlueMatt commented at 7:46 pm on December 29, 2016: member

Went down the rabbit hole here trying to track down a deadlock and then realized that it was already prevented. Note that (very much on purpose) cs_main is still used prior to all CNodeStateAccessors except for a few where we only call Misbehaving() or otherwise sure we are only locking one CNodeState at a time (to avoid deadlocks where we lock them in different orders).

TheBlueMatt force-pushed on Dec 29, 2016

TheBlueMatt commented at 8:14 pm on December 29, 2016: member

Addressed @sipa’s comments.

Move mapNodeState into an encapsulation class 0a15834e12

TheBlueMatt force-pushed on Dec 29, 2016

in src/net_processing.cpp: in 79c5a2124c outdated

246+    CNodeStateAccessor(const CNodeStateAccessor&) =delete;
247+    CNodeStateAccessor& operator= (const CNodeStateAccessor&) =delete;
248+
249+    CNodeStateAccessor(CNodeStateAccessor&& o) { pstate = o.pstate; o.pstate = NULL; }
250+
251+    bool operator()() const { return (bool)pstate; }

theuni commented at 9:38 pm on December 29, 2016:

explicit operator bool instead? Using the call operator for this seems strange.

TheBlueMatt commented at 10:20 pm on December 29, 2016:

I dunno I dont really like the bool operator. If you really prefer it I’m happy to change it.

theuni commented at 2:02 am on December 30, 2016:

explicit bool matches the semantics of smart pointers, so I think it’s a good fit here. But more importantly, the call operator makes it look as though something is happening to the accessor, so I’d really rather not use that.

TheBlueMatt commented at 4:11 am on January 1, 2017:

OK, switched to regular bool.

in src/net_processing.cpp: in 4bb585ed6c outdated

245+    CNodeState* operator->() { return &(*pstate); }
246+    const CNodeState* operator->() const { return &(*pstate); }
247+};
248+
249 class NodeStateStorage {
250     /** Map maintaining per-node state. Requires cs_main. */

theuni commented at 9:40 pm on December 29, 2016:

Stale comment?

TheBlueMatt commented at 4:11 am on January 1, 2017:

Fixed.

in src/net_processing.cpp: in 4bb585ed6c outdated

227+class CNodeStateAccessor {
228+private:
229+    std::shared_ptr<CNodeState> pstate;
230+
231+    CNodeStateAccessor() : pstate() { }
232+    CNodeStateAccessor(std::shared_ptr<CNodeState>&& pstateIn) : pstate(std::move(pstateIn)) { ENTER_CRITICAL_SECTION(pstate->cs); }

theuni commented at 9:46 pm on December 29, 2016:

Why not just have a lock as a member?

TheBlueMatt commented at 10:22 pm on December 29, 2016:

I didnt realize we had an RAII lock in sync.h that would still do DEBUG_LOCKORDER checks. Will fix with CMutexLock.

TheBlueMatt commented at 3:53 am on January 1, 2017:

Hmm, looking again using CMutexLock means adding __FILE__, __LINE__, etc macros in net_processing for parameters to CMutexLock, instead of letting ENTER_CRITICAL_SECTION figure it out. If you prefer I could try to add some crazy defines like CONSTRUCT_CMUTEXLOCK(mutex, lockName) which fills in a constructor for use in the CNodeStateAccessor constructor?

theuni commented at 9:40 pm on January 2, 2017:

Hmm, I think you actually want these coming from net_processing anyway, no? I think if you make the ctor inline and pass __LINE__ to CMutexLock, you’d end up seeing the source of the lock. Otherwise, you’re always seeing it come from the same place I should think.

Taking that a step further, I think it would work to change CMutexLock’s ctors to take default __FILE__ and __LINE__ args?

theuni commented at 10:20 pm on December 29, 2016: member

Wanted to make sure my nits so far were reasonable, so I went ahead and patched ’em in. Feel free to take from https://github.com/theuni/bitcoin/commit/058193e4d51bdd0cb8e7c225130414b817207ac2

Give CNodeState its own lock (and accessor class for RAII access) 56c4392080

Let shared_ptrs cleanup CNodeStates

This removes reliance on net in net_processing for maintaining the
refcount == 0 invariant when calling FinalizeNode

This also removes a few asserts which used to be checked when there
were no more CNodeStates remaining, which we can no longer check for.

These should be pretty rarely checked anyway, so probably didn't
serve much use.

470911c75a

Add a few AssertLockHelds for cs_main in net_processing c410d9303b

Optimize State() calls in ::SENDCMPCT processing f1c0ae0999

Remove cs_main lock requirements for Misbehaving() e195459b2c

TheBlueMatt force-pushed on Jan 1, 2017

TheBlueMatt commented at 4:16 am on January 1, 2017: member

Fixed a few of @theuni’s comments, rebased diff-tree is at https://0bin.net/paste/nkpqzdxyoAXkoX7B#STuXxIS32GXf3gMP1JbiIPrufL9gq1hcSH+g6yXSKdq

theuni commented at 9:57 pm on January 2, 2017: member

@TheBlueMatt Maybe I’m missing something big-picture here, but would it not be possible to hold a new global mutex rather than cs_main to serialize CNodeStateAccessor access? Or is there a reason that they must share the same lock?

sipa commented at 10:10 pm on January 2, 2017: member

@theuni I think that’s the plan longer term.

theuni commented at 10:46 pm on January 2, 2017: member

I suppose I’m trying to figure out, for the sake of reviewing this PR, what remains to be done before that can happen. I’m certainly not insinuating that we should hold this up until that point, I’d just like to get an idea of what other issues still need to be solved.

So if, for ex, we naively made CNodeState::cs static and dropped the cs_main guards, what kind of carnage would be expected?

TheBlueMatt commented at 10:51 pm on January 2, 2017: member

Aside from the places that do, actually, require cs_main, a global order between various CNodeState::cs locks needs to be defined (or, equivalently, we could say that you may not hold multiple CNodeState::cs locks at once).

TheBlueMatt commented at 6:58 pm on January 12, 2017: member

Without #9488 (which I do not think should make 0.14 at this juncture), this should not go in for 0.14.

TheBlueMatt commented at 3:41 pm on January 17, 2017: member

Closing for now. Will make access exclusive and work towards a more whole solution for 0.15.

TheBlueMatt closed this on Jan 17, 2017

MarcoFalke locked this on Sep 8, 2021

Stop Using cs_main for CNodeState/State() #9419