Skip to content
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

Fix to handle suspend/resume of virtual/carrier threads #16943

Merged

Conversation

dipak-bagadiya
Copy link
Contributor

@dipak-bagadiya dipak-bagadiya commented Mar 17, 2023

JVMTI treats a virtual thread and its carrier thread as two separate threads.
If JVMTI suspends a virtual thread, then its carrier thread
should not be suspended and vice-versa. Similarly, if JVMTI resumes
a virtual thread, then its carrier thread should not be resumed and
vice-versa.
In OpenJ9, currently, a mounted virtual thread and its carrier thread
shared the same J9VMThread. The thread state is derived from the
J9VMThread. Due to the sharing of the J9VMThread in the mounted
state, if a mounted virtual thread is suspended, then the JVMTI
functions will also reflect that its carrier thread is suspended.

Currently, "isSuspendedByJVMTI" is the hidden field that holds
the suspend status for the virtual thread only, now it made
available for carrier thread too, i.e., the scope has changed
from "java/lang/VirtualThread" to "java/lang/Thread" threads
Also, it is renamed to "isSuspendedInternal" as this hidden field is
not just specific to JVMTI.

The following functions has changed to set/reset and verify
"isSuspendedInternal" status and halt flag for threads: -

1) SUSPEND THREAD:-
suspendhelper.cpp:suspendThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_suspendImpl
->In case of suspend the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is set if the thread is
mounted, i.e., threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a non-zero value for the thread instance in all cases
regardless of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.

2) RESUME THREAD:-
jvmtiThread.c:resumeThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_resumeImpl
->In case of resume the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is cleared if the thread
is mounted, i.e., threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a zero value for the thread instance in all cases regardless
of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.

3) GETSTATE:-
jvmtiHelpers.c:getThreadState
jvmtiHelpers.c:getVirtualThreadState
->The getThreadState function is changed in such a way that will
verify the "isSuspendedInternal" filed to check the thread is
suspend or not.

The basic concept behind the new changes:-
[mounted + unmounted] set the "isSuspendedInternal" field
[mounted] the halt flag is only modified if the thread object is
stored in targetThread->threadObject
[unmounted] Delay setting the halt flag until the thread mount.

Related: #16689
Signed-off-by: Dipak Bagadiya [email protected]

@dipak-bagadiya dipak-bagadiya marked this pull request as draft March 17, 2023 08:55
@babsingh babsingh self-assigned this Mar 17, 2023
@dipak-bagadiya dipak-bagadiya force-pushed the suspend_resume_status branch 6 times, most recently from 2362f58 to ca18ba7 Compare March 24, 2023 06:39
@babsingh
Copy link
Contributor

@dipak-bagadiya After verifying that sanity.openjdk and extended.openjdk pass, please change PR's state to "Ready for review".

@dipak-bagadiya dipak-bagadiya force-pushed the suspend_resume_status branch 3 times, most recently from cb0afcb to cdb97e6 Compare March 28, 2023 18:08
@dipak-bagadiya
Copy link
Contributor Author

@babsingh I made some changes to the code and ran the build. I'll check the results for sanity. openjdk and extended openjdk, once passed, will modify PR.
Requesting you to Please review the code section.

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two key points:

  • Code changes should not break older JDK versions; and
  • Avoid redundant code via sharing.

runtime/jcl/common/thread.cpp Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Outdated Show resolved Hide resolved
@babsingh
Copy link
Contributor

babsingh commented Mar 30, 2023

Also, please review the commit message in MS Word to fix typos, sentence structure and grammar issues.

@babsingh
Copy link
Contributor

babsingh commented Apr 5, 2023

@dipak-bagadiya The revised suspend, resume and get thread state logic is posted in #16689 (comment).

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dipak-bagadiya

  • Please review the logic thoroughly for functional correctness.
  • Some of the previous feedback was unaddressed.
  • The commit message and PR description need to reflect the new design.

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/thread.cpp Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • There are still issues in the logic which will cause functional failures.
  • Typo in the first word of the PR description and commit message: VMTI -> JVMTI.

runtime/j9vm/javanextvmi.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/thread.cpp Outdated Show resolved Hide resolved
runtime/jcl/common/thread.cpp Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Do the JVMTI tests in extended.openjdk pass for JDK19?
  • Also, test compilation with JDK17.

runtime/jcl/common/thread.cpp Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiHelpers.c Outdated Show resolved Hide resolved
runtime/jvmti/jvmtiThread.c Outdated Show resolved Hide resolved
runtime/jvmti/suspendhelper.cpp Show resolved Hide resolved
@dipak-bagadiya dipak-bagadiya force-pushed the suspend_resume_status branch 4 times, most recently from 7a5b7b7 to 36b28c2 Compare April 20, 2023 16:17
@dipak-bagadiya dipak-bagadiya force-pushed the suspend_resume_status branch 3 times, most recently from 5ab4901 to e8e22fc Compare April 24, 2023 17:54
@babsingh
Copy link
Contributor

jenkins build sanity,extended.openjdk zlinux,win jdk19,jdk20

@babsingh
Copy link
Contributor

jenkins test sanity,extended.openjdk zlinux,win jdk19,jdk20

@babsingh
Copy link
Contributor

jenkins test sanity win jdk8

@dipak-bagadiya
Copy link
Contributor Author

dipak-bagadiya commented Apr 27, 2023

Failures from the below issues are seen in the PR builds:

Further investigation is required for the below failure. The below failure happens in JDK19 and not in JDK20.
https://openj9-jenkins.osuosl.org/job/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal/101/tapResults/

18:05:59.728 0x1d2fe00    j9vm.228    *   ** ASSERTION FAILED ** at /home/jenkins/workspace/Build_JDK19_s390x_linux_Personal/openj9/runtime/vm/VMAccess.cpp:355: (0 == ((vmThread)->publicFlags & J9_PUBLIC_FLAGS_VM_ACCESS))
JVMDUMP039I Processing dump event "traceassert", detail "" at 2023/04/25 18:05:59 - please wait.
JVMDUMP032I JVM requested System dump using '/home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/core.20230425.180559.731559.0001.dmp' in response to an event
JVMDUMP010I System dump written to /home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/core.20230425.180559.731559.0001.dmp
JVMDUMP032I JVM requested Java dump using '/home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/javacore.20230425.180559.731559.0002.txt' in response to an event
JVMDUMP010I Java dump written to /home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/javacore.20230425.180559.731559.0002.txt
JVMDUMP032I JVM requested Snap dump using '/home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/Snap.20230425.180559.731559.0003.trc' in response to an event
JVMDUMP010I Snap dump written to /home/jenkins/workspace/Test_openjdk19_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/output_16824422641535/JCL_Test_none_SCC_0/Snap.20230425.180559.731559.0003.trc
JVMDUMP013I Processed dump event "traceassert", detail "".

Below are failures that look like the known ones, but I did not find any open issues for the same reason, and they seem unrelated to the changes in this PR.

  • sun/security/mscapi/NonAsciiAlias.java.NonAsciiAlias
  • com/sun/jndi/dns/ConfigTests/Timeout.java.Timeout

JVMTI treats a virtual thread and its carrier thread as two separate
threads. If JVMTI suspends a virtual thread, then its carrier thread
should not be suspended and vice-versa. Similarly, if JVMTI resumes
a virtual thread, then its carrier thread should not be resumed and
vice-versa.
In OpenJ9, currently, a mounted virtual thread and its carrier thread
shared the same J9VMThread. The thread state is derived from the
J9VMThread. Due to the sharing of the J9VMThread in the mounted
state,if a mounted virtual thread is suspended, then the JVMTI
functions will also reflect that its carrier thread is suspended.

Currently, "isSuspendedByJVMTI" is the hidden field that holds
the suspend status for the virtual thread only, now it made
available for carrier thread too, i.e. the scope has changed
from "java/lang/VirtualThread" to "java/lang/Thread" threads
Also, it is renamed to "isSuspendedInternal" as this hidden field is
not just specific to JVMTI.

The following functions has changed to set/reset and verify
"isSuspendedInternal" status for threads:-

1) SUSPEND THREAD:-
suspendhelper.cpp:suspendThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_suspendImpl
->In case of suspend the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is set if the thread is
mounted, i.e. threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a non-zero value for the thread instance in all cases
regardless of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.

2) RESUME THREAD:-
jvmtiThread.c:resumeThread:
jvmtiThread.c:jvmtiSuspendResumeCallBack
thread.cpp:Java_java_lang_Thread_resumeImpl
->In case of resume the halt flag
(J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND) is cleared if the thread
is mounted, i.e. threadObject == targetThread->threadObject in
J9VMThread's publicFlags also the hidden field "isSuspendedInternal"
set to a zero value for the thread instance in all cases regardless
of thread being mounted or unmounted.
->If any of the public flags are already set, then the relevant
failure message is assigned.

3) GETSTATE:-
jvmtiHelpers.c:getThreadState
jvmtiHelpers.c:getVirtualThreadState
->The getThreadState function is changed in such a way that will
verify the "isSuspendedInternal" filed to check the thread is
suspend or not.

The basic concept behind the new changes:
[mounted + unmounted] set the "isSuspendedInternal" field
[mounted]   the halt flag is only modified if the thread object is
	    stored in targetThread->threadObject
[unmounted] Delay setting the halt flag until the thread mount.

Related: eclipse-openj9#16689
Signed-off-by: Dipak Bagadiya [email protected]
@babsingh babsingh marked this pull request as ready for review May 2, 2023 22:27
@babsingh
Copy link
Contributor

babsingh commented May 3, 2023

jenkins test sanity alinux64 jdk20

@babsingh
Copy link
Contributor

babsingh commented May 3, 2023

jenkins compile win jdk8

Copy link
Contributor

@babsingh babsingh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, the code changes LGTM. re #16943 (comment): In the earlier PR builds, mostly known failures are seen. The VM-Access assertion is seen on JDK19 and not on JDK20; this is because JDK19 is out-of-service and has become stale.

@babsingh babsingh merged commit aa5727e into eclipse-openj9:master May 3, 2023
dipak-bagadiya added a commit to dipak-bagadiya/aqa-tests that referenced this pull request May 3, 2023
dipak-bagadiya added a commit to dipak-bagadiya/aqa-tests that referenced this pull request May 3, 2023
karianna pushed a commit to adoptium/aqa-tests that referenced this pull request May 3, 2023
Test has been fixed by eclipse-openj9/openj9#16943.

Signed-off-by: Dipak Bagadiya [email protected]

Signed-off-by: Dipak Bagadiya [email protected]
while (J9_ARE_ALL_BITS_SET(targetThread->publicFlags, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND | J9_PUBLIC_FLAGS_VM_ACCESS)) {
omrthread_monitor_wait(targetThread->publicFlagsMutex);
vmFuncs->setHaltFlag(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
} else {
Copy link
Contributor

@babsingh babsingh May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During the review, I was looking at the code with white-space hidden. So, I missed that the else block is indented incorrectly. It needs to include the monitor exit and goto calls.

@dipak-bagadiya This might be the potential fix for the assertion seen in #17334.

fyi @pshipton, we will thoroughly test the code before merging again.

Revised code:

...
#if JAVA_SPEC_VERSION >= 19
J9OBJECT_U32_STORE(currentThread, receiverObject, vm->isSuspendedInternalOffset, 1);
if (receiverObject == targetThread->threadObject)
#endif /* JAVA_SPEC_VERSION >= 19 */
{
	if (currentThread == targetThread) {
		vmFuncs->setHaltFlag(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
	} else {
		vmFuncs->internalExitVMToJNI(currentThread);
		omrthread_monitor_enter(targetThread->publicFlagsMutex);
		VM_VMAccess::setHaltFlagForVMAccessRelease(targetThread, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND);
		if (VM_VMAccess::mustWaitForVMAccessRelease(targetThread)) {
			while (J9_ARE_ALL_BITS_SET(targetThread->publicFlags, J9_PUBLIC_FLAGS_HALT_THREAD_JAVA_SUSPEND | J9_PUBLIC_FLAGS_VM_ACCESS)) {
				omrthread_monitor_wait(targetThread->publicFlagsMutex);
			}
		}
		omrthread_monitor_exit(targetThread->publicFlagsMutex);
		goto vmAccessReleased;
	}
}
...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants