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
8271348: Add stronger sanity check of thread state when polling for safepoint/handshakes #4936
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,8 @@ void SafepointMechanism::process(JavaThread *thread, bool allow_suspend) { | |
// local poll already checked, if used. | ||
bool need_rechecking; | ||
do { | ||
JavaThreadState state = thread->thread_state(); | ||
guarantee(SafepointSynchronize::is_a_block_safe_state(state), "Illegal threadstate encountered: %d", state); | ||
Comment on lines
+121
to
+122
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be outside the loop? Though we are doubling up given the assert in block(). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the thread's state can change during this loop so having the check in the The assert() in Yes, I specifically wanted a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would expect any state change to restore the original state before we get back into this loop. So this seems a little paranoid, but okay I guess. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No on further thought I'm not sure about this. If we take the path to SS::block() then this guarantee must hold. But what if we don't take that path? What if this is called due to a local poll and the thread is executing code that precludes the possibility of a global poll (e.g. holds Threads_lock) - what are the potential valid states in that case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your last comment is exactly why I want this While I've done a lot of Mach5 test runs for this change (plus the fix for JDK-8271251), |
||
if (global_poll()) { | ||
// Any load in ::block() must not pass the global poll load. | ||
// Otherwise we might load an old safepoint counter (for example). | ||
|
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.
Existing: I struggle to understand what this comment means - we are storing the value of safepoint_id so I don't see how the loading of safepoint_id can be reordered. ???
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.
I'll have to check to see who added that comment.
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.
It was added here:
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.
Here's the code block:
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 I think @robehn was trying to separate these two lines of code:
uint64_t safepoint_id = SafepointSynchronize::safepoint_counter();
and
thread->safepoint_state()->set_safepoint_id(safepoint_id);
I think the same situation applies in the updated code, but it is harder
to see in this GitHub view. Especially now that I've added all these
comments.
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.
Thanks and apologies for the digression. I'm assuming the intended required order is:
But the store is a release_store, so it is effectively preceded by a LoadStore|SoreStore barrier. So both loads must come before the store. The loads themselves could be reordered AFAICS but with no affect on correctness. So I remain unclear about the "load dependent store" comment actually relates to.
Oh well, not a problem for this PR.
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.
Okay so we're done with this one for this PR.