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

w0xlt commented at 10:21 pm on January 25, 2022: contributor

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.

w0xlt marked this as a draft on Jan 25, 2022

DrahtBot added the label P2P on Jan 25, 2022

w0xlt force-pushed on Jan 26, 2022

w0xlt marked this as ready for review on Jan 26, 2022

in src/net.h:914 in 6cb0fab330 outdated

910@@ -910,24 +911,22 @@ class CConnman
911     //! that peer during `net_processing.cpp:PushNodeVersion()`.
912     ServiceFlags GetLocalServices() const;
913 
914-    uint64_t GetMaxOutboundTarget() const;
915+    uint64_t GetMaxOutboundTarget() const LOCKS_EXCLUDED(m_total_bytes_sent_mutex);

hebasto commented at 1:48 pm on February 2, 2022:

Is it feasible

0    uint64_t GetMaxOutboundTarget() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);

here and after?

For context and example see #24235?

vasild commented at 2:12 pm on February 2, 2022:

I think yes.

hebasto commented at 3:09 pm on February 10, 2022:

And in other places?

w0xlt force-pushed on Feb 10, 2022

w0xlt marked this as a draft on Feb 10, 2022

w0xlt force-pushed on Feb 10, 2022

w0xlt marked this as ready for review on Feb 10, 2022

w0xlt commented at 6:57 pm on February 10, 2022: contributor

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.

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

hebasto commented at 7:51 pm on February 10, 2022: member

@w0xlt

Not sure about the reason for this error.

Maybe:

 0--- a/src/net.h
 1+++ b/src/net.h
 2@@ -829,7 +829,8 @@ public:
 3 
 4     bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
 5 
 6-    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
 7+    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
 8+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
 9 
10     using NodeFn = std::function<void(CNode*)>;
11     void ForEachNode(const NodeFn& func)
12@@ -1024,7 +1025,8 @@ private:
13     /**
14      * Check connected and listening sockets for IO readiness and process them accordingly.
15      */
16-    void SocketHandler();
17+    void SocketHandler()
18+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
19 
20     /**
21      * Do the read/write for connected sockets that are ready for IO.
22@@ -1037,7 +1039,8 @@ private:
23     void SocketHandlerConnected(const std::vector<CNode*>& nodes,
24                                 const std::set<SOCKET>& recv_set,
25                                 const std::set<SOCKET>& send_set,
26-                                const std::set<SOCKET>& error_set);
27+                                const std::set<SOCKET>& error_set)
28+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
29 
30     /**
31      * Accept incoming connections, one from each read-ready listening socket.
32@@ -1045,7 +1048,8 @@ private:
33      */
34     void SocketHandlerListening(const std::set<SOCKET>& recv_set);
35 
36-    void ThreadSocketHandler();
37+    void ThreadSocketHandler()
38+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
39     void ThreadDNSAddressSeed();
40 
41     uint64_t CalculateKeyedNetGroup(const CAddress& ad) const;
42@@ -1074,7 +1078,8 @@ private:
43 
44     // Network stats
45     void RecordBytesRecv(uint64_t bytes);
46-    void RecordBytesSent(uint64_t bytes) LOCKS_EXCLUDED(m_total_bytes_sent_mutex);
47+    void RecordBytesSent(uint64_t bytes)
48+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
49 
50     /**
51      * Return vector of current BLOCK_RELAY peers.

?

w0xlt force-pushed on Feb 10, 2022

w0xlt commented at 10:45 pm on February 10, 2022: contributor

@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 commented at 12:30 pm on February 11, 2022: member

@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 force-pushed on Feb 11, 2022

w0xlt commented at 7:37 pm on February 11, 2022: contributor

Maybe squash them?

Sure. Done.

in src/net.h:1042 in 0b6c5da930 outdated

1036@@ -1034,15 +1037,17 @@ class CConnman
1037     void SocketHandlerConnected(const std::vector<CNode*>& nodes,
1038                                 const std::set<SOCKET>& recv_set,
1039                                 const std::set<SOCKET>& send_set,
1040-                                const std::set<SOCKET>& error_set);
1041+                                const std::set<SOCKET>& error_set)
1042+        EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
1043+;

hebasto commented at 8:02 pm on February 12, 2022:

A spare semicolon? :smile:

w0xlt commented at 7:53 pm on February 13, 2022:

Oops. Fixed.

hebasto approved

hebasto commented at 8:05 pm on February 12, 2022: member

ACK 0b6c5da9304af71dedb262bca9a0493c8616985b

DrahtBot commented at 2:19 pm on February 13, 2022: 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:

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

w0xlt force-pushed on Feb 13, 2022

hebasto approved

hebasto commented at 9:51 pm on February 13, 2022: member

re-ACK f04ffdd2a79a77ea70d892be972de7821c1b876c

in src/net.cpp:2937 in f04ffdd2a7 outdated

2933+    AssertLockNotHeld(m_total_bytes_sent_mutex);
2934+    LOCK(m_total_bytes_sent_mutex);
2935+    return _GetMaxOutboundTimeLeftInCycle();
2936+}
2937+
2938+std::chrono::seconds CConnman::_GetMaxOutboundTimeLeftInCycle() const

vasild commented at 1:29 pm on February 14, 2022:

nit: I think that the unwritten convention is to put the underscore at the end, like e.g. AddrManImpl::Good_(). Or maybe I am mistaken, is there any other place in the code that uses prefix _?

w0xlt commented at 7:13 pm on February 14, 2022:

Changed the underscore to the end. I think only in AddrManImpl this is used.

in src/net.h:782 in f04ffdd2a7 outdated

795@@ -796,7 +796,8 @@ class CConnman
796         nReceiveFloodSize = connOptions.nReceiveFloodSize;
797         m_peer_connect_timeout = std::chrono::seconds{connOptions.m_peer_connect_timeout};
798         {
799-            LOCK(cs_totalBytesSent);
800+            AssertLockNotHeld(m_total_bytes_sent_mutex);
801+            LOCK(m_total_bytes_sent_mutex);

vasild commented at 1:33 pm on February 14, 2022:

This is not the beginning of a function. AssertLockNotHeld() does not belong here - it should be put in the beginning of functions.

w0xlt commented at 7:13 pm on February 14, 2022:

Fixed. Thanks.

in src/net.h:814 in f04ffdd2a7 outdated

828@@ -828,7 +829,7 @@ class CConnman
829 
830     bool ForNode(NodeId id, std::function<bool(CNode* pnode)> func);
831 
832-    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg);
833+    void PushMessage(CNode* pnode, CSerializedNetMsg&& msg) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);

vasild commented at 1:38 pm on February 14, 2022:

Why not add AssertLockNotHeld(m_total_bytes_sent_mutex) in the beginning of PushMessage(), SocketHandler(), SocketHandlerConnected() and ThreadSocketHandler()?

w0xlt commented at 7:13 pm on February 14, 2022:

Good catch. Done. Thanks.

w0xlt force-pushed on Feb 14, 2022

w0xlt commented at 7:15 pm on February 14, 2022: contributor

d370938 addresses @vasild suggestions.

vasild approved

vasild commented at 1:11 pm on February 18, 2022: member

ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739

hebasto approved

hebasto commented at 8:30 pm on February 20, 2022: member

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

UPDATE: ACK retracted. See #24157 (review)

w0xlt commented at 5:59 pm on April 12, 2022: contributor

Is any further action needed in this PR?

in src/net.h:783 in eef2c7b8c2 outdated

780@@ -781,6 +781,8 @@ class CConnman
781     };
782 
783     void Init(const Options& connOptions) {

ajtowns commented at 4:52 pm on April 13, 2022:

Missing EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)

hebasto commented at 6:25 pm on April 13, 2022:

Agree. I’ve verified this PR rebased on top of the current master (f60a63cc5f16b738d9d2ada3f10b27cf999df323):

0$ ./configure CC=clang CXX=clang++ CXXFLAGS='-Wthread-safety -Wthread-safety-negative'
1$ make clean
2$ make 2>&1 | grep m_total_bytes_sent_mutex

ends with warnings

0./net.h:781:13: warning: acquiring mutex 'm_total_bytes_sent_mutex' requires negative capability '!m_total_bytes_sent_mutex' [-Wthread-safety-negative]
1            LOCK(m_total_bytes_sent_mutex);

To silent all of them, the following patch works:

 0--- a/src/net.h
 1+++ b/src/net.h
 2@@ -760,7 +760,8 @@ public:
 3         bool m_i2p_accept_incoming;
 4     };
 5 
 6-    void Init(const Options& connOptions) {
 7+    void Init(const Options& connOptions) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex)
 8+    {
 9         AssertLockNotHeld(m_total_bytes_sent_mutex);
10 
11         nLocalServices = connOptions.nLocalServices;
12@@ -791,7 +792,7 @@ public:
13 
14     CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true);
15     ~CConnman();
16-    bool Start(CScheduler& scheduler, const Options& options);
17+    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);
18 
19     void StopThreads();
20     void StopNodes();

jonatack commented at 11:29 am on April 15, 2022:

Reproduced the issue and confirm that this patch works (nice catch!)

w0xlt commented at 4:25 pm on April 18, 2022:

Done. Thanks for the reviews and suggestions.

ajtowns commented at 9:31 am on April 20, 2022:

#24931 may be of interest

ajtowns commented at 4:57 pm on April 13, 2022: member

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

jonatack commented at 11:47 am on April 15, 2022: member

ACK modulo the missing annotation

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

 0GetMaxOutboundTarget
 1GetMaxOutboundTimeLeftInCycle
 2GetMaxOutboundTimeLeftInCycle_
 3GetOutboundTargetBytesLeft
 4GetTotalBytesSent
 5OutboundTargetReached
 6PushMessage
 7RecordBytesSent
 8SocketHandler
 9SocketHandlerConnected
10ThreadSocketHandler

scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex

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

a237a065cc

w0xlt force-pushed on Apr 18, 2022

in src/net.h:795 in eff791863d outdated

791@@ -789,7 +792,7 @@ class CConnman
792 
793     CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, bool network_active = true);
794     ~CConnman();
795-    bool Start(CScheduler& scheduler, const Options& options);
796+    bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex);

jonatack commented at 5:16 pm on April 18, 2022:

e59472f8af0a1962fa5c331e41fa22a4f8602db8 missing assertion in the corresponding definition in src/net.cpp

0 bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
1 {
2+    AssertLockNotHeld(m_total_bytes_sent_mutex);
3     Init(connOptions);

w0xlt commented at 8:44 am on April 22, 2022:

Done. Thanks.

vasild approved

vasild commented at 6:52 am on April 22, 2022: member

ACK eff791863da5dd1fcda7bddc7d729a63d772659d

Compiles with clang 15.

p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex`

Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>

8be75fd0f0

p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex 709af67add

w0xlt force-pushed on Apr 22, 2022

w0xlt commented at 8:46 am on April 22, 2022: contributor

Added the suggestion #24157 (review)

vasild approved

vasild commented at 9:24 am on April 22, 2022: member

ACK 709af67add93f6674fb80e3ae8e3f175653a62f0

hebasto approved

hebasto commented at 9:46 am on April 22, 2022: member

ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.

fanquake requested review from ajtowns on Apr 22, 2022

jonatack commented at 10:11 am on April 22, 2022: member

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

laanwj merged this on Apr 26, 2022

laanwj closed this on Apr 26, 2022

sidhujag referenced this in commit 751ffb710d on Apr 26, 2022

w0xlt deleted the branch on Apr 27, 2022

DrahtBot locked this on Apr 27, 2023

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

Conflicts

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