Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

troygiorshev · 2020-08-06T16:00:13Z

As it stands, RecordBytesSent is called twice, once under the cs_vSend lock, and once not. This PR moves both calls out of the lock.

In doing this, I've made a change to SocketSendData. Before, it required the caller to lock cs_vSend, now it locks cs_vSend itself. The only case where this makes a difference is when performing an optimistic write (where MessageHandler is the thread that calls SocketSendData, not SocketHandler). As justified in the commit message, I don't think this will cause any problems, and subjectively I think this is an improvement to how we're using the locks. I'm more than happy to be told that I'm wrong :)

Additionally this PR brings PushMessage into line with the style guide, and does verious other cleanups.

This PR was originally related to #18784 but it now stands on its own.

DrahtBot · 2020-08-06T19:22:22Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

net: Move RecordBytesSent() call out of cs_vSend lock #20816 (net: Move RecordBytesSent() call out of cs_vSend lock by jnewbery)

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.

jnewbery

Concept ACK.

It seems to me that there are a few additional members of CNode that should be guarded by the cs_vSend lock:

mapSendBytesPerMsgCmd
nSendSize
nSendOffset

I don't think we want any of those to be accessed/updated while another thread is locking cs_vSend and updating vSendMsg and nSendBytes. If you agree, what do you think about updating them in this PR?

src/net.cpp

troygiorshev · 2020-08-26T14:00:33Z

git range-diff master 59df5d1 e85fb2d

Added missing lock annotations
Cleaned PushMessage more thoroughly

jnewbery · 2020-08-26T15:00:47Z

utACK e85fb2d

hebasto

Concept ACK.

The first commit message could be prefixed with refactor: , no?

src/net.cpp

src/net.h

src/net.cpp

This commit brings PushMessage in line with the style guide, with a few simple renames, some changes to if statements, and the removal of an unnecessary "== true". Easter Egg - serializedHeader has the weird "in between" naming convention: no szHungarianNotation but still camelCase.

These variables are already guarded by cs_vSend, they are just missing the annotations.

SocketSendData requires cs_vSend to be locked, but it places this responsibility with the caller. This is unnecessary, unclear, and has caused us to accidentally add RecordBytesSent under the lock. This commit fixes both problems. Justification is needed in the case of PushMessage. It is ok that we let go of cs_vSend for a moment before locking it again and calling SocketSendData. In the case where optimistic_send == false, this lock is let go of for a long time between adding messages to vSendMsg and the call to SocketSendData (in SocketHandler). As corrected in the comment change, optimistic_send represents whether vSendMsg *was* empty, not *is* empty (as, of course, we've just added to it). If SocketHandler grabs cs_vSend in the middle then SocketSendData will be empty when we call it in PushMessage. As suggested by the "if (bytes_sent)" check, this is fine.

troygiorshev · 2020-11-03T17:39:18Z

git range-diff master e85fb2d 9d47d6d

Clean commit improved
Improved various const initializations
Renamed the cleanup commit

troygiorshev · 2020-11-03T17:47:38Z

Seeing that #18784 is now closed, I've reworded the description of this PR. This PR didn't end up being related to the bug #18784 was trying to fix, so this PR now stands on its own.

troygiorshev · 2020-11-04T06:36:01Z

Can't replicate CI failure. mempool_updatefromblock.py passes in 172 seconds on my machine. (It was the longest test by a factor of 2x). Didn't investigate further.

jnewbery · 2020-11-04T08:36:46Z

src/net.cpp

    {
        LOCK(pnode->cs_vSend);
-        bool optimisticSend(pnode->vSendMsg.empty());
+        optimistic_send = pnode->vSendMsg.empty();

        //log total amount of bytes per message type


If you retouch the branch, you may as well touch every line in this function:

Suggested change

//log total amount of bytes per message type

// Log total amount of bytes per message type

jnewbery · 2020-11-04T08:59:44Z

src/net.h

-    size_t nSendSize{0}; // total size of all vSendMsg entries
-    size_t nSendOffset{0}; // offset inside the first vSendMsg already sent
+    size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries
+    size_t nSendOffset GUARDED_BY(cs_vSend){0}; // offset inside the first vSendMsg already sent
    uint64_t nSendBytes GUARDED_BY(cs_vSend){0};
    std::deque<std::vector<unsigned char>> vSendMsg GUARDED_BY(cs_vSend);
    RecursiveMutex cs_vSend;


I think this could use a comment. Something like:

//* Guards the send buffer and statistics about data sent on this connection */

The GUARDED_BY annotation seems quite self-documented; especially, when the mutex and all of the data members protected by it are nicely grouped :)

jnewbery · 2020-11-04T09:02:49Z

utACK 9d47d6d

Justification for the change in commit Move cs_vSend lock into SocketSendData looks good to me.

I'd personally remove the easter egg from the cleanup: Clean PushMessage commit log, but it's fine either way.

troygiorshev · 2020-11-04T16:47:15Z

Thanks for the review @jnewbery! I'll leave your suggestions for now, and I'll look at adding them if there are non-stylistic changes needed for this PR.

My usage of list initialization comes from this stackoverflow. The C++ Core Guidelines talk about this as well, ES.23.

Edit: Another article about list initialization for those interested (link)

jnewbery · 2020-11-04T18:33:40Z

My usage of list initialization comes from this stackoverflow. The C++ Core Guidelines talk about this as well, ES.23.

Edit: Another article about list initialization for those interested (link)

Thanks for the links. I retract my vote for = initialization!

hebasto

ACK 9d47d6d, I have reviewed the code and it looks OK, I agree it can be merged.

hebasto · 2020-11-11T16:43:57Z

src/net.h

@@ -759,8 +759,8 @@ class CNode
    // socket
    std::atomic<ServiceFlags> nServices{NODE_NONE};
    SOCKET hSocket GUARDED_BY(cs_hSocket);
-    size_t nSendSize{0}; // total size of all vSendMsg entries
-    size_t nSendOffset{0}; // offset inside the first vSendMsg already sent
+    size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries


92897d4

nit, clang-format suggests:

Suggested change

size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries

size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries

hebasto · 2020-11-11T16:46:14Z

src/net.cpp

@@ -747,8 +747,9 @@ void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
    CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, header, 0, hdr};
 }

-size_t CConnman::SocketSendData(CNode *pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_vSend)
+size_t CConnman::SocketSendData(CNode *pnode) const


9d47d6d

nit, clang-format suggests:

Suggested change

size_t CConnman::SocketSendData(CNode *pnode) const

size_t CConnman::SocketSendData(CNode* pnode) const

hebasto · 2020-11-11T16:47:25Z

src/net.cpp

        if (sendSet)
        {


9d47d6d, nit:

if (sendSet) {

troygiorshev · 2020-11-17T15:23:46Z

Thanks for the ack hebasto! I now have an ack from you and jnewbery. Both of you gave style suggestions (which I agree with). Since the suggestions are both further improvements to existing code (as opposed to style problems I'm adding myself, with the exception of the spaces before the comment) I'm going to leave them until a functional change is needed.

maflcko

What is the goal of this change? If it is to avoid a lock when RecordBytesSent, you can simply release the lock before calling RecordBytesSent. I don't think this justifies the other behaviour changes.

maflcko · 2020-12-22T12:32:10Z

src/net.cpp

-    if (bytes_sent) RecordBytesSent(bytes_sent);
+    // If write queue was empty, attempt "optimistic write"
+    if (optimistic_send) {
+        const size_t bytes_sent{SocketSendData(pnode)};


Why is this change needed? Previously we'd optimistically write only one message, now we might write more than one message when other messages have been pushed by other threads.

jnewbery · 2020-12-27T09:39:48Z

@MarcoFalke would you prefer an approach like https://github.com/jnewbery/bitcoin/tree/pr19673.1 ?

maflcko · 2020-12-28T19:00:02Z

Yes, if the goal is to avoid a lock when RecordBytesSent, then simply releasing the lock before calling RecordBytesSent seems a good solution.

jnewbery · 2020-12-31T11:22:30Z

@MarcoFalke - I've opened #20816 as an alternative.

troygiorshev · 2021-01-06T06:09:08Z

I was too preoccupied with whether or not I could, I didn't stop to think whether or not I should :)

Closing this in favor of #20816.

378aedc [net] Add cs_vSend lock annotations (John Newbery) 6732545 [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery) Pull request description: RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend. Also correctly annotate the CNode data members that are guarded by cs_vSend. This is a simpler alternative to #19673. ACKs for top commit: jnewbery: ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs. troygiorshev: ACK 378aedc theStack: re-ACK 378aedc MarcoFalke: review ACK 378aedc 🔌 Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07

… lock 378aedc [net] Add cs_vSend lock annotations (John Newbery) 6732545 [net] Move RecordBytesSent() call out of cs_vSend lock (John Newbery) Pull request description: RecordBytesSent() does not require cs_vSend to be locked, so reduce the scope of cs_vSend. Also correctly annotate the CNode data members that are guarded by cs_vSend. This is a simpler alternative to bitcoin#19673. ACKs for top commit: jnewbery: ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs. troygiorshev: ACK 378aedc theStack: re-ACK 378aedc MarcoFalke: review ACK 378aedc 🔌 Tree-SHA512: e9cd6c472b7e1479120c1bf2d1c640cf6d18c7d589a5f9b7dfc4875e5790adaab403a7a1b945a47e79e7249a614b8583270e4549f89b22e8a9edb2e4818b0d07

laanwj added the P2P label Aug 6, 2020

This was referenced Aug 6, 2020

Per-Peer Message Capture #19509

Merged

net: RecordBytesSent under cs_vSend lock #18784

Closed

jnewbery reviewed Aug 18, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Aug 21, 2020

Add BIP324 encrypted p2p transport de-/serializer (only used in tests) #18242

Closed

troygiorshev force-pushed the 2020-08-socketsenddata branch from 59df5d1 to e85fb2d Compare August 26, 2020 13:58

DrahtBot mentioned this pull request Sep 4, 2020

eBPF Linux tracepoints #19866

Merged

3 tasks

hebasto requested changes Oct 14, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

troygiorshev added 3 commits November 3, 2020 11:57

Add missing lock annotations

92897d4

These variables are already guarded by cs_vSend, they are just missing the annotations.

troygiorshev force-pushed the 2020-08-socketsenddata branch from e85fb2d to 9d47d6d Compare November 3, 2020 17:39

jnewbery reviewed Nov 4, 2020

View reviewed changes

hebasto approved these changes Nov 11, 2020

View reviewed changes

hebasto mentioned this pull request Nov 11, 2020

refactor, net: Increase CNode data member encapsulation #20373

Merged

maflcko reviewed Dec 22, 2020

View reviewed changes

jnewbery mentioned this pull request Dec 31, 2020

net: Move RecordBytesSent() call out of cs_vSend lock #20816

Merged

troygiorshev closed this Jan 6, 2021

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

troygiorshev commented Aug 6, 2020 •

edited

DrahtBot commented Aug 6, 2020 •

edited

jnewbery left a comment

troygiorshev commented Aug 26, 2020

jnewbery commented Aug 26, 2020

hebasto left a comment

troygiorshev commented Nov 3, 2020

troygiorshev commented Nov 3, 2020

troygiorshev commented Nov 4, 2020

jnewbery Nov 4, 2020

jnewbery Nov 4, 2020

hebasto Nov 11, 2020

jnewbery commented Nov 4, 2020

troygiorshev commented Nov 4, 2020 •

edited

jnewbery commented Nov 4, 2020

hebasto left a comment

hebasto Nov 11, 2020

hebasto Nov 11, 2020

hebasto Nov 11, 2020

troygiorshev commented Nov 17, 2020

maflcko left a comment

maflcko Dec 22, 2020

jnewbery commented Dec 27, 2020

maflcko commented Dec 28, 2020

jnewbery commented Dec 31, 2020

troygiorshev commented Jan 6, 2021

	//log total amount of bytes per message type
	// Log total amount of bytes per message type

	size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries
	size_t nSendSize GUARDED_BY(cs_vSend){0}; // total size of all vSendMsg entries

	size_t CConnman::SocketSendData(CNode *pnode) const
	size_t CConnman::SocketSendData(CNode* pnode) const

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

Conversation

troygiorshev commented Aug 6, 2020 • edited

DrahtBot commented Aug 6, 2020 • edited

Conflicts

jnewbery left a comment

Choose a reason for hiding this comment

troygiorshev commented Aug 26, 2020

jnewbery commented Aug 26, 2020

hebasto left a comment

Choose a reason for hiding this comment

troygiorshev commented Nov 3, 2020

troygiorshev commented Nov 3, 2020

troygiorshev commented Nov 4, 2020

jnewbery Nov 4, 2020

Choose a reason for hiding this comment

jnewbery Nov 4, 2020

Choose a reason for hiding this comment

hebasto Nov 11, 2020

Choose a reason for hiding this comment

jnewbery commented Nov 4, 2020

troygiorshev commented Nov 4, 2020 • edited

jnewbery commented Nov 4, 2020

hebasto left a comment

Choose a reason for hiding this comment

hebasto Nov 11, 2020

Choose a reason for hiding this comment

hebasto Nov 11, 2020

Choose a reason for hiding this comment

hebasto Nov 11, 2020

Choose a reason for hiding this comment

troygiorshev commented Nov 17, 2020

maflcko left a comment

Choose a reason for hiding this comment

maflcko Dec 22, 2020

Choose a reason for hiding this comment

jnewbery commented Dec 27, 2020

maflcko commented Dec 28, 2020

jnewbery commented Dec 31, 2020

troygiorshev commented Jan 6, 2021

troygiorshev commented Aug 6, 2020 •

edited

DrahtBot commented Aug 6, 2020 •

edited

troygiorshev commented Nov 4, 2020 •

edited