Stop Using cs_main for CNodeState/State() #9419

TheBlueMatt · 2016-12-24T16:20:42Z

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.

dcousens · 2016-12-27T08:50:04Z

src/util.cpp

-            const vector<string>& categories = mapMultiArgs["-debug"];
-            ptrCategory.reset(new set<string>(categories.begin(), categories.end()));
-            // thread_specific_ptr automatically deletes the set when the thread ends.
+            if (mapMultiArgs.count("-debug")) {


Why not IsMultiArgSet?

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

dcousens · 2016-12-27T08:51:06Z

src/util.cpp

@@ -412,6 +428,13 @@ bool SoftSetBoolArg(const std::string& strArg, bool fValue)
        return SoftSetArg(strArg, std::string("0"));
 }

+void ForceSetArg(const std::string& strArg, const std::string& strValue)
+{
+    mapArgs[strArg] = strValue;


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

Fixed in #9243

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.

TheBlueMatt · 2016-12-27T18:21:34Z

Rebased after #9243 merge.

sipa · 2016-12-27T18:46:48Z

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 · 2016-12-27T19:28:50Z

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 ***@***.***> 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: #9419 (comment)

TheBlueMatt · 2016-12-28T13:42:12Z

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.

sipa · 2016-12-28T19:57:19Z

src/net_processing.cpp

+
+    void AddStateForNode(NodeId nodeid, const CAddress& addr, const std::string& addrName) {
+        LOCK(cs_main);
+        mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName)));


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.

sipa · 2016-12-28T20:19:38Z

src/net_processing.cpp

+                return CNodeStateAccessor();
+            pstate = it->second;
+        }
+        return CNodeStateAccessor(pstate);


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

sipa · 2016-12-28T20:21:00Z

src/net_processing.cpp

+    std::shared_ptr<CNodeState> pstate;
+
+    CNodeStateAccessor() : pstate() { }
+    CNodeStateAccessor(std::shared_ptr<CNodeState> pstateIn) : pstate(pstateIn) { ENTER_CRITICAL_SECTION(pstate->cs); }


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

TheBlueMatt · 2016-12-29T19:46:36Z

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 · 2016-12-29T20:14:31Z

Addressed @sipa's comments.

theuni · 2016-12-29T21:38:11Z

src/net_processing.cpp

+
+    CNodeStateAccessor(CNodeStateAccessor&& o) { pstate = o.pstate; o.pstate = NULL; }
+
+    bool operator()() const { return (bool)pstate; }


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

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

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.

OK, switched to regular bool.

theuni · 2016-12-29T21:40:10Z

src/net_processing.cpp

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


Stale comment?

theuni · 2016-12-29T21:46:10Z

src/net_processing.cpp

+    std::shared_ptr<CNodeState> pstate;
+
+    CNodeStateAccessor() : pstate() { }
+    CNodeStateAccessor(std::shared_ptr<CNodeState>&& pstateIn) : pstate(std::move(pstateIn)) { ENTER_CRITICAL_SECTION(pstate->cs); }


Why not just have a lock as a member?

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

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?

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 · 2016-12-29T22:20:13Z

Wanted to make sure my nits so far were reasonable, so I went ahead and patched 'em in. Feel free to take from theuni@058193e

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.

TheBlueMatt · 2017-01-01T04:16:13Z

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

theuni · 2017-01-02T21:57:56Z

@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 · 2017-01-02T22:10:03Z

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

theuni · 2017-01-02T22:46:20Z

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 · 2017-01-02T22:51:34Z

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 · 2017-01-12T18:58:23Z

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

TheBlueMatt · 2017-01-17T15:41:50Z

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

TheBlueMatt force-pushed the 2016-12-nodestate branch 4 times, most recently from 2b53e69 to fcc8c2c Compare December 24, 2016 19:38

fanquake added the Refactoring label Dec 24, 2016

TheBlueMatt force-pushed the 2016-12-nodestate branch from fcc8c2c to 67bc69b Compare December 25, 2016 04:51

dcousens reviewed Dec 27, 2016

View reviewed changes

TheBlueMatt mentioned this pull request Dec 27, 2016

Clean up mapArgs and mapMultiArgs Usage #9243

Merged

TheBlueMatt added 2 commits December 27, 2016 19:18

Make CBlockIndex*es in net_processing const

b37bcbc

TheBlueMatt force-pushed the 2016-12-nodestate branch from 67bc69b to 8a5ebaa Compare December 27, 2016 18:21

TheBlueMatt force-pushed the 2016-12-nodestate branch from 8a5ebaa to 32dd1c3 Compare December 28, 2016 13:41

sipa reviewed Dec 28, 2016

View reviewed changes

TheBlueMatt force-pushed the 2016-12-nodestate branch from 32dd1c3 to 227e394 Compare December 29, 2016 20:14

Move mapNodeState into an encapsulation class

0a15834

TheBlueMatt force-pushed the 2016-12-nodestate branch from 227e394 to 79c5a21 Compare December 29, 2016 20:14

theuni reviewed Dec 29, 2016

View reviewed changes

TheBlueMatt added 3 commits January 1, 2017 05:10

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

56c4392

Add a few AssertLockHelds for cs_main in net_processing

c410d93

TheBlueMatt added 2 commits January 1, 2017 05:10

Optimize State() calls in ::SENDCMPCT processing

f1c0ae0

Remove cs_main lock requirements for Misbehaving()

e195459

TheBlueMatt force-pushed the 2016-12-nodestate branch from 79c5a21 to e195459 Compare January 1, 2017 04:15

This was referenced Jan 8, 2017

Parallel ThreadMessageHandler #9488

Closed

Relay compact block messages prior to full block connection #9375

Merged

TheBlueMatt mentioned this pull request Jan 12, 2017

Split CNode::cs_vSend: message processing and message sending #9535

Merged

TheBlueMatt closed this Jan 17, 2017

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021


		CNodeStateAccessor(CNodeStateAccessor&& o) { pstate = o.pstate; o.pstate = NULL; }

		bool operator()() const { return (bool)pstate; }

Stop Using cs_main for CNodeState/State() #9419

Stop Using cs_main for CNodeState/State() #9419

Uh oh!

Conversation

TheBlueMatt commented Dec 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcousens Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Dec 27, 2016

Uh oh!

sipa commented Dec 27, 2016

Uh oh!

TheBlueMatt commented Dec 27, 2016 via email

Uh oh!

TheBlueMatt commented Dec 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Dec 29, 2016

Uh oh!

TheBlueMatt commented Dec 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theuni commented Dec 29, 2016

Uh oh!

TheBlueMatt commented Jan 1, 2017

Uh oh!

theuni commented Jan 2, 2017

Uh oh!

sipa commented Jan 2, 2017 via email

Uh oh!

theuni commented Jan 2, 2017

Uh oh!

TheBlueMatt commented Jan 2, 2017

Uh oh!

TheBlueMatt commented Jan 12, 2017

Uh oh!

TheBlueMatt commented Jan 17, 2017

Uh oh!

Uh oh!

dcousens Dec 27, 2016 •

edited

Loading

TheBlueMatt Jan 1, 2017 •

edited

Loading