refactor: various RecursiveMutex replacements in CConnman

theStack commented at 11:59 am on August 29, 2021: member

This PR is related to #19303 and gets rid of the following RecursiveMutex members in class CConnman:

for cs_totalBytesRecv, protecting nTotalBytesRecv, std::atomic is used instead (the member is only increment at one and read at another place, so this is sufficient)
for m_addr_fetches_mutex, protecting m_addr_fetches, a regular Mutex is used instead (there is no chance that within one critical section, another one is called)
for cs_vAddedNodes, protecting vAddedNodes, a regular Mutex is used instead (there is no chance that within one critical section, another one is called)

Additionally, the PR takes the chance to rename all node vector members (vNodes, vAddedNodes) and its corresponding mutexes (cs_vNodes, cs_vAddedNodes) to match the coding guidelines via a scripted-diff.

theStack force-pushed on Aug 29, 2021

DrahtBot added the label P2P on Aug 29, 2021

DrahtBot added the label Refactoring on Aug 29, 2021

DrahtBot commented at 0:21 am on August 30, 2021: 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:

#21943 (Dedup and RAII-fy the creation of a copy of CConnman::vNodes by vasild)
#21879 (Wrap accept() and extend usage of Sock by vasild)
#21878 (Make all networking code mockable by vasild)

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.

promag commented at 5:42 pm on September 5, 2021: member

Code review ACK 94a99d05f59c6163e4d22665559567b45f28a23c.

jonatack commented at 8:37 am on September 6, 2021: member

Concept ACK and clang 13 debug build is clean.

MarcoFalke commented at 8:49 am on September 6, 2021: member

there is no chance that within one critical section, another one is called

why? What prevents ForEachNode to call DisconnectNode or so via the lambda?

MarcoFalke commented at 8:50 am on September 6, 2021: member

The scripted diff could be shortened with a rename helper like 37dcd12d539e4a875581fa049aa0f7fafeb932a4

theStack commented at 11:26 am on September 6, 2021: member

there is no chance that within one critical section, another one is called

why? What prevents ForEachNode to call DisconnectNode or so via the lambda?

What concrete mutex are you referring to? Note that this PR only tackles the RecursiveMutex->Mutex replacement for vAddedNodes_cs (which is only locked in a small number of places, see commit log message for details), not vNodes_cs, which is a way more complex beast to beat.

The scripted diff could be shortened with a rename helper like 37dcd12

Good idea, will do in a bit.

theStack force-pushed on Sep 6, 2021

theStack commented at 2:57 pm on September 6, 2021: member

Force-pushed with changed commit subject for the scripted-diff (using a helper), as suggested by MarcoFalke (https://github.com/bitcoin/bitcoin/pull/22829#issuecomment-913468228):

 0$ git range-diff 94a99d05...6f0930a9
 11:  cf29163ca ! 1:  887df8ecc scripted-diff: rename node vector/mutex members in CConnman
 2    @@ Commit message
 3         scripted-diff: rename node vector/mutex members in CConnman
 4
 5         -BEGIN VERIFY SCRIPT-
 6    -    sed -i 's/cs_vAddedNodes/m_added_nodes_mutex/g' src/net.h src/net.cpp
 7    -    sed -i 's/vAddedNodes/m_added_nodes/g' src/net.h src/net.cpp
 8    -    sed -i 's/cs_vNodes/m_nodes_mutex/g' src/net.h src/net.cpp src/test/util/net.h
 9    -    sed -i 's/vNodesDisconnectedCopy/nodes_disconnected_copy/g' src/net.cpp
10    -    sed -i 's/vNodesDisconnected/m_nodes_disconnected/g' src/net.h src/net.cpp
11    -    sed -i 's/vNodesCopy/nodes_copy/g' src/net.cpp
12    -    sed -i 's/vNodesSize/nodes_size/g' src/net.cpp
13    -    sed -i 's/vNodes/m_nodes/g' src/net.h src/net.cpp src/test/util/net.h
14    +
15    +    ren() { sed -i "s/$1/$2/g" $3 $4 $5; }
16    +
17    +    ren cs_vAddedNodes         m_added_nodes_mutex     src/net.h src/net.cpp
18    +    ren vAddedNodes            m_added_nodes           src/net.h src/net.cpp
19    +    ren cs_vNodes              m_nodes_mutex           src/net.h src/net.cpp src/test/util/net.h
20    +    ren vNodesDisconnectedCopy nodes_disconnected_copy src/net.cpp
21    +    ren vNodesDisconnected     m_nodes_disconnected    src/net.h src/net.cpp
22    +    ren vNodesCopy             nodes_copy              src/net.cpp
23    +    ren vNodesSize             nodes_size              src/net.cpp
24    +    ren vNodes                 m_nodes                 src/net.h src/net.cpp src/test/util/net.h
25    +
26         -END VERIFY SCRIPT-
27
28      ## src/net.cpp ##
292:  5aca12d16 = 2:  73685d923 refactor: replace RecursiveMutex m_addr_fetches_mutex with Mutex
303:  94a99d05f = 3:  6f0930a96 refactor: replace RecursiveMutex m_added_nodes_mutex with Mutex

in src/net.h:1078 in 6f0930a965 outdated

1029@@ -1030,9 +1030,8 @@ class CConnman
1030     static bool NodeFullyConnected(const CNode* pnode);
1031 
1032     // Network usage totals
1033-    mutable RecursiveMutex cs_totalBytesRecv;
1034     mutable RecursiveMutex cs_totalBytesSent;
1035-    uint64_t nTotalBytesRecv GUARDED_BY(cs_totalBytesRecv) {0};
1036+    std::atomic<uint64_t> nTotalBytesRecv{0};

vasild commented at 11:49 am on October 4, 2021:

nit: might use std::atomic_uint64_t if it is available on all platforms that we aim to support.

MarcoFalke commented at 12:52 pm on October 4, 2021:

std::atomic_uint64_t

The two are guaranteed to be the same either way.

in src/net.cpp:1918 in 6f0930a965 outdated

1881-                if (m_addr_fetches.empty() && vAddedNodes.empty()) {
1882+                LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
1883+                if (m_addr_fetches.empty() && m_added_nodes.empty()) {
1884                     add_fixed_seeds_now = true;
1885                     LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -seednode(s) attempted\n");
1886                 }

vasild commented at 12:07 pm on October 4, 2021:

Not necessary for this PR, but it is a good practice to avoid printouts (which may block) while holding mutexes. Feel free to take this or ignore it:

0-                LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
1-                if (m_addr_fetches.empty() && m_added_nodes.empty()) {
2-                    add_fixed_seeds_now = true;
3+                {
4+                    LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex);
5+                    add_fixed_seeds_now = m_addr_fetches.empty() && m_added_nodes.empty();
6+                }
7+                if (add_fixed_seeds_now) {
8                     LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -see
9                 }

vasild approved

vasild commented at 12:45 pm on October 4, 2021: member

ACK 6f0930a965fcdffcb1b9e18d431ca73c5cd1a285

Clang 12 build is clean with -Werror.

hebasto commented at 2:23 pm on October 8, 2021: member

Concept ACK.

in src/net.h:795 in 6f0930a965 outdated

792@@ -793,8 +793,8 @@ class CConnman
793         }
794         vWhitelistedRange = connOptions.vWhitelistedRange;
795         {
796-            LOCK(cs_vAddedNodes);
797-            vAddedNodes = connOptions.m_added_nodes;
798+            LOCK(m_added_nodes_mutex);
799+            m_added_nodes = connOptions.m_added_nodes;

hebasto commented at 2:43 pm on October 8, 2021:

nit, could be

0            WITH_LOCK(m_added_nodes_mutex, m_added_nodes = connOptions.m_added_nodes);

without surrounding braces.

Mayby, better to leave it (and other similar places) for a follow up.

hebasto approved

hebasto commented at 2:47 pm on October 8, 2021: member

ACK 6f0930a965fcdffcb1b9e18d431ca73c5cd1a285, I have reviewed the code, and verified that there are no ways to lock the mentioned mutexes recursivly.

DrahtBot added the label Needs rebase on Nov 24, 2021

refactor: remove RecursiveMutex cs_totalBytesRecv, use std::atomic instead

The RecursiveMutex cs_totalBytesRecv is only used at two places: in
CConnman::RecordBytesRecv() to increment the nTotalBytesRecv member, and in
CConnman::GetTotalBytesRecv() to read it. For this simple use-case, we can
make the member std::atomic instead to achieve the same result.

574cc4271a

scripted-diff: rename node vector/mutex members in CConnman

-BEGIN VERIFY SCRIPT-

ren() { sed -i "s/$1/$2/g" $3 $4 $5; }

ren cs_vAddedNodes         m_added_nodes_mutex     src/net.h src/net.cpp
ren vAddedNodes            m_added_nodes           src/net.h src/net.cpp
ren cs_vNodes              m_nodes_mutex           src/net.h src/net.cpp src/test/util/net.h
ren vNodesDisconnectedCopy nodes_disconnected_copy src/net.cpp
ren vNodesDisconnected     m_nodes_disconnected    src/net.h src/net.cpp
ren vNodesCopy             nodes_copy              src/net.cpp
ren vNodesSize             nodes_size              src/net.cpp
ren vNodes                 m_nodes                 src/net.h src/net.cpp src/test/util/net.h

-END VERIFY SCRIPT-

d51d2a3bb5

refactor: replace RecursiveMutex m_addr_fetches_mutex with Mutex

The RecursiveMutex m_addr_fetches_mutex is used at three places:
    - CConnman::AddAddrFetch()
    - CConnman::ProcessAddrFetch()
    - CConnman::ThreadOpenConnections()
In each of the critical sections, only the the m_addr_fetches is accessed
(and in the last case, also vAddedNodes), without any chance that within
one section another one is called. Hence, we can use an ordinary Mutex
instead of RecursiveMutex.

7d52ff5c38

refactor: replace RecursiveMutex m_added_nodes_mutex with Mutex

The RecursiveMutex m_added_nodes_mutex is used at three places:
    - CConnman::GetAddedNodeInfo()
    - CConnman::AddNode()
    - CConnman::ThreadOpenConnections()
In each of the critical sections, only the the m_added_nodes member is
accessed (and in the last case, also m_addr_fetches), without any chance
that within one section another one is called. Hence, we can use an
ordinary Mutex instead of RecursiveMutex.

3726a45958

theStack force-pushed on Nov 24, 2021

DrahtBot removed the label Needs rebase on Nov 24, 2021

vasild approved

vasild commented at 9:14 am on November 25, 2021: member

ACK 3726a4595837b66d37f151faf1cec2796d6b74d7

promag commented at 9:55 am on November 25, 2021: member

Code review ACK 3726a4595837b66d37f151faf1cec2796d6b74d7.

hebasto approved

hebasto commented at 10:28 am on November 25, 2021: member

re-ACK 3726a4595837b66d37f151faf1cec2796d6b74d7

MarcoFalke merged this on Nov 25, 2021

MarcoFalke closed this on Nov 25, 2021

theStack deleted the branch on Nov 25, 2021

sidhujag referenced this in commit 9fa45ab326 on Nov 25, 2021

fanquake deleted a comment on Nov 25, 2021

DrahtBot locked this on Nov 25, 2022

refactor: various RecursiveMutex replacements in CConnman #22829

Conflicts