-
Notifications
You must be signed in to change notification settings - Fork 6.1k
8360048: NMT crash in gtest/NMTGtests.java: fatal error: NMT corruption: Block at 0x0000017748307120: header canary broken #26284
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
Conversation
…on: Block at 0x0000017748307120: header canary broken
/contributor add afshin-zafari |
👋 Welcome back dholmes! A progress list of the required criteria for merging this PR into |
@dholmes-ora 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 12 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@dholmes-ora Syntax:
User names can only be used for users in the census associated with this repository. For other contributors you need to supply the full name and email address. |
@dholmes-ora The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
/contributor add azafari |
@dholmes-ora |
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.
Are you accepting nits here? I see other PR was already reviewed.
Really hard to understand where the fix is. Bug synopsis also does not help :) I assume the problem is really in the test?
Co-authored-by: Aleksey Shipilëv <[email protected]>
Co-authored-by: Aleksey Shipilëv <[email protected]>
The intent was to just create a proxy for the other PR so we could hit the integrate button. But that hasn't happened yet so nits accepted.
I have similar thoughts and am asking for some clarification from Gerard (as original reviewer). |
Agree, it's confusing, Afshin said:
but frankly I don't see any locks involved in this code path: This where we detect the issue:
called by:
called by:
called by:
called by:
called by:
called by:
As you can see in the old code, we call I think the simplest temp fix here would be to do:
In the new code we don't call Afshin original fix incorporated feedback, not directly applicable to this fix, and now I wish we went with a simple fix and left other enhancements for later. Live and learn... |
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.
Wishing that we did not include all the tangential work in the original fix...
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.
So replacing the uses of (sigleton, shared) VirtualMemoryTracker::Instance
with local VirtualMemoryTracker vmt(true);
dodges the locking issue, right?
What confuses me in this patch is why are we doing replacements like:
- site->commit_memory(rgn->committed_size());
+ site->commit_memory(VirtualMemoryTracker::Instance::committed_size(rgn));
Doesn't that introduce dependencies on that singleton instance? Can you confirm that is sane?
@gerard-ziemski Can you prepare just the test fix for us to review? This should be a further RFE. |
Exactly.
I will take a look, but I think the plan now is to come up with a simple fix (since this needs to be backported to jdk25), so it looks like we are not going to push this one (Coleen said you fixed your issue already, so there is urgency anymore? Please let me know if that is not the case) |
Happily, on it... |
Yeah, mine is JDK-8361752, it is orthogonal to this. I am still interested not having gtest failures in our testing :) |
Closing this PR and returning control to Afshin and Gerard. The main issue we thought this would address was JDK-8361752 but that is a separate issue. |
This is a clone of #25950 that we need to get integrated ASAP.
The canary header test failed since there were concurrent remove and free() from the tree. The remove operations are synch'ed with corresponding NMT lock. The ReserveMemory::reserve() uses the same lock internally and is not included in the locked code block.
I'm re-testing with tiers 1-4
Progress
Issue
Reviewers
Contributors
<[email protected]>
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26284/head:pull/26284
$ git checkout pull/26284
Update a local copy of the PR:
$ git checkout pull/26284
$ git pull https://git.openjdk.org/jdk.git pull/26284/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26284
View PR using the GUI difftool:
$ git pr show -t 26284
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26284.diff
Using Webrev
Link to Webrev Comment