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

pull w0xlt wants to merge 3 commits into bitcoin:master from w0xlt:cs_totalBytesSent_mutex changing 2 files +55 −27
  1. 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.

  2. w0xlt marked this as a draft on Jan 25, 2022
  3. DrahtBot added the label P2P on Jan 25, 2022
  4. w0xlt force-pushed on Jan 26, 2022
  5. w0xlt marked this as ready for review on Jan 26, 2022
  6. 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

        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?

  7. w0xlt force-pushed on Feb 10, 2022
  8. w0xlt marked this as a draft on Feb 10, 2022
  9. w0xlt force-pushed on Feb 10, 2022
  10. w0xlt marked this as ready for review on Feb 10, 2022
  11. 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.

    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);
  12. hebasto commented at 7:51 PM on February 10, 2022: member

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

    ?

  13. w0xlt force-pushed on Feb 10, 2022
  14. 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.

  15. 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?

  16. w0xlt force-pushed on Feb 11, 2022
  17. w0xlt commented at 7:37 PM on February 11, 2022: contributor

    Maybe squash them?

    Sure. Done.

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

  19. hebasto approved
  20. hebasto commented at 8:05 PM on February 12, 2022: member

    ACK 0b6c5da9304af71dedb262bca9a0493c8616985b

  21. DrahtBot commented at 2:19 PM on February 13, 2022: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  22. w0xlt force-pushed on Feb 13, 2022
  23. hebasto approved
  24. hebasto commented at 9:51 PM on February 13, 2022: member

    re-ACK f04ffdd2a79a77ea70d892be972de7821c1b876c

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

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

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

  28. w0xlt force-pushed on Feb 14, 2022
  29. w0xlt commented at 7:15 PM on February 14, 2022: contributor

    d370938 addresses @vasild suggestions.

  30. vasild approved
  31. vasild commented at 1:11 PM on February 18, 2022: member

    ACK eef2c7b8c2d3c35da027429bf89ebb792e55f739

  32. hebasto approved
  33. 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)

  34. w0xlt commented at 5:59 PM on April 12, 2022: contributor

    Is any further action needed in this PR?

  35. 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):

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

    ends with warnings

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

    To silent all of them, the following patch works:

    --- a/src/net.h
+++ b/src/net.h
@@ -760,7 +760,8 @@ 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;
@@ -791,7 +792,7 @@ public:
 
     CConnman(uint64_t seed0, uint64_t seed1, AddrMan& addrman, 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();
    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

  36. ajtowns commented at 4:57 PM on April 13, 2022: member

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

  37. 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:

    GetMaxOutboundTarget
GetMaxOutboundTimeLeftInCycle
GetMaxOutboundTimeLeftInCycle_
GetOutboundTargetBytesLeft
GetTotalBytesSent
OutboundTargetReached
PushMessage
RecordBytesSent
SocketHandler
SocketHandlerConnected
ThreadSocketHandler
  38. 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
  39. w0xlt force-pushed on Apr 18, 2022
  40. 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

     bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
 {
+    AssertLockNotHeld(m_total_bytes_sent_mutex);
     Init(connOptions);
    w0xlt commented at 8:44 AM on April 22, 2022:

    Done. Thanks.

  41. vasild approved
  42. vasild commented at 6:52 AM on April 22, 2022: member

    ACK eff791863da5dd1fcda7bddc7d729a63d772659d

    Compiles with clang 15.

  43. 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
  44. p2p: replace RecursiveMutex `m_total_bytes_sent_mutex` with Mutex 709af67add
  45. w0xlt force-pushed on Apr 22, 2022
  46. w0xlt commented at 8:46 AM on April 22, 2022: contributor

    Added the suggestion #24157 (review)

  47. vasild approved
  48. vasild commented at 9:24 AM on April 22, 2022: member

    ACK 709af67add93f6674fb80e3ae8e3f175653a62f0

  49. hebasto approved
  50. hebasto commented at 9:46 AM on April 22, 2022: member

    ACK 709af67add93f6674fb80e3ae8e3f175653a62f0, tested on Ubuntu 22.04.

  51. fanquake requested review from ajtowns on Apr 22, 2022
  52. 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

  53. laanwj merged this on Apr 26, 2022
  54. laanwj closed this on Apr 26, 2022
  55. sidhujag referenced this in commit 751ffb710d on Apr 26, 2022
  56. w0xlt deleted the branch on Apr 27, 2022
  57. DrahtBot locked this on Apr 27, 2023
Contributors
w0xlthebastovasildDrahtBotajtownsjonatack


ajtowns

Labels
P2P
Linked (view graph)
#19303 Replace all of the RecursiveMutex instances with the Mutex ones
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-02 12:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me