JDK-8310584: GetThreadState reports blocked and runnable for pinned suspended virtual threads #14878

alexmenkov · 2023-07-13T19:18:38Z

The change fixes handling of "suspended" bit in VT state.
The code looks very strange.
java_lang_VirtualThread::RUNNING == 3, so line 803 clears JVMTI_THREAD_STATE_ALIVE(1) and JVMTI_THREAD_STATE_TERMINATED(2)
Per log this code came from loom repo with VT integration.

Testing: tier1-4, updated GetThreadStateMountedTest.java

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8310584: GetThreadState reports blocked and runnable for pinned suspended virtual threads (Bug - P3)

Reviewers

Serguei Spitsyn (@sspitsyn - Reviewer)
David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/14878/head:pull/14878
$ git checkout pull/14878

Update a local copy of the PR:
$ git checkout pull/14878
$ git pull https://git.openjdk.org/jdk.git pull/14878/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14878

View PR using the GUI difftool:
$ git pr show -t 14878

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14878.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-07-13T19:19:55Z

👋 Welcome back amenkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-07-13T19:21:51Z

@alexmenkov The following labels will be automatically applied to this pull request:

hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-07-13T19:26:18Z

Webrevs

01: Full - Incremental (084e9707)
00: Full (e2cf7c3c)

sspitsyn

This looks good. Thank you for filing bug and fixing it!
I've one question besides this fix.
Thanks,
Serguei

openjdk · 2023-07-14T03:44:35Z

@alexmenkov 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:

8310584: GetThreadState reports blocked and runnable for pinned suspended virtual threads

Reviewed-by: sspitsyn, dholmes

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 63 new commits pushed to the master branch:

201e3bc: 8291065: Creating a VarHandle for a static field triggers class initialization
a53345a: 8287061: Support for rematerializing scalar replaced objects participating in allocation merges
3236ba0: 8312189: ProblemList serviceability/jvmti/vthread/VThreadTLSTest/VThreadTLSTest.java#id1
5cc71f8: 8312089: Simplify and modernize equals, hashCode, and compareTo in java.nio and implementation code
6a09992: 8311968: Clarify Three-letter time zone IDs in java.util.TimeZone
6ae152d: 8312196: ProblemList test/hotspot/jtreg/applications/ctw/modules/jdk_crypto_ec.java
295ae11: 8312195: Changes in JDK-8284493 use wrong copyright syntax
f975be4: 8284493: Improve computeNextExponential tail performance and accuracy
e737968: 8308398: Move SunEC crypto provider into java.base
69a46c2: 8310157: Allow void-returning filters for MethodHandles::collectCoordinates
... and 53 more: https://git.openjdk.org/jdk/compare/15195e6018c104cbeca0f387ae18bff106792908...master

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 master branch, type /integrate in a new comment.

sspitsyn · 2023-07-14T03:51:15Z

src/hotspot/share/prims/jvmtiEnvBase.cpp

-    state &= ~java_lang_VirtualThread::RUNNING;
-    state |= JVMTI_THREAD_STATE_ALIVE | JVMTI_THREAD_STATE_RUNNABLE | JVMTI_THREAD_STATE_SUSPENDED;
+    state |= JVMTI_THREAD_STATE_SUSPENDED;
  }


One question unrelated to this bug and your fix.
I wonder if any check and handling is needed for the case:
if (ext_suspended && ((state & JVMTI_THREAD_STATE_ALIVE) == 0))
Not sure this condition is even possible. But do we need to add an assert here?

AFAIU it's possible in the case when we have terminated VT and JvmtiVTSuspender is requested to suspend all virtual threads

So there is a window in which a VT is marked as terminated yet is still visible for actions like this? For regular threads we would always have filtered out thread in the process of exiting. Seeing terminated threads seems potentially problematic but perhaps all the VT code is prepared to handle this.

I hope there is no such window in GetThreadState() case, but get_vthread_state method is also called from MultipleStackTracesCollector::fill_frames and there is a comment there:
// Note that either or both of thr and thread_oop
// may be null if the thread is new or has exited.

I keep this check for safety (though fill_frames does not care about suspend bit)

sspitsyn · 2023-07-14T03:54:42Z

@alexmenkov Do you consider backporting this to 21?

dholmes-ora

The change seems consistent with the definition of GetThreadState. But I note that the interrupt bit should also only be set if the target is alive.

alexmenkov · 2023-07-14T18:31:25Z

@alexmenkov Do you consider backporting this to 21?

maybe it makes sense.

alexmenkov · 2023-07-14T18:41:01Z

The change seems consistent with the definition of GetThreadState. But I note that the interrupt bit should also only be set if the target is alive.

we get interrupt bit from Thread object, so the value is consistent with terminated state. suspend bit is a bit different - see my reply to Serguei

dholmes-ora · 2023-07-17T04:37:08Z

The change seems consistent with the definition of GetThreadState. But I note that the interrupt bit should also only be set if the target is alive.

we get interrupt bit from Thread object, so the value is consistent with terminated state. suspend bit is a bit different - see my reply to Serguei

Sorry I don't follow. I don't see anything that prevents the target from terminating after you have read the interrupt bit from the thread object, but before you read the actual state.

AlanBateman · 2023-07-17T09:43:59Z

Sorry I don't follow. I don't see anything that prevents the target from terminating after you have read the interrupt bit from the thread object, but before you read the actual state.

The virtual thread state and the interrupt status are separate. That's okay for the suspended case, assuming not resumed while JVMTI GetThreadState executes. If not suspended then it looks like it could give an inconsistent view of the state. I don't know why GetThreadState defined a state flag for interrupted.

alexmenkov · 2023-07-17T22:30:08Z

So AFAIU GetThreadState for platform threads (get_thread_state_base) don't have similar issue because suspended/interrupted values are read after reading main thread state value.
For virtual threads suspended/interrupted values are read before. There is a comment in the line 796: // This call can trigger a safepoint, so thread_oop must not be used after it. I suppose this is the reason to read them earlier.
Then I think we need to ensure the thread is still alive before applying suspended/interrupted bits

dholmes-ora

Seems fine to me, assuming all tests pass.

Thanks.

sspitsyn

The update looks good.
Need to make sure there are no new failures though.
Thanks,
Serguei

alexmenkov · 2023-07-18T20:16:09Z

/integrate

openjdk · 2023-07-18T20:16:43Z

Going to push as commit af5bf81.
Since your change was applied there have been 71 commits pushed to the master branch:

b4dce0d: 8310814: Clarify the targetName parameter of Lookup::findClass
9905f75: 8311040: JFR: RecordedThread::getOSThreadId() should return -1 if thread is virtual
1dfb0fb: 8311188: Simplify and modernize equals and hashCode in java.text
1fc726a: 8312163: Crash in dominance check when compiling unnamed patterns
b20dc1e: 8310629: java/security/cert/CertPathValidator/OCSP/OCSPTimeout.java fails with RuntimeException Server not ready
4b9ec82: 8310355: Move the stub test from initialize_final_stubs() to test/hotspot/gtest
8ec136e: 8312072: Deprecate for removal the -Xnoagent option
fbe51e3: 8312127: FileDescriptor.sync should temporarily increase parallelism
201e3bc: 8291065: Creating a VarHandle for a static field triggers class initialization
a53345a: 8287061: Support for rematerializing scalar replaced objects participating in allocation merges
... and 61 more: https://git.openjdk.org/jdk/compare/15195e6018c104cbeca0f387ae18bff106792908...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-07-18T20:16:49Z

@alexmenkov Pushed as commit af5bf81.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

fix

e2cf7c3

openjdk bot added the rfr Pull request is ready for review label Jul 13, 2023

openjdk bot added serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jul 13, 2023

sspitsyn approved these changes Jul 14, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 14, 2023

sspitsyn reviewed Jul 14, 2023

View reviewed changes

dholmes-ora reviewed Jul 14, 2023

View reviewed changes

check that VT is alive before applying suspend/interrupt bits

084e970

dholmes-ora approved these changes Jul 18, 2023

View reviewed changes

sspitsyn approved these changes Jul 18, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jul 18, 2023

openjdk bot closed this Jul 18, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 18, 2023

alexmenkov deleted the vt_state_susp branch July 18, 2023 20:22

JDK-8310584: GetThreadState reports blocked and runnable for pinned suspended virtual threads #14878

JDK-8310584: GetThreadState reports blocked and runnable for pinned suspended virtual threads #14878

Conversation

alexmenkov commented Jul 13, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jul 13, 2023

Uh oh!

openjdk bot commented Jul 13, 2023

Uh oh!

mlbridge bot commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sspitsyn Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

dholmes-ora Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexmenkov Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

sspitsyn commented Jul 14, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

alexmenkov commented Jul 14, 2023

Uh oh!

alexmenkov commented Jul 14, 2023

Uh oh!

dholmes-ora commented Jul 17, 2023

Uh oh!

AlanBateman commented Jul 17, 2023

Uh oh!

alexmenkov commented Jul 17, 2023

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

sspitsyn left a comment

Choose a reason for hiding this comment

Uh oh!

alexmenkov commented Jul 18, 2023

Uh oh!

openjdk bot commented Jul 18, 2023

Uh oh!

openjdk bot commented Jul 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

alexmenkov commented Jul 13, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 13, 2023 •

edited

Loading

openjdk bot commented Jul 14, 2023 •

edited

Loading

dholmes-ora Jul 14, 2023 •

edited

Loading