net: Move RecordBytesSent() call out of cs

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

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.

fanquake added the label P2P on Dec 31, 2020

MarcoFalke commented at 11:26 am on December 31, 2020: member

Concept ACK. Will review next year.

DrahtBot commented at 3:44 pm on December 31, 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:

#19673 (Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency by troygiorshev)

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.

practicalswift commented at 4:08 pm on December 31, 2020: contributor

Concept ACK

Thanks for improving locking!

in src/net.cpp:1517 in a1b86d09c8 outdated

1522-            if (nBytes) {
1523-                RecordBytesSent(nBytes);
1524-            }
1525+        if (sendSet) {
1526+            // Send data
1527+            size_t bytes_sent = WITH_LOCK(pnode->cs_vSend, {return SocketSendData(pnode);});

theStack commented at 2:35 pm on January 2, 2021:

stylistic yocto-nit: could drop the curly braces for the lambda argument here – according to git grep WITH_LOCK.*return on all other places the code is directly passed, w/o putting in a block.

0            size_t bytes_sent = WITH_LOCK(pnode->cs_vSend, return SocketSendData(pnode));

OTOH maybe the way in this PR is preferred and the others should be adapted (though not in this PR, obviously), IDK.

jnewbery commented at 5:21 pm on January 2, 2021:

Fixed. Much nicer, thanks!

theStack approved

theStack commented at 2:42 pm on January 2, 2021: member

ACK a06c1ed04dd6b318eb0e7aa0e152ded834b647d6 🔒

[net] Move RecordBytesSent() call out of cs_vSend lock 673254515a

[net] Add cs_vSend lock annotations 378aedc452

jnewbery force-pushed on Jan 2, 2021

theStack approved

theStack commented at 5:19 pm on January 2, 2021: member

re-ACK 378aedc45248cea82d9a3e6dc1038d6828008a76

in src/net.cpp:1516 in 673254515a outdated

1521-            size_t nBytes = SocketSendData(pnode);
1522-            if (nBytes) {
1523-                RecordBytesSent(nBytes);
1524-            }
1525+        if (sendSet) {
1526+            // Send data

MarcoFalke commented at 10:22 am on January 5, 2021:

in the first commit: I think the code is already self-documenting. No need for this

jnewbery commented at 11:27 am on January 5, 2021:

I only kept this because there was a comment before but you’re right - it’s useless. Removing.

in src/net.cpp:1517 in 673254515a outdated

1522-            if (nBytes) {
1523-                RecordBytesSent(nBytes);
1524-            }
1525+        if (sendSet) {
1526+            // Send data
1527+            size_t bytes_sent = WITH_LOCK(pnode->cs_vSend, return SocketSendData(pnode));

MarcoFalke commented at 10:24 am on January 5, 2021:

same commit: The lock annotation for this member function SocketSendData is in the wrong place. Should be in the h file not the cpp file because otherwise moving code can disable the annotation.

Would be nice to fix in this pull and maybe also update the SocketSendData pointer arg to a reference?

jnewbery commented at 12:01 pm on January 5, 2021:

Done.

MarcoFalke approved

MarcoFalke commented at 10:46 am on January 5, 2021: member

review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUh/bAwAlAViNkPZb/+MFuod/2ZmdPPu+12UtIpBh3oTcEKw+G/wdVHJ7fCPI7Tv
 8yvd8cPzeOKp3dXSqsuTu6UOHJ9+mcRodOkiu9F6smvgo0NLIC+DKYFUoRD8ywaEN
 92GhjFdiIKJoaDPRLM+YA3FEVbJG1oxqUX6ifnlk/Zyh+/zCLd5l4S3GqcLQmhyJ/
104guvd1eq9a4H1apiGEop3j75Xm+jFvou07WF31RJgJgmP9L3WELsYItcpLe7+Gar
11nVhL6amBwh4Ctt2DYcggNhP3mk/a6bOSvYVBDOAlfya4SOlg1weBXwjU1o1jpv2C
12X5HgiToUfl4UzlYoFz7kxPoFJHCMhgaTadL23IiJpf5Tm/nNX/NVcJbTf5+WRUfd
13ns/enM8FESK7cp4z4qwnOk1EY5tfHhrGTnXx36GaVyABggL/TaObMxrGZbxjBiPx
14T8wByTCoTUdpXHjtqloT3AzLoJq8uTRJQlyd5WyqPjQbn75hcBij1JmprCD6KoUo
15CAf3cVsZ
16=tcoi
17-----END PGP SIGNATURE-----

Timestamp of file with hash df2bf40ec7e87ed6608ff1eeaa002645df088c8901feef602a141b1ef0909d5f -

MarcoFalke commented at 10:47 am on January 5, 2021: member

let me know if you want this merged or address the style nits

jnewbery commented at 11:25 am on January 5, 2021: member

let me know if you want this merged or address the style nits

I’ll address your review comments. Might as well fix those things up while I’m touching this code.

jnewbery force-pushed on Jan 5, 2021

jnewbery commented at 12:02 pm on January 5, 2021: member

Thanks for the review @MarcoFalke. I’ve addressed your comments.

theStack commented at 12:38 pm on January 5, 2021: member

CI is failing now:

0In file included from net.cpp:10:
1./net.h:451:77: error: member access into incomplete type 'CNode'
2    size_t SocketSendData(CNode& pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode.cs_vSend);
3                                                                            ^
4./net.h:39:7: note: forward declaration of 'CNode'
5class CNode;
6      ^
71 error generated.

jnewbery force-pushed on Jan 5, 2021

jnewbery commented at 1:38 pm on January 5, 2021: member

ok, reverting to commit 378aedc which has two ACKs already. Any style issues can be fixed up in future PRs.

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

ACK 378aedc45248cea82d9a3e6dc1038d6828008a76

This is definitely the right way to fix this.

MarcoFalke merged this on Jan 6, 2021

MarcoFalke closed this on Jan 6, 2021

jnewbery deleted the branch on Jan 6, 2021

sidhujag referenced this in commit c1fd7a4a22 on Jan 6, 2021

DrahtBot locked this on Aug 16, 2022

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

Conflicts