diff options
author | laanwj <126646+laanwj@users.noreply.github.com> | 2022-04-26 10:21:27 +0200 |
---|---|---|
committer | laanwj <126646+laanwj@users.noreply.github.com> | 2022-04-26 10:21:52 +0200 |
commit | a19f641a80cb2841ca9f593e9be589dbc4b4ae7c (patch) | |
tree | 8d623772aefc204cbbe20a6a5da17967c19f19a9 | |
parent | 1ae65b4c5fbffb691e755dc0dacf1c63b55ad81b (diff) | |
parent | 709af67add93f6674fb80e3ae8e3f175653a62f0 (diff) | |
download | bitcoin-a19f641a80cb2841ca9f593e9be589dbc4b4ae7c.tar.xz |
Merge bitcoin/bitcoin#24157: p2p: Replace RecursiveMutex `cs_totalBytesSent` with Mutex and rename it
709af67add93f6674fb80e3ae8e3f175653a62f0 p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex (w0xlt)
8be75fd0f0039eeea5f9af7c1eb17c584ed9f507 p2p: add assertions and negative TS annotations for `m_total_bytes_sent_mutex` (w0xlt)
a237a065cc2c6337e3797cc30a0f84c56c6d2f3b scripted-diff: rename cs_totalBytesSent -> m_total_bytes_sent_mutex (w0xlt)
Pull request description:
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.
ACKs for top commit:
jonatack:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0 per `git range-diff 7a4ac71 eff7918 709af67`, rebase to master, clang 15 debug build, and build with -Wthread-safety-negative
vasild:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0
hebasto:
ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.
Tree-SHA512: 560b4e6c92b1511911d69185207df6ee809db09b96d97f96430d8d2595dc05c98cc691aaec8a58ef87cf2ab0a98675c210b8ce0be3dedb81e31114bbbfdfd8be
-rw-r--r-- | src/net.cpp | 36 | ||||
-rw-r--r-- | src/net.h | 46 |
2 files changed, 55 insertions, 27 deletions
diff --git a/src/net.cpp b/src/net.cpp index 77fa06ce26..aa70bed226 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1567,6 +1567,8 @@ void CConnman::SocketEvents(const std::vector<CNode*>& nodes, void CConnman::SocketHandler() { + AssertLockNotHeld(m_total_bytes_sent_mutex); + std::set<SOCKET> recv_set; std::set<SOCKET> send_set; std::set<SOCKET> error_set; @@ -1593,6 +1595,8 @@ void CConnman::SocketHandlerConnected(const std::vector<CNode*>& nodes, const std::set<SOCKET>& send_set, const std::set<SOCKET>& error_set) { + AssertLockNotHeld(m_total_bytes_sent_mutex); + for (CNode* pnode : nodes) { if (interruptNet) return; @@ -1694,6 +1698,8 @@ void CConnman::SocketHandlerListening(const std::set<SOCKET>& recv_set) void CConnman::ThreadSocketHandler() { + AssertLockNotHeld(m_total_bytes_sent_mutex); + SetSyscallSandboxPolicy(SyscallSandboxPolicy::NET); while (!interruptNet) { @@ -2578,6 +2584,7 @@ bool CConnman::InitBinds(const Options& options) bool CConnman::Start(CScheduler& scheduler, const Options& connOptions) { + AssertLockNotHeld(m_total_bytes_sent_mutex); Init(connOptions); if (fListen && !InitBinds(connOptions)) { @@ -2930,7 +2937,9 @@ void CConnman::RecordBytesRecv(uint64_t bytes) void CConnman::RecordBytesSent(uint64_t bytes) { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); + nTotalBytesSent += bytes; const auto now = GetTime<std::chrono::seconds>(); @@ -2946,7 +2955,8 @@ void CConnman::RecordBytesSent(uint64_t bytes) uint64_t CConnman::GetMaxOutboundTarget() const { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); return nMaxOutboundLimit; } @@ -2957,7 +2967,15 @@ std::chrono::seconds CConnman::GetMaxOutboundTimeframe() const std::chrono::seconds CConnman::GetMaxOutboundTimeLeftInCycle() const { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); + return GetMaxOutboundTimeLeftInCycle_(); +} + +std::chrono::seconds CConnman::GetMaxOutboundTimeLeftInCycle_() const +{ + AssertLockHeld(m_total_bytes_sent_mutex); + if (nMaxOutboundLimit == 0) return 0s; @@ -2971,14 +2989,15 @@ std::chrono::seconds CConnman::GetMaxOutboundTimeLeftInCycle() const bool CConnman::OutboundTargetReached(bool historicalBlockServingLimit) const { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); if (nMaxOutboundLimit == 0) return false; if (historicalBlockServingLimit) { // keep a large enough buffer to at least relay each block once - const std::chrono::seconds timeLeftInCycle = GetMaxOutboundTimeLeftInCycle(); + const std::chrono::seconds timeLeftInCycle = GetMaxOutboundTimeLeftInCycle_(); const uint64_t buffer = timeLeftInCycle / std::chrono::minutes{10} * MAX_BLOCK_SERIALIZED_SIZE; if (buffer >= nMaxOutboundLimit || nMaxOutboundTotalBytesSentInCycle >= nMaxOutboundLimit - buffer) return true; @@ -2991,7 +3010,8 @@ bool CConnman::OutboundTargetReached(bool historicalBlockServingLimit) const uint64_t CConnman::GetOutboundTargetBytesLeft() const { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); if (nMaxOutboundLimit == 0) return 0; @@ -3005,7 +3025,8 @@ uint64_t CConnman::GetTotalBytesRecv() const uint64_t CConnman::GetTotalBytesSent() const { - LOCK(cs_totalBytesSent); + AssertLockNotHeld(m_total_bytes_sent_mutex); + LOCK(m_total_bytes_sent_mutex); return nTotalBytesSent; } @@ -3052,6 +3073,7 @@ bool CConnman::NodeFullyConnected(const CNode* pnode) void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg) { + AssertLockNotHeld(m_total_bytes_sent_mutex); size_t nMessageSize = msg.data.size(); LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n", msg.m_type, nMessageSize, pnode->GetId()); if (gArgs.GetBoolArg("-capturemessages", false)) { @@ -761,7 +761,10 @@ public: bool m_i2p_accept_incoming; }; - void Init(const Options& connOptions) { + void Init(const Options& connOptions) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex) + { + AssertLockNotHeld(m_total_bytes_sent_mutex); + nLocalServices = connOptions.nLocalServices; nMaxConnections = connOptions.nMaxConnections; m_max_outbound_full_relay = std::min(connOptions.m_max_outbound_full_relay, connOptions.nMaxConnections); @@ -777,7 +780,7 @@ public: nReceiveFloodSize = connOptions.nReceiveFloodSize; m_peer_connect_timeout = std::chrono::seconds{connOptions.m_peer_connect_timeout}; { - LOCK(cs_totalBytesSent); + LOCK(m_total_bytes_sent_mutex); nMaxOutboundLimit = connOptions.nMaxOutboundLimit; } vWhitelistedRange = connOptions.vWhitelistedRange; @@ -792,7 +795,7 @@ public: bool network_active = true); ~CConnman(); - bool Start(CScheduler& scheduler, const Options& options); + bool Start(CScheduler& scheduler, const Options& options) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); void StopThreads(); void StopNodes(); @@ -811,7 +814,7 @@ 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) @@ -902,24 +905,22 @@ public: //! that peer during `net_processing.cpp:PushNodeVersion()`. ServiceFlags GetLocalServices() const; - uint64_t GetMaxOutboundTarget() const; + uint64_t GetMaxOutboundTarget() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); std::chrono::seconds GetMaxOutboundTimeframe() const; //! check if the outbound target is reached //! if param historicalBlockServingLimit is set true, the function will //! response true if the limit for serving historical blocks has been reached - bool OutboundTargetReached(bool historicalBlockServingLimit) const; + bool OutboundTargetReached(bool historicalBlockServingLimit) const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); //! response the bytes left in the current max outbound cycle //! in case of no limit, it will always response 0 - uint64_t GetOutboundTargetBytesLeft() const; + uint64_t GetOutboundTargetBytesLeft() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); - //! returns the time left in the current max outbound cycle - //! in case of no limit, it will always return 0 - std::chrono::seconds GetMaxOutboundTimeLeftInCycle() const; + std::chrono::seconds GetMaxOutboundTimeLeftInCycle() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); uint64_t GetTotalBytesRecv() const; - uint64_t GetTotalBytesSent() const; + uint64_t GetTotalBytesSent() const EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); /** Get a unique deterministic randomizer. */ CSipHasher GetDeterministicRandomizer(uint64_t id) const; @@ -945,6 +946,10 @@ private: NetPermissionFlags m_permissions; }; + //! returns the time left in the current max outbound cycle + //! in case of no limit, it will always return 0 + std::chrono::seconds GetMaxOutboundTimeLeftInCycle_() const EXCLUSIVE_LOCKS_REQUIRED(m_total_bytes_sent_mutex); + bool BindListenPort(const CService& bindAddr, bilingual_str& strError, NetPermissionFlags permissions); bool Bind(const CService& addr, unsigned int flags, NetPermissionFlags permissions); bool InitBinds(const Options& options); @@ -1004,7 +1009,7 @@ 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. @@ -1017,7 +1022,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. @@ -1025,7 +1031,7 @@ 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; @@ -1054,7 +1060,7 @@ private: // Network stats void RecordBytesRecv(uint64_t bytes); - void RecordBytesSent(uint64_t bytes); + void RecordBytesSent(uint64_t bytes) EXCLUSIVE_LOCKS_REQUIRED(!m_total_bytes_sent_mutex); /** * Return vector of current BLOCK_RELAY peers. @@ -1065,14 +1071,14 @@ private: static bool NodeFullyConnected(const CNode* pnode); // Network usage totals - mutable RecursiveMutex cs_totalBytesSent; + mutable Mutex m_total_bytes_sent_mutex; std::atomic<uint64_t> nTotalBytesRecv{0}; - uint64_t nTotalBytesSent GUARDED_BY(cs_totalBytesSent) {0}; + uint64_t nTotalBytesSent GUARDED_BY(m_total_bytes_sent_mutex) {0}; // outbound limit & stats - uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(cs_totalBytesSent) {0}; - std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(cs_totalBytesSent) {0}; - uint64_t nMaxOutboundLimit GUARDED_BY(cs_totalBytesSent); + uint64_t nMaxOutboundTotalBytesSentInCycle GUARDED_BY(m_total_bytes_sent_mutex) {0}; + std::chrono::seconds nMaxOutboundCycleStartTime GUARDED_BY(m_total_bytes_sent_mutex) {0}; + uint64_t nMaxOutboundLimit GUARDED_BY(m_total_bytes_sent_mutex); // P2P timeout in seconds std::chrono::seconds m_peer_connect_timeout; |