Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency

troygiorshev commented at 4:00 pm on August 6, 2020: contributor

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.

laanwj added the label P2P on Aug 6, 2020

DrahtBot commented at 7:22 pm on August 6, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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.

in src/net.cpp:2834 in 59df5d1c63 outdated

2840         if (pnode->nSendSize > nSendBufferMaxSize)
2841             pnode->fPauseSend = true;
2842-        pnode->vSendMsg.push_back(std::move(serializedHeader));
2843-        if (nMessageSize)
2844+        pnode->vSendMsg.push_back(std::move(serialized_header));
2845+        if (message_size)

jnewbery commented at 1:47 pm on August 18, 2020:

Since you’re cleaning everything up here, this if block (and the one above) should have braces added.

troygiorshev commented at 1:04 pm on August 26, 2020:

Good eye, thanks

jnewbery commented at 1:51 pm on August 18, 2020: member

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?

troygiorshev force-pushed on Aug 26, 2020

troygiorshev commented at 2:00 pm on August 26, 2020: contributor

git range-diff master 59df5d1 e85fb2d

Added missing lock annotations
Cleaned PushMessage more thoroughly

jnewbery commented at 3:00 pm on August 26, 2020: member

utACK e85fb2d34d8515513e404163cf149be61e1ed708

in src/net.cpp:2816 in d093d261e2 outdated

2812@@ -2813,35 +2813,37 @@ bool CConnman::NodeFullyConnected(const CNode* pnode)
2813 
2814 void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
2815 {
2816-    size_t nMessageSize = msg.data.size();
2817-    LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n",  SanitizeString(msg.m_type), nMessageSize, pnode->GetId());
2818+    size_t message_size = msg.data.size();

hebasto commented at 2:00 pm on October 14, 2020:

d093d261e2df27b2466ffe4e92bd48f22818d326

0    const size_t message_size = msg.data.size();

but I think that the code would look better without the message_size variable at all.

troygiorshev commented at 4:11 pm on November 3, 2020:

I don’t have a strong opinion on the removal of message_size and/or total_size. However such a change is a bit more than I’m looking to do here.

in src/net.cpp:2822 in d093d261e2 outdated

2822-    std::vector<unsigned char> serializedHeader;
2823-    pnode->m_serializer->prepareForTransport(msg, serializedHeader);
2824-    size_t nTotalSize = nMessageSize + serializedHeader.size();
2825+    std::vector<unsigned char> serialized_header;
2826+    pnode->m_serializer->prepareForTransport(msg, serialized_header);
2827+    size_t total_size = message_size + serialized_header.size();

hebasto commented at 2:01 pm on October 14, 2020:

d093d261e2df27b2466ffe4e92bd48f22818d326

0    const size_t total_size = message_size + serialized_header.size();

in src/net.h:820 in 9e92f63e2d outdated

816@@ -817,7 +817,7 @@ class CNode
817     std::atomic_bool fPauseSend{false};
818 
819 protected:
820-    mapMsgCmdSize mapSendBytesPerMsgCmd;
821+    mapMsgCmdSize mapSendBytesPerMsgCmd GUARDED_BY(cs_vSend);

hebasto commented at 2:13 pm on October 14, 2020:

9e92f63e2deadbfd7424ce4585f02ad580dcee8c

Is there any reason to keep mapSendBytesPerMsgCmd protected?

troygiorshev commented at 4:22 pm on November 3, 2020:

~~Yes, it’s to keep it consistent with nSendBytes for calls to the getpeerinfo rpc.~~

~~Edit: Well we can’t quite ensure consistency. But with it locking the way it is now, we can ensure that mapSendBytesPerMsgCmd is at least always updated before nSendBytes.~~

I’m sorry, I completely misread your comment! I agree with you and jnewbery, there is no reason for these to be kept protected.

jnewbery commented at 8:19 am on November 4, 2020:

There’s no reason for either of these to be protected, since nothing inherits from CNode. However, that’s separate from this PR and could be fixed elsewhere.

in src/net.cpp:1451 in e85fb2d34d outdated

1450             size_t nBytes = SocketSendData(pnode);
1451-            if (nBytes) {
1452-                RecordBytesSent(nBytes);
1453-            }
1454+            if (nBytes) RecordBytesSent(nBytes);
1455         }

hebasto commented at 2:28 pm on October 14, 2020:

e85fb2d34d8515513e404163cf149be61e1ed708

0        if (sendSet) {
1            const size_t bytes_sent = SocketSendData(pnode);
2            if (bytes_sent) RecordBytesSent(bytes_sent);
3        }

troygiorshev commented at 4:23 pm on November 3, 2020:

Thanks. Again I’ve preferred list initialization (see next comment).

in src/net.cpp:2843 in e85fb2d34d outdated

2844-    if (bytes_sent) RecordBytesSent(bytes_sent);
2845+    // If write queue was empty, attempt "optimistic write"
2846+    if (optimistic_send == true) {
2847+        size_t bytes_sent{SocketSendData(pnode)};
2848+        if (bytes_sent) RecordBytesSent(bytes_sent);
2849+    }

hebasto commented at 2:35 pm on October 14, 2020:

e85fb2d34d8515513e404163cf149be61e1ed708

0    if (optimistic_send) {
1        const size_t bytes_sent = SocketSendData(pnode);
2        if (bytes_sent) RecordBytesSent(bytes_sent);
3    }

troygiorshev commented at 4:18 pm on November 3, 2020:

Thanks for catching the const on this one. However, I prefer list initialization whenever possible, so I’m going to keep it unless you feel strongly against it.

jnewbery commented at 8:29 am on November 4, 2020:

I have a mild stylistic preference to use = when initializing a simple type with the return value of a function, and to use braced initialization for initializing from a literal. That’s perhaps inconsistent, but it seems to be the style that most people gravitate towards. No strong preference since they both result in the same thing, but if it’s a vote, I’d go for = :)

hebasto changes_requested

hebasto commented at 2:36 pm on October 14, 2020: member

Concept ACK.

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

cleanup: Clean PushMessage

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.

75e6280649

Add missing lock annotations

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

92897d4344

Move cs_vSend lock into SocketSendData

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.

9d47d6da52

troygiorshev force-pushed on Nov 3, 2020

troygiorshev commented at 5:39 pm on November 3, 2020: contributor

git range-diff master e85fb2d 9d47d6d

Clean commit improved
Improved various const initializations
Renamed the cleanup commit

troygiorshev commented at 5:47 pm on November 3, 2020: contributor

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 commented at 6:36 am on November 4, 2020: contributor

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.

in src/net.cpp:2827 in 9d47d6da52

2829     {
2830         LOCK(pnode->cs_vSend);
2831-        bool optimisticSend(pnode->vSendMsg.empty());
2832+        optimistic_send = pnode->vSendMsg.empty();
2833 
2834         //log total amount of bytes per message type

jnewbery commented at 8:36 am on November 4, 2020:

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

0        // Log total amount of bytes per message type

in src/net.h:766 in 9d47d6da52

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

jnewbery commented at 8:59 am on November 4, 2020:

I think this could use a comment. Something like:

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

hebasto commented at 4:19 pm on November 11, 2020:

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 commented at 9:02 am on November 4, 2020: member

utACK 9d47d6da524b78ee4a628119f09636b855e881a2

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 commented at 4:47 pm on November 4, 2020: contributor

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 commented at 6:33 pm on November 4, 2020: member

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!

in src/net.h:762 in 92897d4344 outdated

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

hebasto commented at 4:43 pm on November 11, 2020:

92897d43443db3a7cd68d8655d1bbf4b9b69c427

nit, clang-format suggests:

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

in src/net.cpp:750 in 9d47d6da52

746@@ -747,8 +747,9 @@ void V1TransportSerializer::prepareForTransport(CSerializedNetMsg& msg, std::vec
747     CVectorWriter{SER_NETWORK, INIT_PROTO_VERSION, header, 0, hdr};
748 }
749 
750-size_t CConnman::SocketSendData(CNode *pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode->cs_vSend)
751+size_t CConnman::SocketSendData(CNode *pnode) const

hebasto commented at 4:46 pm on November 11, 2020:

9d47d6da524b78ee4a628119f09636b855e881a2

nit, clang-format suggests:

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

in src/net.cpp:1448 in 9d47d6da52

1445@@ -1445,11 +1446,8 @@ void CConnman::SocketHandler()
1446         //
1447         if (sendSet)
1448         {

hebasto commented at 4:47 pm on November 11, 2020:

9d47d6da524b78ee4a628119f09636b855e881a2, nit:

0        if (sendSet) {

hebasto approved

hebasto commented at 4:48 pm on November 11, 2020: member

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

troygiorshev commented at 3:23 pm on November 17, 2020: contributor

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.

in src/net.cpp:2841 in 9d47d6da52

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

MarcoFalke commented at 12:32 pm on December 22, 2020:

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.

MarcoFalke commented at 12:33 pm on December 22, 2020: member

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.

jnewbery commented at 9:39 am on December 27, 2020: member

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

MarcoFalke commented at 7:00 pm on December 28, 2020: member

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

jnewbery commented at 11:22 am on December 31, 2020: member

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

troygiorshev commented at 6:09 am on January 6, 2021: contributor

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.

MarcoFalke referenced this in commit 4eada5d8b1 on Jan 6, 2021

troygiorshev closed this on Jan 6, 2021

sidhujag referenced this in commit c1fd7a4a22 on Jan 6, 2021

DrahtBot locked this on Aug 16, 2022

Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency #19673

Conflicts