Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Apr 8, 2016

The lockorder potential deadlock detection works by remembering for each lock A that is acquired while holding another B the pair (A,B), and triggering a warning when (B,A) already exists in the table.

A and B in the above text are represented by pointers to the CCriticalSection object that is acquired. This does mean however that we need to clean up the table entries that refer to any critical section which is destroyed, as its memory address can potentially be used for another unrelated lock in the future.

Implement this clean up by remembering not only the pairs in forward direction, but also backward direction. This allows for fast iteration over all pairs that use a deleted CCriticalSection in either the first or the second position.

@sipa
Copy link
Member Author

sipa commented Apr 8, 2016

Hopefully this fixes #7470.

@laanwj laanwj added the Tests label Apr 9, 2016
@laanwj
Copy link
Member

laanwj commented Apr 9, 2016

Thanks!
I lack the understanding of this code to be able to review it, unfortunately this makes the code even more complicated. Luckily it's only active in debug mode (--enable-debug) anyhow.

utACK if it fixes #7470.

The lockorder potential deadlock detection works by remembering for each
lock A that is acquired while holding another B the pair (A,B), and
triggering a warning when (B,A) already exists in the table.

A and B in the above text are represented by pointers to the CCriticalSection
object that is acquired. This does mean however that we need to clean up the
table entries that refer to any critical section which is destroyed, as it
memory address can potentially be used for another unrelated lock in the future.

Implement this clean up by remembering not only the pairs in forward direction,
but also backward direction. This allows for fast iteration over all pairs that
use a deleted CCriticalSection in either the first or the second position.
@sipa
Copy link
Member Author

sipa commented Apr 10, 2016

@laanwj Added a PR and commit description that explains the change and rationale.

@jonasschnelli
Copy link
Contributor

Nice! LGTM.
utACK 5eeb913.

@laanwj
Copy link
Member

laanwj commented Apr 14, 2016

@sipa OHH I get it now.
So sync.cpp keeps track of locks after they are unlocked. They are simply never cleaned up. I had assumed this was only keeping track of active locks, so I didn't understand why locks would be deleted that were still held somewhere.
But well in that case this fixes a memory leak too!
tACK 5eeb913

@laanwj laanwj merged commit 5eeb913 into bitcoin:master Apr 14, 2016
laanwj added a commit that referenced this pull request Apr 14, 2016
5eeb913 Clean up lockorder data of destroyed mutexes (Pieter Wuille)
thokon00 added a commit to faircoin/faircoin that referenced this pull request Mar 20, 2017
codablock pushed a commit to codablock/dash that referenced this pull request Dec 20, 2017
5eeb913 Clean up lockorder data of destroyed mutexes (Pieter Wuille)
LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Feb 20, 2021
The lockorder potential deadlock detection works by remembering for each
lock A that is acquired while holding another B the pair (A,B), and
triggering a warning when (B,A) already exists in the table.

A and B in the above text are represented by pointers to the CCriticalSection
object that is acquired. This does mean however that we need to clean up the
table entries that refer to any critical section which is destroyed, as it
memory address can potentially be used for another unrelated lock in the future.

Implement this clean up by remembering not only the pairs in forward direction,
but also backward direction. This allows for fast iteration over all pairs that
use a deleted CCriticalSection in either the first or the second position.

zcash: cherry picked from commit 5eeb913
zcash: bitcoin/bitcoin#7846
zkbot added a commit to zcash/zcash that referenced this pull request Apr 1, 2021
Bitcoin 0.13 locking PRs

These are locking changes from upstream (bitcoin core) release 0.13, oldest to newest (when they were merged to the master branch).
- bitcoin/bitcoin#7846
- bitcoin/bitcoin#7913
- bitcoin/bitcoin#8016
  - second commit only; first commit, test changes, are already done
- bitcoin/bitcoin#7942

This PR does not include:
 - bitcoin/bitcoin#8244 bitcoin/bitcoin@27f8126
   -  zcash requires locking `cs_main` in this instance (`getrawmempool()` calls `mempoolToJSON()`, which calls `chainActive.Height()`).
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants