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.

Sign up for GitHub

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] Make cs_inventory nonrecursive #19347

Merged
merged 3 commits into from
Jul 8, 2020
Merged

[net] Make cs_inventory nonrecursive #19347

merged 3 commits into from
Jul 8, 2020

Conversation

jnewbery
Copy link
Contributor

  • Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
  • Make cs_inventory a nonrecursive mutex
  • Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.

@jnewbery
Copy link
Contributor Author

@hebasto : you seem to be on a mutex-cleaning spree. Perhaps you'd be interested in this?

@fanquake fanquake added the P2P label Jun 21, 2020
@hebasto hebasto mentioned this pull request Jun 21, 2020
Open
36 tasks
@hebasto
Copy link
Member

hebasto commented Jun 21, 2020

Concept ACK. Made a link here from the tracking issue #19303.

hebasto
hebasto reviewed Jun 21, 2020
View reviewed changes
src/net.cpp Show resolved Hide resolved
@jnewbery jnewbery force-pushed the 2020-06-cs-inventory branch from 8c372dd to eaced81 Compare June 21, 2020 16:45
hebasto
hebasto reviewed Jun 21, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK eaced81, I've verified that mutex does not get locked recursively. Also tested on Linux Mint 20 (x86_64) with the double_lock_detected() function from #19337.

For safety in the future I suggest to merge this PR on top of the #19337 which preserves UB in case of any recursive locking.

@hebasto
Copy link
Member

hebasto commented Jun 21, 2020

nit: s/cs_inventory/m_inventory_mutex/

@jnewbery
Copy link
Contributor Author

nit: s/cs_inventory/m_inventory_mutex

I agree! I'm planning to move this mutex into net processing where it belongs. I'll do the rename at the same time. See https://github.com/jnewbery/bitcoin/commits/2020-06-cs-main-split

laanwj
laanwj reviewed Jun 22, 2020
View reviewed changes
src/net_processing.cpp Outdated Show resolved Hide resolved
@jnewbery jnewbery force-pushed the 2020-06-cs-inventory branch from eaced81 to 6db15a8 Compare June 22, 2020 19:36
@hebasto
Copy link
Member

hebasto commented Jun 22, 2020

Does the recent change decrease concurrentness of the code? It seems like a premature optimization a bit, no?

@jnewbery
Copy link
Contributor Author

Does the recent change decrease concurrentness of the code?

I think it's fine to expand the lock scope:

  • it's extremely unlikely that this'll increase contention for this lock. UpdatedBlockTip() is only called once every ten minutes on average, it's very unlikely that it'll push two block hashes onto the vBlockHashesToAnnounce vector (only if there's a re-org), and the time it takes to push an extra hash onto the vector is tiny.
  • in the very unlikely case we do block the other thread, it's probably an improvement. Rather than sending one hash in a headers message to the peer, and then another hash in a second headers message, we'll send both in the same message.

jnewbery added 3 commits June 23, 2020 08:46
@jnewbery
[net processing] Remove PushBlockInventory and PushBlockHash
344e831 
PushBlockInventory() and PushBlockHash() are functions that can
be replaced with single-line statements. This also eliminates
the single place that cs_inventory is taken recursively.
@jnewbery
[net] Make cs_inventory a non-recursive mutex
3556227 
cs_inventory is never taken recursively. Make it a non-recursive mutex.
@jnewbery
[net] Don't try to take cs_inventory before deleting CNode
e8a2822 
The TRY_LOCK(cs_inventory) in DisconnectNodes() is taken after the CNode
object has been removed from vNodes and when the CNode's nRefCount is
zero.

The only other places that cs_inventory can be taken are:

- In ProcessMessages() or SendMessages(), when the CNode's nRefCount
must be >0 (see ThreadMessageHandler(), where the refcount is
incremented before calling ProcessMessages() and SendMessages()).
- In a ForEachNode() lambda in PeerLogicValidation::UpdatedBlockTip().
ForEachNode() locks cs_vNodes and calls the function on the CNode
objects in vNodes.

Therefore, cs_inventory is never locked by another thread when the
TRY_LOCK(cs_inventory) is reached in DisconnectNodes(). Since the
only purpose of this TRY_LOCK is to ensure that the lock is not
taken by another thread, this always succeeds. Remove the check.
@jnewbery jnewbery force-pushed the 2020-06-cs-inventory branch from 6db15a8 to e8a2822 Compare June 23, 2020 12:50
@jnewbery
Copy link
Contributor Author

I've moved the lock scope change to the correct commit, and also moved it one line up, outside the surrounding if block. It doesn't make any difference here, but makes a commit in a future branch tidier.

@sipa
Copy link
Member

sipa commented Jul 7, 2020

utACK e8a2822

hebasto
hebasto approved these changes Jul 8, 2020
View reviewed changes
Copy link
Member

@hebasto hebasto 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 e8a2822

@maflcko
Copy link
Member

maflcko commented Jul 8, 2020

ACK e8a2822 🍬

Show signature and timestamp

Signature:

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

ACK e8a2822119233ade0de84f791a9e92918a3d6896 🍬
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgLcQv/SQmH5S8+LYeoatDRRklzvqBa7vDSX+1JFkyauaibxrGa2j0CT5pmbrQi
VBQSMZNrfC3F7g0hydzbplkkDzTciuj0rJGK6ScTmPLJA1X1ytCCoOjncCB+hz5O
L250NVZCJRsbESdtIzqfmJZcl/ANYbxeM9ZF+sgVFvG5tq+OMmHy1zWN6Yejxnxh
vryK7Ke16Mny2QLmrgohTQTyYicHzX0wrKDV2v7J1DQL6TaN4nVAsmPV7eKbIAI6
wTPl8msWKfknfrGU2SEurFEdfKN0IapV8KxM+trbjKpakIGcF3sFaP60cHzf1haG
HjfckGlwx9hiYWs6d6RwM9mGI+t8k4UkdPbxrAwjW/OnK0jAw6Zp/NOJK7VX19Em
DcoKZXMHmrdNvIV5sxxXkGJtEQJsknuyIyjmbOB9HezJG+obnSNGuMvdqF7WLV20
UQpV+WhV07LlYz4akXZ/SGsDvRJkFzIskaIc4u2tKVVHM/HIHjsrB7Vy2DTd8A4J
XLhWoDVu
=9k86
-----END PGP SIGNATURE-----

Timestamp of file with hash 19ea9d208bf92e8a00c6a74b291941159f29e27a9ad985a83fc50fb9e7eeb14a -

@maflcko maflcko merged commit 9f4c0a9 into bitcoin:master Jul 8, 2020
@jnewbery jnewbery deleted the 2020-06-cs-inventory branch July 8, 2020 20:01
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 9, 2020
Merge bitcoin#19347: [net] Make cs_inventory nonrecursive
2d4ee79 
e8a2822 [net] Don't try to take cs_inventory before deleting CNode (John Newbery)
3556227 [net] Make cs_inventory a non-recursive mutex (John Newbery)
344e831 [net processing] Remove PushBlockInventory and PushBlockHash (John Newbery)

Pull request description:

  - Remove PushBlockInventory() and PushBlockHash(). These are one-line functions that can easy be inlined into the calling code. Doing so also allows us to eliminate the one place that cs_inventory is recursively locked.
  - Make cs_inventory a nonrecursive mutex
  - Remove a redundant TRY_LOCK of cs_inventory when deleting CNode.

ACKs for top commit:
  sipa:
    utACK e8a2822
  MarcoFalke:
    ACK e8a2822 🍬
  hebasto:
    re-ACK e8a2822

Tree-SHA512: dbc721d102cdef7b5827a8f2549daf8b54f543050266999a7ea56c9f36618565b71e31ce0beb1209ba2db43d15388be173355a03fb6db8ad24e2475b145050bd
@hebasto hebasto mentioned this pull request Sep 5, 2020
Merged
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Apr 20, 2021
@jnewbery @PiRK
[net] Make cs_inventory nonrecursive
8ef7cf9 
Summary:
> [net processing] Remove PushBlockInventory and PushBlockHash
>
> PushBlockInventory() and PushBlockHash() are functions that can
> be replaced with single-line statements. This also eliminates
> the single place that cs_inventory is taken recursively.

> [net] Make cs_inventory a non-recursive mutex
>
> cs_inventory is never taken recursively. Make it a non-recursive mutex.

> [net] Don't try to take cs_inventory before deleting CNode
>
> The TRY_LOCK(cs_inventory) in DisconnectNodes() is taken after the CNode
> object has been removed from vNodes and when the CNode's nRefCount is
> zero.
>
> The only other places that cs_inventory can be taken are:
>
> - In ProcessMessages() or SendMessages(), when the CNode's nRefCount
> must be >0 (see ThreadMessageHandler(), where the refcount is
> incremented before calling ProcessMessages() and SendMessages()).
> - In a ForEachNode() lambda in PeerLogicValidation::UpdatedBlockTip().
> ForEachNode() locks cs_vNodes and calls the function on the CNode
> objects in vNodes.
>
> Therefore, cs_inventory is never locked by another thread when the
> TRY_LOCK(cs_inventory) is reached in DisconnectNodes(). Since the
> only purpose of this TRY_LOCK is to ensure that the lock is not
> taken by another thread, this always succeeds. Remove the check.

This is a backport of [[bitcoin/bitcoin#19347 | core#19347]]

Test Plan: `ninja all check-all`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D9433
@vasild vasild mentioned this pull request Apr 22, 2021
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@maflcko maflcko maflcko left review comments

@promag promag promag left review comments

@laanwj laanwj laanwj left review comments

@hebasto hebasto hebasto 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.
7 participants
@jnewbery @hebasto @sipa @maflcko @laanwj @promag @fanquake