[net] Make cs_inventory nonrecursive by jnewbery · Pull Request #19347 · bitcoin/bitcoin

jnewbery · 2020-06-21T13:23:53Z

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 · 2020-06-21T13:24:33Z

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

hebasto · 2020-06-21T13:30:17Z

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

src/net.cpp

hebasto

ACK eaced81501f035193523d1abde4569dfc73ae327, 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 · 2020-06-21T18:30:04Z

nit: s/cs_inventory/m_inventory_mutex/

jnewbery · 2020-06-21T18:36:29Z

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

src/net_processing.cpp

hebasto · 2020-06-22T21:03:52Z

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

jnewbery · 2020-06-22T21:44:29Z

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.

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.

cs_inventory is never taken recursively. Make it a non-recursive mutex.

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 · 2020-06-23T12:51:14Z

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 · 2020-07-07T18:33:46Z

utACK e8a2822

hebasto

re-ACK e8a2822

maflcko · 2020-07-08T19:57:06Z

ACK e8a2822 🍬

Show signature and timestamp

Signature:

Timestamp of file with hash 19ea9d208bf92e8a00c6a74b291941159f29e27a9ad985a83fc50fb9e7eeb14a -

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

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

fanquake added the P2P label Jun 21, 2020

hebasto mentioned this pull request Jun 21, 2020

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

35 tasks

hebasto reviewed Jun 21, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-06-cs-inventory branch from 8c372dd to eaced81 Compare June 21, 2020 16:45

hebasto reviewed Jun 21, 2020

View reviewed changes

hebasto approved these changes Jun 21, 2020

View reviewed changes

laanwj reviewed Jun 22, 2020

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jnewbery force-pushed the 2020-06-cs-inventory branch from eaced81 to 6db15a8 Compare June 22, 2020 19:36

jnewbery added 3 commits June 23, 2020 08:46

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

[net] Make cs_inventory a non-recursive mutex

3556227

cs_inventory is never taken recursively. Make it a non-recursive mutex.

jnewbery force-pushed the 2020-06-cs-inventory branch from 6db15a8 to e8a2822 Compare June 23, 2020 12:50

hebasto approved these changes Jul 8, 2020

View reviewed changes

maflcko merged commit 9f4c0a9 into bitcoin:master Jul 8, 2020

jnewbery deleted the 2020-06-cs-inventory branch July 8, 2020 20:01

hebasto mentioned this pull request Sep 5, 2020

net processing: Move block inventory state to net_processing #19829

Merged

vasild mentioned this pull request Apr 22, 2021

net: remove unnecessary check of CNode::cs_vSend #21750

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[net] Make cs_inventory nonrecursive#19347

[net] Make cs_inventory nonrecursive#19347
maflcko merged 3 commits intobitcoin:masterfrom
jnewbery:2020-06-cs-inventory

jnewbery commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 21, 2020

Uh oh!

hebasto commented Jun 21, 2020

Uh oh!

Uh oh!

hebasto left a comment

Uh oh!

hebasto commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 21, 2020

Uh oh!

Uh oh!

hebasto commented Jun 22, 2020

Uh oh!

jnewbery commented Jun 22, 2020

Uh oh!

jnewbery commented Jun 23, 2020

Uh oh!

sipa commented Jul 7, 2020

Uh oh!

hebasto left a comment

Uh oh!

maflcko commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

jnewbery commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 21, 2020

Uh oh!

hebasto commented Jun 21, 2020

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 21, 2020

Uh oh!

jnewbery commented Jun 21, 2020

Uh oh!

Uh oh!

hebasto commented Jun 22, 2020

Uh oh!

jnewbery commented Jun 22, 2020

Uh oh!

jnewbery commented Jun 23, 2020

Uh oh!

sipa commented Jul 7, 2020

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jul 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants