[net] Make cs_inventory nonrecursive

jnewbery commented at 1:23 pm on June 21, 2020: member

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 commented at 1:24 pm on June 21, 2020: member

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

fanquake added the label P2P on Jun 21, 2020

hebasto commented at 1:30 pm on June 21, 2020: member

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

in src/net.cpp:1108 in 8c372dd1cc outdated

1109-                        if (lockSend) {
1110-                            fDelete = true;
1111-                        }
1112+                    TRY_LOCK(pnode->cs_vSend, lockSend);
1113+                    if (lockSend) {
1114+                        fDelete = true;

hebasto commented at 3:38 pm on June 21, 2020:

From commit message

Therefore, cs_inventory can never be locked when the TRY_LOCK(cs_inventory) is reached in DisconnectNodes(). Remove the TRY_LOCK.

I understand that if (lockInv) is never true, and the code block TRY_LOCK(pnode->cs_vSend, lockSend); ... is dead in master. This commit makes this code always reachable, right?

MarcoFalke commented at 4:11 pm on June 21, 2020:

I think the comment says “Therefore, cs_inventory can never be locked by another thread …”, so it is always true (success).

jnewbery commented at 4:31 pm on June 21, 2020:

Sorry, you’re right that this commit message is unclear. Marco is correct that I meant “cs_inventory can never be locked by another thread”. I’ll update the commit message.

jnewbery commented at 4:46 pm on June 21, 2020:

I’ve made the commit log clearer.

jnewbery force-pushed on Jun 21, 2020

hebasto commented at 6:17 pm on June 21, 2020: member

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 approved

hebasto commented at 6:30 pm on June 21, 2020: member

nit: s/cs_inventory/m_inventory_mutex/

jnewbery commented at 6:36 pm on June 21, 2020: member

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

in src/net_processing.cpp:1333 in e335e4fcc8 outdated

1329@@ -1330,7 +1330,7 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB
1330         connman->ForEachNode([nNewHeight, &vHashes](CNode* pnode) {
1331             if (nNewHeight > (pnode->nStartingHeight != -1 ? pnode->nStartingHeight - 2000 : 0)) {
1332                 for (const uint256& hash : reverse_iterate(vHashes)) {
1333-                    pnode->PushBlockHash(hash);
1334+                    WITH_LOCK(pnode->cs_inventory, pnode->vBlockHashesToAnnounce.push_back(hash));

laanwj commented at 6:40 pm on June 22, 2020:

When doing this anyway, woulid it make sense to move this lock outside the (inner) loop? Re-taking it for every node × every block seems overkill.

jnewbery commented at 7:36 pm on June 22, 2020:

Good idea. Done.

promag commented at 9:26 am on June 23, 2020:

e335e4fcc881260906fb561f23a1b5017ed04d91

The suggestion was done in 6db15a8b79597a0f311490acd89ee50db298c6e8, but IMO should be in e335e4fcc881260906fb561f23a1b5017ed04d91.

jnewbery commented at 12:50 pm on June 23, 2020:

Done.

jnewbery force-pushed on Jun 22, 2020

hebasto commented at 9:03 pm on June 22, 2020: member

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

jnewbery commented at 9:44 pm on June 22, 2020: member

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.

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

344e831de5

[net] Make cs_inventory a non-recursive mutex

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

3556227ddd

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

e8a2822119

jnewbery force-pushed on Jun 23, 2020

jnewbery commented at 12:51 pm on June 23, 2020: member

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 commented at 6:33 pm on July 7, 2020: member

utACK e8a2822119233ade0de84f791a9e92918a3d6896

hebasto approved

hebasto commented at 4:53 pm on July 8, 2020: member

re-ACK e8a2822119233ade0de84f791a9e92918a3d6896

MarcoFalke commented at 7:57 pm on July 8, 2020: member

ACK e8a2822119233ade0de84f791a9e92918a3d6896 🍬

Signature:

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e8a2822119233ade0de84f791a9e92918a3d6896 🍬
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUgLcQv/SQmH5S8+LYeoatDRRklzvqBa7vDSX+1JFkyauaibxrGa2j0CT5pmbrQi
 8VBQSMZNrfC3F7g0hydzbplkkDzTciuj0rJGK6ScTmPLJA1X1ytCCoOjncCB+hz5O
 9L250NVZCJRsbESdtIzqfmJZcl/ANYbxeM9ZF+sgVFvG5tq+OMmHy1zWN6Yejxnxh
10vryK7Ke16Mny2QLmrgohTQTyYicHzX0wrKDV2v7J1DQL6TaN4nVAsmPV7eKbIAI6
11wTPl8msWKfknfrGU2SEurFEdfKN0IapV8KxM+trbjKpakIGcF3sFaP60cHzf1haG
12HjfckGlwx9hiYWs6d6RwM9mGI+t8k4UkdPbxrAwjW/OnK0jAw6Zp/NOJK7VX19Em
13DcoKZXMHmrdNvIV5sxxXkGJtEQJsknuyIyjmbOB9HezJG+obnSNGuMvdqF7WLV20
14UQpV+WhV07LlYz4akXZ/SGsDvRJkFzIskaIc4u2tKVVHM/HIHjsrB7Vy2DTd8A4J
15XLhWoDVu
16=9k86
17-----END PGP SIGNATURE-----

Timestamp of file with hash 19ea9d208bf92e8a00c6a74b291941159f29e27a9ad985a83fc50fb9e7eeb14a -

MarcoFalke merged this on Jul 8, 2020

MarcoFalke closed this on Jul 8, 2020

jnewbery deleted the branch on Jul 8, 2020

sidhujag referenced this in commit 2d4ee79def on Jul 9, 2020

Fabcien referenced this in commit 8ef7cf94d1 on Apr 20, 2021

DrahtBot locked this on Feb 15, 2022

[net] Make cs_inventory nonrecursive #19347