refactor: replace RecursiveMutex `cs_vProcessMsg` with Mutex (and rename) #24122

pull theStack wants to merge 3 commits into bitcoin:master from theStack:202201-refactor_replace_RecursiveMutex_cs_vProcess changing 5 files +15 −12

theStack commented at 12:38 PM on January 21, 2022: contributor

This PR is related to #19303 and gets rid of the RecursiveMutex cs_vProcessMsg. Both of the critical sections only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex: https://github.com/bitcoin/bitcoin/blob/e3ce019667fba2ec50a59814a26566fb67fa9125/src/net.cpp#L1597-L1602 https://github.com/bitcoin/bitcoin/blob/e3ce019667fba2ec50a59814a26566fb67fa9125/src/net_processing.cpp#L4142-L4150 Also, it is renamed to adapt to the (unwritten) naming convention to use the _mutex suffix rather than the cs_ prefix.

scripted-diff: rename vProcessMsg list/mutex members in CNode

-BEGIN VERIFY SCRIPT-
sed -i 's/cs_vProcessMsg/m_process_msgs_mutex/g' $(git grep -l cs_vProcessMsg)
sed -i 's/vProcessMsg/m_process_msgs/g' $(git grep -l vProcessMsg)
-END VERIFY SCRIPT-

d55f8a5972

refactor: replace RecursiveMutex `m_process_msgs_mutex` with Mutex

In each of the critical sections, only the the m_process_msgs member is
accessed, without any chance that within one section another one is
called.  Hence, we can use an ordinary Mutex instead of RecursiveMutex.

9a355e1d20

DrahtBot added the label P2P on Jan 21, 2022
DrahtBot added the label Refactoring on Jan 21, 2022
shaavan commented at 6:09 PM on January 21, 2022: contributor

Concept ACK

I was thinking, should we use AssertLockNotHeld(m_process_msgs_mutex) before locking the mutex and add thread safety annotation LOCKS_EXCLUDED(m_process_msgs_mutex), with the function declaration of where this mutex is used?

Doing so will prevent any risk of error due to simultaneous locking of this mutex.
w0xlt approved
w0xlt commented at 12:39 AM on January 22, 2022: contributor

crACK 9a355e1

I agree with @shaavan .

It might be nice to add AssertLockNotHeld(mutex) macros combined with LOCKS_EXCLUDED(mutex) thread safety annotations. This avoids any risk of recursive locking.
theStack commented at 8:54 PM on January 22, 2022: contributor

@shaavan @w0xlt: Thanks for your feedback, I agree that adding thread safety assertions and annotations is a good idea. Added a commit doing that -- note though that the LOCKS_EXCLUDED annotation is only possible for one method (PeerManagerImpl::ProcessMessages), as for the other (CConnman::SocketHandlerConnected) the elements that cointain the loop are in a list rather than a direct parameter that can be accessed.
DrahtBot commented at 9:11 AM on January 23, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #25109 (Strengthen AssertLockNotHeld assertions by ajtowns)
- #24931 (Strengthen thread safety assertions by ajtowns)
- #24474 (Additional thread safety annotations for CNode/Peer by ajtowns)
- #21527 (net_processing: lock clean up by ajtowns)
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.

in src/test/util/net.cpp:26 in 9add3f9c34 outdated

  21 | @@ -22,8 +22,8 @@ void ConnmanTestMsg::NodeReceiveMsgBytes(CNode& node, Span<const uint8_t> msg_by
  22 |              nSizeAdded += it->m_raw_message_size;
  23 |          }
  24 |          {
  25 | -            LOCK(node.cs_vProcessMsg);
  26 | -            node.vProcessMsg.splice(node.vProcessMsg.end(), node.vRecvMsg, node.vRecvMsg.begin(), it);
  27 | +            LOCK(node.m_process_msgs_mutex);

shaavan commented at 2:21 PM on January 23, 2022:

Can we add AssertLockNotHeld above this line?

            AssertLockNotHeld(node.m_process_msgs_mutex);
            LOCK(node.m_process_msgs_mutex);

theStack commented at 1:46 PM on January 24, 2022:

Good catch, done! Also added the thread safety annotation to the corresponding method ConnmanTestMsg::NodeReceiveMsgBytes.

shaavan commented at 2:22 PM on January 23, 2022: contributor

as for the other (CConnman::SocketHandlerConnected) the elements that cointain the loop are in a list rather than a direct parameter that can be accessed.

Sounds interesting. Let me take another look at this before I can be 100% sure. In the meantime, I had one observation that I would like to discuss.
theStack force-pushed on Jan 24, 2022
refactor: add thread safety lock assertions for `m_process_msgs_mutex`
Co-authored-by: Shashwat <svangani239@gmail.com>
9c0b2687ca
theStack force-pushed on Jan 24, 2022

in src/net.h:418 in 9c0b2687ca

 412 | @@ -413,8 +413,8 @@ class CNode
 413 |      Mutex cs_hSocket;
 414 |      Mutex cs_vRecv;
 415 |  
 416 | -    RecursiveMutex cs_vProcessMsg;
 417 | -    std::list<CNetMessage> vProcessMsg GUARDED_BY(cs_vProcessMsg);
 418 | +    Mutex m_process_msgs_mutex;
 419 | +    std::list<CNetMessage> m_process_msgs GUARDED_BY(m_process_msgs_mutex);
 420 |      size_t nProcessQueueSize{0};

hebasto commented at 2:57 PM on January 24, 2022:

Reading the code makes me strongly believe that the value of nProcessQueueSize must correspond to the m_process_msgs content. To preserve this invariant of the CNode class I'm suggesting:

    size_t nProcessQueueSize GUARDED_BY(m_process_msgs_mutex){0};

hebasto commented at 3:15 PM on January 24, 2022: member

Concept ACK.

Following best practices, mutexes should synchronize access to internal class data to prevent violation of class invariants.

The current implementation has a significant design drawback -- external synchronizing, i.e., acquiring CNode::m_process_msgs_mutex from outside of the CNode instance.

Therefore, I'm suggesting to consider to encapsulate access to the private members m_process_msgs and nProcessQueueSize with internal synchronizing using m_process_msgs_mutex.

One new CNode member function could implement https://github.com/bitcoin/bitcoin/blob/e3de7cb9039770e0fd5b8bb8a5cba35c87ae8f00/src/net.cpp#L1592-L1604

Another one https://github.com/bitcoin/bitcoin/blob/e3de7cb9039770e0fd5b8bb8a5cba35c87ae8f00/src/net_processing.cpp#L4165-L4173
DrahtBot added the label Needs rebase on May 16, 2022
DrahtBot commented at 1:05 PM on May 16, 2022: contributor



🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>
DrahtBot commented at 10:51 AM on August 1, 2022: contributor
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
- Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
- Is it no longer relevant? ➡️ Please close.
- Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.
achow101 commented at 6:57 PM on October 12, 2022: member

Are you still working on this?
achow101 commented at 5:12 PM on November 10, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.
achow101 closed this on Nov 10, 2022
bitcoin locked this on Nov 10, 2023

Contributors

Labels

Refactoring P2P Needs rebase

Linked (view graph)

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

refactor: replace RecursiveMutex `cs_vProcessMsg` with Mutex (and rename) #24122

Conflicts