refactor: replace RecursiveMutex `m_cs_callbacks_pending` with Mutex (and rename) #24069

theStack · 2022-01-14T12:35:02Z

This PR is related to #19303 and gets rid of the RecursiveMutex m_cs_callbacks_pending. All of the critical sections (6 in total) 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:

bitcoin/src/scheduler.cpp

Lines 138 to 145 in 807169e

    
           { 
        
               LOCK(m_cs_callbacks_pending); 
        
               // Try to avoid scheduling too many copies here, but if we 
        
               // accidentally have two ProcessQueue's scheduled at once its 
        
               // not a big deal. 
        
               if (m_are_callbacks_running) return; 
        
               if (m_callbacks_pending.empty()) return; 
        
           }

bitcoin/src/scheduler.cpp

Lines 152 to 160 in 807169e

    
           { 
        
               LOCK(m_cs_callbacks_pending); 
        
               if (m_are_callbacks_running) return; 
        
               if (m_callbacks_pending.empty()) return; 
        
               m_are_callbacks_running = true; 
        
               callback = std::move(m_callbacks_pending.front()); 
        
               m_callbacks_pending.pop_front(); 
        
           }

bitcoin/src/scheduler.cpp

Lines 169 to 172 in 807169e

    
           { 
        
               LOCK(instance->m_cs_callbacks_pending); 
        
               instance->m_are_callbacks_running = false; 
        
           }

bitcoin/src/scheduler.cpp

Lines 184 to 187 in 807169e

    
           { 
        
               LOCK(m_cs_callbacks_pending); 
        
               m_callbacks_pending.emplace_back(std::move(func)); 
        
           }

bitcoin/src/scheduler.cpp

Lines 197 to 199 in 807169e

    
               LOCK(m_cs_callbacks_pending); 
        
               should_continue = !m_callbacks_pending.empty(); 
        
           }

bitcoin/src/scheduler.cpp

Lines 203 to 206 in 807169e

    
           { 
        
               LOCK(m_cs_callbacks_pending); 
        
               return m_callbacks_pending.size(); 
        
           }

Also, it is renamed to adapt to the (unwritten) naming convention to use the _mutex suffix rather than the cs_ prefix.

-BEGIN VERIFY SCRIPT- sed -i 's/m_cs_callbacks_pending/m_callbacks_mutex/g' ./src/scheduler.h ./src/scheduler.cpp -END VERIFY SCRIPT

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

hebasto

ACK 5574e6e, I have reviewed the code and it looks OK, I agree it can be merged.

w0xlt

crACK 5574e6e

…_pending` with Mutex (and rename) 5574e6e refactor: replace RecursiveMutex `m_callbacks_mutex` with Mutex (Sebastian Falbesoner) 3aa2581 scripted-diff: rename `m_cs_callbacks_pending` -> `m_callbacks_mutex` (Sebastian Falbesoner) Pull request description: This PR is related to bitcoin#19303 and gets rid of the RecursiveMutex `m_cs_callbacks_pending`. All of the critical sections (6 in total) 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/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L138-L145 https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L152-L160 https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L169-L172 https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L184-L187 https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L197-L199 https://github.com/bitcoin/bitcoin/blob/807169e10b4a18324356ed6ee4d69587b96a7c70/src/scheduler.cpp#L203-L206 Also, it is renamed to adapt to the (unwritten) naming convention to use the `_mutex` suffix rather than the `cs_` prefix. ACKs for top commit: hebasto: ACK 5574e6e, I have reviewed the code and it looks OK, I agree it can be merged. w0xlt: crACK 5574e6e Tree-SHA512: ba4b151d956582f4c7183a1d51702b269158fc5e2902c51e6a242aaeb1c72cfcdf398f9ffa42e3072f5aba21a8c493086a5fe7c450c271322da69bd54c37ed1f

theStack added 2 commits January 14, 2022 13:25

scripted-diff: rename m_cs_callbacks_pending -> m_callbacks_mutex

3aa2581

-BEGIN VERIFY SCRIPT- sed -i 's/m_cs_callbacks_pending/m_callbacks_mutex/g' ./src/scheduler.h ./src/scheduler.cpp -END VERIFY SCRIPT

refactor: replace RecursiveMutex m_callbacks_mutex with Mutex

5574e6e

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

hebasto approved these changes Jan 14, 2022

View reviewed changes

hebasto mentioned this pull request Jan 14, 2022

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

Open

36 tasks

DrahtBot added the Refactoring label Jan 14, 2022

w0xlt approved these changes Jan 16, 2022

View reviewed changes

maflcko merged commit 427e9c9 into bitcoin:master Jan 17, 2022

theStack deleted the 202201-refactor_replace_recursive_mutex_callbacks branch January 17, 2022 11:34

bitcoin locked and limited conversation to collaborators Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: replace RecursiveMutex `m_cs_callbacks_pending` with Mutex (and rename) #24069

refactor: replace RecursiveMutex `m_cs_callbacks_pending` with Mutex (and rename) #24069

Uh oh!

theStack commented Jan 14, 2022

Uh oh!

hebasto left a comment

Uh oh!

w0xlt left a comment

Uh oh!

Uh oh!

	{
	LOCK(m_cs_callbacks_pending);
	// Try to avoid scheduling too many copies here, but if we
	// accidentally have two ProcessQueue's scheduled at once its
	// not a big deal.
	if (m_are_callbacks_running) return;
	if (m_callbacks_pending.empty()) return;
	}

	{
	LOCK(instance->m_cs_callbacks_pending);
	instance->m_are_callbacks_running = false;
	}

	{
	LOCK(m_cs_callbacks_pending);
	m_callbacks_pending.emplace_back(std::move(func));
	}

	LOCK(m_cs_callbacks_pending);
	should_continue = !m_callbacks_pending.empty();
	}

	{
	LOCK(m_cs_callbacks_pending);
	return m_callbacks_pending.size();
	}

refactor: replace RecursiveMutex m_cs_callbacks_pending with Mutex (and rename) #24069

refactor: replace RecursiveMutex m_cs_callbacks_pending with Mutex (and rename) #24069

Uh oh!

Conversation

theStack commented Jan 14, 2022

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

refactor: replace RecursiveMutex `m_cs_callbacks_pending` with Mutex (and rename) #24069

refactor: replace RecursiveMutex `m_cs_callbacks_pending` with Mutex (and rename) #24069