p2p: Replace RecursiveMutex `cs_totalBytesSent` with Mutex and rename it #24157

w0xlt · 2022-01-25T22:21:00Z

Related to #19303, this PR gets rid of the RecursiveMutex cs_totalBytesSent and also adds AssertLockNotHeld macros combined with LOCKS_EXCLUDED thread safety annotations to avoid recursive locking.

src/net.h

w0xlt · 2022-02-10T18:57:06Z

Addressed @hebasto suggestion.

All functions changed in the second commit use EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex) instead of LOCKS_EXCLUDED(m_total_bytes_sent_mutex).

The only exception is void RecordBytesSent(uint64_t bytes) which displayed the error below when using EXCLUSIVE_LOCKS_REQUIRED.

Not sure about the reason for this error.

net.cpp:1655:29: error: calling function 'RecordBytesSent' requires negative capability '!m_total_bytes_sent_mutex' [-Werror,-Wthread-safety-analysis]
            if (bytes_sent) RecordBytesSent(bytes_sent);
                            ^
net.cpp:3077:21: error: calling function 'RecordBytesSent' requires negative capability '!m_total_bytes_sent_mutex' [-Werror,-Wthread-safety-analysis]
    if (nBytesSent) RecordBytesSent(nBytesSent);

hebasto · 2022-02-10T19:51:48Z

@w0xlt

Not sure about the reason for this error.

Maybe:

--- a/src/net.h
+++ b/src/net.h
@@ -829,7 +829,8 @@ public:
 
     bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
 
-    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
+    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     using NodeFn = std::function<void(CNode*)>;
     void ForEachNode(const NodeFn& func)
@@ -1024,7 +1025,8 @@ private:
     /**
      * Check connected and listening sockets for IO readiness and process them accordingly.
      */
-    void SocketHandler();
+    void SocketHandler()
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Do the read/write for connected sockets that are ready for IO.
@@ -1037,7 +1039,8 @@ private:
     void SocketHandlerConnected(const std::vector<CNode*>& nodes,
                                 const std::set<SOCKET>& recv_set,
                                 const std::set<SOCKET>& send_set,
-                                const std::set<SOCKET>& error_set);
+                                const std::set<SOCKET>& error_set)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Accept incoming connections, one from each read-ready listening socket.
@@ -1045,7 +1048,8 @@ private:
      */
     void SocketHandlerListening(const std::set<SOCKET>& recv_set);
 
-    void ThreadSocketHandler();
+    void ThreadSocketHandler()
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
     void ThreadDNSAddressSeed();
 
     uint64_t CalculateKeyedNetGroup(const CAddress& ad) const;
@@ -1074,7 +1078,8 @@ private:
 
     // Network stats
     void RecordBytesRecv(uint64_t bytes);
-    void RecordBytesSent(uint64_t bytes) LOCKS_EXCLUDED(m_total_bytes_sent_mutex);
+    void RecordBytesSent(uint64_t bytes)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 
     /**
      * Return vector of current BLOCK_RELAY peers.

?

w0xlt · 2022-02-10T22:45:41Z

@hebasto Thanks. This worked.

Very interesting. So, using EXCLUSIVE_LOCKS_REQUIRED(!mutex) requires that all functions that call the annotated function also have the same annotation. It is a cascading effect.

hebasto · 2022-02-11T12:30:05Z

@hebasto Thanks. This worked.

Very interesting. So, using EXCLUSIVE_LOCKS_REQUIRED(!mutex) requires that all functions that call the annotated function also have the same annotation. It is a cascading effect.

That is why negative capabilities provide a stronger safety guarantee.

I don't see any reason of splitting changes between second and third commits. Maybe squash them?

w0xlt · 2022-02-11T19:37:13Z

Maybe squash them?

Sure. Done.

hebasto

ACK 0b6c5da9304af71dedb262bca9a0493c8616985b

src/net.h

DrahtBot · 2022-02-13T14:19:45Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24931 (Strengthen thread safety assertions by ajtowns)
#24356 (refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() 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.

hebasto

re-ACK f04ffdd2a79a77ea70d892be972de7821c1b876c

src/net.cpp

src/net.h

w0xlt · 2022-02-14T19:15:24Z

d370938 addresses @vasild suggestions.

vasild

ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739

hebasto

~~re-ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739~~, only suggested changes since my previous review.

UPDATE: ACK retracted. See #24157 (comment)

w0xlt · 2022-04-12T17:59:49Z

Is any further action needed in this PR?

ajtowns

Think there's a missing annotation, but otherwise looks good.

src/net.h

jonatack · 2022-04-15T11:47:49Z

ACK modulo the missing annotation

Verified the methods updated here have both an annotation in the declaration and a corresponding assertion in the definition:

GetMaxOutboundTarget
GetMaxOutboundTimeLeftInCycle
GetMaxOutboundTimeLeftInCycle_
GetOutboundTargetBytesLeft
GetTotalBytesSent
OutboundTargetReached
PushMessage
RecordBytesSent
SocketHandler
SocketHandlerConnected
ThreadSocketHandler

-BEGIN VERIFY SCRIPT- sed -i 's/cs_totalBytesSent/m_total_bytes_sent_mutex/g' -- $(git grep --files-with-matches 'cs_totalBytesSent') -END VERIFY SCRIPT-

src/net.h

vasild

ACK eff791863da5dd1fcda7bddc7d729a63d772659d

Compiles with clang 15.

…nt_mutex` Co-authored-by: Hennadii Stepanov <[email protected]>

w0xlt · 2022-04-22T08:46:16Z

Added the suggestion #24157 (comment)

vasild

ACK 709af67

hebasto

ACK 709af67, tested on Ubuntu 22.04.

jonatack · 2022-04-22T10:11:30Z

ACK 709af67 per git range-diff 7a4ac71 eff7918 709af67, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative

…th Mu…

w0xlt marked this pull request as draft January 25, 2022 22:32

DrahtBot added the P2P label Jan 25, 2022

w0xlt force-pushed the cs_totalBytesSent_mutex branch from 756e882 to 6cb0fab Compare January 26, 2022 03:25

w0xlt marked this pull request as ready for review January 26, 2022 06:06

hebasto reviewed Feb 2, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

w0xlt force-pushed the cs_totalBytesSent_mutex branch from 45e2f9b to 378facf Compare February 10, 2022 15:57

w0xlt marked this pull request as draft February 10, 2022 16:12

w0xlt force-pushed the cs_totalBytesSent_mutex branch from 378facf to 6a3eeda Compare February 10, 2022 17:50

w0xlt marked this pull request as ready for review February 10, 2022 18:40

w0xlt force-pushed the cs_totalBytesSent_mutex branch from 6a3eeda to ebf1f2c Compare February 10, 2022 21:51

w0xlt force-pushed the cs_totalBytesSent_mutex branch from ebf1f2c to 0b6c5da Compare February 11, 2022 13:23

hebasto approved these changes Feb 12, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

w0xlt force-pushed the cs_totalBytesSent_mutex branch from 0b6c5da to f04ffdd Compare February 13, 2022 19:50

hebasto approved these changes Feb 13, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 14, 2022

Make all networking code mockable #21878

Closed

12 tasks

vasild reviewed Feb 14, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

w0xlt force-pushed the cs_totalBytesSent_mutex branch from f04ffdd to eef2c7b Compare February 14, 2022 17:20

DrahtBot mentioned this pull request Feb 17, 2022

refactor: replace CConnman::SocketEvents() with mockable Sock::WaitMany() #24356

Merged

vasild approved these changes Feb 18, 2022

View reviewed changes

hebasto approved these changes Feb 20, 2022

View reviewed changes

DrahtBot mentioned this pull request Mar 12, 2022

BIP324: Enable v2 P2P encrypted transport #24545

Closed

DrahtBot mentioned this pull request Apr 5, 2022

test/BIP324: functional tests for v2 P2P encryption #24748

Merged

ajtowns reviewed Apr 13, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex

a237a06

-BEGIN VERIFY SCRIPT- sed -i 's/cs_totalBytesSent/m_total_bytes_sent_mutex/g' -- $(git grep --files-with-matches 'cs_totalBytesSent') -END VERIFY SCRIPT-

w0xlt force-pushed the cs_totalBytesSent_mutex branch from eef2c7b to eff7918 Compare April 18, 2022 16:23

jonatack reviewed Apr 18, 2022

View reviewed changes

src/net.h Outdated Show resolved Hide resolved

ajtowns mentioned this pull request Apr 20, 2022

Strengthen thread safety assertions #24931

Merged

vasild approved these changes Apr 22, 2022

View reviewed changes

w0xlt and others added 2 commits April 22, 2022 05:40

p2p: add assertions and negative TS annotations for `m_total_bytes_se…

8be75fd

…nt_mutex` Co-authored-by: Hennadii Stepanov <[email protected]>

p2p: replace RecursiveMutex m_total_bytes_sent_mutex with Mutex

709af67

w0xlt force-pushed the cs_totalBytesSent_mutex branch from eff7918 to 709af67 Compare April 22, 2022 08:41

vasild approved these changes Apr 22, 2022

View reviewed changes

hebasto approved these changes Apr 22, 2022

View reviewed changes

fanquake requested a review from ajtowns April 22, 2022 09:47

hebasto mentioned this pull request Apr 25, 2022

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

Open

35 tasks

laanwj merged commit a19f641 into bitcoin:master Apr 26, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 26, 2022

Merge bitcoin#24157: p2p: Replace RecursiveMutex cs_totalBytesSent wi…

751ffb7

…th Mu…

w0xlt deleted the cs_totalBytesSent_mutex branch April 27, 2022 21:24

bitcoin locked and limited conversation to collaborators Apr 27, 2023

p2p: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it #24157

p2p: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it #24157

Uh oh!

Conversation

w0xlt commented Jan 25, 2022

Uh oh!

Uh oh!

w0xlt commented Feb 10, 2022

Uh oh!

hebasto commented Feb 10, 2022

Uh oh!

w0xlt commented Feb 10, 2022

Uh oh!

hebasto commented Feb 11, 2022

Uh oh!

w0xlt commented Feb 11, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Feb 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

w0xlt commented Feb 14, 2022

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Apr 12, 2022

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack commented Apr 15, 2022

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt commented Apr 22, 2022

Uh oh!

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

jonatack commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

p2p: Replace RecursiveMutex `cs_totalBytesSent` with Mutex and rename it #24157

p2p: Replace RecursiveMutex `cs_totalBytesSent` with Mutex and rename it #24157

DrahtBot commented Feb 13, 2022 •

edited

Loading

hebasto left a comment •

edited

Loading