refactor: various RecursiveMutex replacements in CConnman #22829

theStack · 2021-08-29T11:59:09Z

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.

DrahtBot · 2021-08-30T00:21:56Z

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

Code review ACK 94a99d0.

jonatack · 2021-09-06T08:37:22Z

Concept ACK and clang 13 debug build is clean.

maflcko · 2021-09-06T08:49:26Z

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?

maflcko · 2021-09-06T08:50:35Z

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

theStack · 2021-09-06T11:26:50Z

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 · 2021-09-06T14:57:07Z

Force-pushed with changed commit subject for the scripted-diff (using a helper), as suggested by MarcoFalke (#22829 (comment)):

$ git range-diff 94a99d05...6f0930a9
1:  cf29163ca ! 1:  887df8ecc scripted-diff: rename node vector/mutex members in CConnman
    @@ Commit message
         scripted-diff: rename node vector/mutex members in CConnman

         -BEGIN VERIFY SCRIPT-
    -    sed -i 's/cs_vAddedNodes/m_added_nodes_mutex/g' src/net.h src/net.cpp
    -    sed -i 's/vAddedNodes/m_added_nodes/g' src/net.h src/net.cpp
    -    sed -i 's/cs_vNodes/m_nodes_mutex/g' src/net.h src/net.cpp src/test/util/net.h
    -    sed -i 's/vNodesDisconnectedCopy/nodes_disconnected_copy/g' src/net.cpp
    -    sed -i 's/vNodesDisconnected/m_nodes_disconnected/g' src/net.h src/net.cpp
    -    sed -i 's/vNodesCopy/nodes_copy/g' src/net.cpp
    -    sed -i 's/vNodesSize/nodes_size/g' src/net.cpp
    -    sed -i 's/vNodes/m_nodes/g' src/net.h src/net.cpp src/test/util/net.h
    +
    +    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-

      ## src/net.cpp ##
2:  5aca12d16 = 2:  73685d923 refactor: replace RecursiveMutex m_addr_fetches_mutex with Mutex
3:  94a99d05f = 3:  6f0930a96 refactor: replace RecursiveMutex m_added_nodes_mutex with Mutex

vasild

ACK 6f0930a

Clang 12 build is clean with -Werror.

vasild · 2021-10-04T11:49:24Z

src/net.h

    mutable RecursiveMutex cs_totalBytesSent;
-    uint64_t nTotalBytesRecv GUARDED_BY(cs_totalBytesRecv) {0};
+    std::atomic<uint64_t> nTotalBytesRecv{0};


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

std::atomic_uint64_t

The two are guaranteed to be the same either way.

vasild · 2021-10-04T12:07:49Z

src/net.cpp

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


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:

- LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex); - if (m_addr_fetches.empty() && m_added_nodes.empty()) { - add_fixed_seeds_now = true; + { + LOCK2(m_addr_fetches_mutex, m_added_nodes_mutex); + add_fixed_seeds_now = m_addr_fetches.empty() && m_added_nodes.empty(); + } + if (add_fixed_seeds_now) { LogPrintf("Adding fixed seeds as -dnsseed=0, -addnode is not provided and all -see }

hebasto · 2021-10-08T14:23:07Z

Concept ACK.

hebasto

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

hebasto · 2021-10-08T14:43:16Z

src/net.h

+            LOCK(m_added_nodes_mutex);
+            m_added_nodes = connOptions.m_added_nodes;


nit, could be

Suggested change

LOCK(m_added_nodes_mutex);

m_added_nodes = connOptions.m_added_nodes;

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.

…stead 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.

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

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.

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.

vasild

ACK 3726a45

promag

Code review ACK 3726a45.

hebasto

re-ACK 3726a45

… CConnman

theStack force-pushed the 202108-refactor-recursive_mutex_replacements_in_cconnman branch 2 times, most recently from 829c50c to 94a99d0 Compare August 29, 2021 12:31

DrahtBot added P2P Refactoring labels Aug 29, 2021

This was referenced Aug 30, 2021

Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

Closed

refactor: wrap accept() and extend usage of Sock #21879

Merged

promag reviewed Sep 5, 2021

View reviewed changes

theStack force-pushed the 202108-refactor-recursive_mutex_replacements_in_cconnman branch 2 times, most recently from 6ce979e to 6f0930a Compare September 6, 2021 13:18

DrahtBot mentioned this pull request Sep 29, 2021

Make all networking code mockable #21878

Closed

12 tasks

vasild approved these changes Oct 4, 2021

View reviewed changes

hebasto approved these changes Oct 8, 2021

View reviewed changes

hebasto mentioned this pull request Oct 8, 2021

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

Open

36 tasks

DrahtBot added the Needs rebase label Nov 24, 2021

theStack added 4 commits November 24, 2021 19:19

theStack force-pushed the 202108-refactor-recursive_mutex_replacements_in_cconnman branch from 6f0930a to 3726a45 Compare November 24, 2021 18:52

DrahtBot removed the Needs rebase label Nov 24, 2021

vasild approved these changes Nov 25, 2021

View reviewed changes

promag reviewed Nov 25, 2021

View reviewed changes

hebasto approved these changes Nov 25, 2021

View reviewed changes

maflcko merged commit 76392b0 into bitcoin:master Nov 25, 2021

theStack deleted the 202108-refactor-recursive_mutex_replacements_in_cconnman branch November 25, 2021 11:00

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2021

Merge bitcoin#22829: refactor: various RecursiveMutex replacements in…

9fa45ab

… CConnman

bitcoin deleted a comment Nov 25, 2021

rebroad mentioned this pull request Nov 28, 2021

Code style PRs after v0.18 branch split #15465

Closed

bitcoin locked and limited conversation to collaborators Nov 25, 2022

		LOCK(m_added_nodes_mutex);
		m_added_nodes = connOptions.m_added_nodes;

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

refactor: various RecursiveMutex replacements in CConnman #22829

refactor: various RecursiveMutex replacements in CConnman #22829

Uh oh!

Conversation

theStack commented Aug 29, 2021

Uh oh!

DrahtBot commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Sep 6, 2021

Uh oh!

maflcko commented Sep 6, 2021

Uh oh!

maflcko commented Sep 6, 2021

Uh oh!

theStack commented Sep 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theStack commented Sep 6, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

maflcko Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

vasild Oct 4, 2021

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 8, 2021

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 8, 2021

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 30, 2021 •

edited

Loading

theStack commented Sep 6, 2021 •

edited

Loading