New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up lockorder data of destroyed mutexes #7846

Merged
merged 1 commit into from Apr 14, 2016

Conversation

Projects
None yet
3 participants
@sipa
Member

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 8, 2016

Member

Hopefully this fixes #7470.

Member

sipa commented Apr 8, 2016

Hopefully this fixes #7470.

@laanwj laanwj added the Tests label Apr 9, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 9, 2016

Member

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.

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.

Clean up lockorder data of destroyed mutexes
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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 10, 2016

Member

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

Member

sipa commented Apr 10, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 11, 2016

Member

Nice! LGTM.
utACK 5eeb913.

Member

jonasschnelli commented Apr 11, 2016

Nice! LGTM.
utACK 5eeb913.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Apr 14, 2016

Member

@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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 14, 2016

Merge #7846: Clean up lockorder data of destroyed mutexes
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 added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge bitcoin#7846: Clean up lockorder data of destroyed mutexes
5eeb913 Clean up lockorder data of destroyed mutexes (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge bitcoin#7846: Clean up lockorder data of destroyed mutexes
5eeb913 Clean up lockorder data of destroyed mutexes (Pieter Wuille)

codablock added a commit to codablock/dash that referenced this pull request Dec 20, 2017

Merge bitcoin#7846: Clean up lockorder data of destroyed mutexes
5eeb913 Clean up lockorder data of destroyed mutexes (Pieter Wuille)

@dustinengle dustinengle referenced this pull request Jun 21, 2018

Merged

Dev update and fixes #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment