Documented locking part 1+2

[alexanderkjeldaas]

This pull request includes needed changes to get started using locking annotations.
threadsafety.h - the set of macros.
sync.h - a mixin that adds annotations to the basic locks.
net.h - I added annotations to functions where the set of held locks before and after the function is called is not the same.

Reviewers: Please look carefully at the TODOs in net.h. The pre/postconditions of those functions are entirely unclear. The set of held locks depend on the value of nHeaderStart ! nHeaderStart is also involved in both != -1 and < 0 tests.

[laanwj]

ACK

[sipa]

Regarding Begin/End/AbortMessage, wouldn't it be cleaner to introduce a CMessageBuilder class, which holds a scoped lock of a referenced CNode::cs_vSend, and forwards operator<< to the respective vSend? For example CNode::PushMessage(pszCommand, a1) could then become simply { CMessageBuilder m(this, pszCommand); m << a1; m.Send(); }.

[jgarzik]

How about just building unlocked, then copying the message into vSend.

That adds a memory copy (potentially large for "block" messages), but it eliminates the locking mess, and also eliminates the whole nHeaderStart-then-back-out-if-we-abort stuff. Clean and simple, if the memory copy burden is OK.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/25511af4a57816c4f9bb960527f090a9719c9010 for binaries and test log.

[laanwj]

I like @jgarzik's idea. Shared-nothing passing messages is a safe and elegant default, if it turns out to be a performance burden, which I don't believe so, it can always be optimized again without the locking mess.

[alexanderkjeldaas]

Hi, sorry I have not had time to follow up on these locking changes, but..

Actually locking and synchronous shared-nothing message passing are very
similar from a theoretical point of view.

To build an intuition for this, imagine that you create a thread for every
set of locks that can be held in the program at the same time, by some
thread. What does this mean? It means that you create a thread for every
single "group" of data that is protected by a set of locks, much like in a
fine-grained message-passing implementation.

Also, for any two locks that can be held at the same time, define a lock
acquisition order (so lock A must be taken before lock B, if they can both
be held by the same thread at the same time).

Now, imagine that every time you take a lock, you instead pass a message to
the designated thread with a copy of "the environment" which consists of
all variables visible in the lexical scope. This message is synchronous,
so you wait until that other thread returns.

Then this is equivalent to the synchronous shared nothing message passing
implementation.

Note that no other thread can legally modify "the environment" because that
would require a lock, and thus execution of said modification would have to
happen in a thread which is currently blocked.

Anyways, what this means is that the process of annotating which data is
protected by which locks is exactly the same information that one encodes
when restructuring a program into message-passing style. However,
message-passing style is a pretty heavy transformation, as it affects both
performance and correctness. Lock annotation only deals with correctness.

Also, for performance reasons, message-passing style in C++ is often not
synchronous. Asynchronous message-passing can actually be a lot harder to
do right than a lock-based implementation because the number of states that
the program can be in is suddenly expanded over the lock-based
implementation.

So I would suggest that the bitcoin software is not rewritten to use
message-passing.

Rather, simply use lock annotations, and as a hygiene issue, limit the size
of "the environment" typically by restricting the visibility of global
variables.

Also, try to get rid of TRY_LOCK.

Alexander

On 6 December 2012 04:44, Wladimir J. van der Laan <notifications@github.com

wrote:

I like @jgarzik https://github.com/jgarzik's idea. Shared-nothing
passing messages is a safe and elegant default, if it turns out to be a
performance burden, which I don't believe so, it can always be optimized
again without the locking mess.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2003#issuecomment-11075710.

[laanwj]

Indeed, theoretically they are equivalent (still, in practice it is harder to mess up as subtly with message passing, as you can see in one glance what is passed instead of having to spend a lot of time carefully analyzing locks).

But I think jgarzik was talking about one specific case, building a message that's going to go over a socket anyway to get rid of a TRY_LOCK, not rewriting the entire application.

[alexanderkjeldaas]

Aha. Mea culpa. That does indeed seem like a good plan.

On 6 December 2012 15:58, Wladimir J. van der Laan <notifications@github.com

wrote:

Indeed, theoretically they are equivalent.
But I think jgarzik was talking about one specific case, building a
message that's going to go over a socket anyway to get rid of a TRY_LOCK,
not rewriting the entire application.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2003#issuecomment-11098676.

[gavinandresen]

Merging; I'm excited about using clang to help us make sure locking is correct and efficient.