-
Notifications
You must be signed in to change notification settings - Fork 58
8290867: Race freeing remembered set segments #152
8290867: Race freeing remembered set segments #152
Conversation
👋 Welcome back tschatzl! A progress list of the required criteria for merging this PR into |
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@tschatzl This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 5 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
Testing showed an issue with VM shutdown: VM shutdown calls the the destructor for the (global) free list pool, which in turn frees all segments; however at that time the VM is not in a state where Passes tier1-5 with no crashes. |
Thanks @kimbarrett @sangheon for your reviews |
Going to push as commit e265b2a.
Your commit was automatically rebased without conflicts. |
For the record, we discussed this before Thomas pushed, and I agree with his assessment. The workaround is simple and safe, and doing something more principled might not fit in the current release schedule. |
Hi all,
please review this fix for a crash due to a race in remembered set segment deallocation. Here is the description (provided by chaeubl as reported):
G1SegmentedArray::create_new_segment
and tries to pop an element from the_free_segment_list
. For that, thread A executesLockFreeStack::pop()
LockFreeStack::top()
LockFreeStack::pop()
, also readsLockFreeStack::top()
and pops that element from the stackAtomic::cmpxchg(&_first, prev, next);
inG1SegmentedArray::create_new_segment
but it fails because another thread already registered a different segmentG1SegmentedArraySegment::delete_segment
and frees the valuetop()->next
inLockFreeStack::pop()
, which causes a segfault becausetop()
was freed by thread BThe fix is to delay the deletion of that memory segment until all readers (i.e. in
G1SegmentedArrayFreeList::get
calling_list.pop()
) drop the references to that memory segment. The readers are already guarded by aCriticalSection
.Testing: tier1-5 running, reproducer that adds extra delays that significantly delays to widen the opportunity this race can occur passes on BigRAMTester (otherwise crashes in a few seconds)
Thanks,
Thomas
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk19 pull/152/head:pull/152
$ git checkout pull/152
Update a local copy of the PR:
$ git checkout pull/152
$ git pull https://git.openjdk.org/jdk19 pull/152/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 152
View PR using the GUI difftool:
$ git pr show -t 152
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk19/pull/152.diff