Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 2 commits into from Jan 6, 2021
Merged

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

merged 2 commits into from Jan 6, 2021

Conversation

jnewbery
Copy link
Contributor

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.

@jnewbery jnewbery mentioned this pull request Dec 31, 2020
Closed
@fanquake fanquake added the P2P label Dec 31, 2020
@maflcko
Copy link
Member

maflcko commented Dec 31, 2020

Concept ACK. Will review next year.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 31, 2020 •
edited

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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
Copy link
Contributor

Concept ACK

Thanks for improving locking!

@DrahtBot DrahtBot mentioned this pull request Dec 31, 2020
Merged
theStack
theStack approved these changes Jan 2, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a06c1ed 🔒

src/net.cpp Outdated Show resolved Hide resolved
theStack
theStack approved these changes Jan 2, 2021
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK 378aedc

maflcko
maflcko approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

review ACK 378aedc 🔌

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK 378aedc45248cea82d9a3e6dc1038d6828008a76 🔌
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUh/bAwAlAViNkPZb/+MFuod/2ZmdPPu+12UtIpBh3oTcEKw+G/wdVHJ7fCPI7Tv
yvd8cPzeOKp3dXSqsuTu6UOHJ9+mcRodOkiu9F6smvgo0NLIC+DKYFUoRD8ywaEN
2GhjFdiIKJoaDPRLM+YA3FEVbJG1oxqUX6ifnlk/Zyh+/zCLd5l4S3GqcLQmhyJ/
4guvd1eq9a4H1apiGEop3j75Xm+jFvou07WF31RJgJgmP9L3WELsYItcpLe7+Gar
nVhL6amBwh4Ctt2DYcggNhP3mk/a6bOSvYVBDOAlfya4SOlg1weBXwjU1o1jpv2C
X5HgiToUfl4UzlYoFz7kxPoFJHCMhgaTadL23IiJpf5Tm/nNX/NVcJbTf5+WRUfd
ns/enM8FESK7cp4z4qwnOk1EY5tfHhrGTnXx36GaVyABggL/TaObMxrGZbxjBiPx
T8wByTCoTUdpXHjtqloT3AzLoJq8uTRJQlyd5WyqPjQbn75hcBij1JmprCD6KoUo
CAf3cVsZ
=tcoi
-----END PGP SIGNATURE-----

Timestamp of file with hash df2bf40ec7e87ed6608ff1eeaa002645df088c8901feef602a141b1ef0909d5f -

src/net.cpp Show resolved Hide resolved
src/net.cpp Show resolved Hide resolved
@maflcko
Copy link
Member

maflcko commented Jan 5, 2021

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

@jnewbery
Copy link
Contributor Author

jnewbery commented Jan 5, 2021

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
Copy link
Contributor Author

jnewbery commented Jan 5, 2021

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

@theStack
Copy link
Contributor

theStack commented Jan 5, 2021

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
Copy link
Contributor Author

jnewbery commented Jan 5, 2021

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

@troygiorshev
Copy link
Contributor

ACK 378aedc

This is definitely the right way to fix this.

@maflcko maflcko merged commit 4eada5d into bitcoin:master Jan 6, 2021
@jnewbery jnewbery deleted the 2020-12-cs-vsend branch January 6, 2021 09:42
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 6, 2021
Merge bitcoin#20816: net: Move RecordBytesSent() call out of cs_vSend…
c1fd7a4 
… 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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@theStack theStack theStack approved these changes

@maflcko maflcko maflcko approved these changes

Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jnewbery @maflcko @DrahtBot @practicalswift @theStack @troygiorshev @fanquake