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

pull jnewbery wants to merge 2 commits into bitcoin:master from jnewbery:2020-12-cs-vsend changing 2 files +9 −13

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.

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 🔌

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash df2bf40ec7e87ed6608ff1eeaa002645df088c8901feef602a141b1ef0909d5f -

</details>

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:

In file included from net.cpp:10:
./net.h:451:77: error: member access into incomplete type 'CNode'
    size_t SocketSendData(CNode& pnode) const EXCLUSIVE_LOCKS_REQUIRED(pnode.cs_vSend);
                                                                            ^
./net.h:39:7: note: forward declaration of 'CNode'
class CNode;
      ^
1 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

Contributors

Labels

Linked (view graph)

#19673 Move cs_vSend into SocketSendData and resolve RecordBytesSent lock inconsistency

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

Conflicts