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

Pass threadObject to walkContinuationStackFrames #18180

Merged

Conversation

babsingh
Copy link
Contributor

@babsingh babsingh commented Sep 21, 2023

threadObject is required in jvmtiFollowReferences to get thread
specific details such as the TID.

So, threadObject needs to be passed via walkContinuationStackFrames
so that it is accessible in jvmtiFollowReferences.

walkContinuationStackFrames is invoked either for a
[1] J9VMContinuation in the Continuation object or
[2] J9VMThread->currentContinuation.

In [1], if the Continuation is fully mounted, then it stores the state
of the CarrierThread. Otherwise, it stores the state of the
VirtualThread.

In [2], if J9VMThread->currentContinuation is not NULL, then it stores
the state of the CarrierThread because the VirtualThread is mounted on
the J9VMThread. The CarrierThread is retrieved from
J9VMThread->carrierThreadObject.

We have assumed that the state of the Continuation or J9VMContinuation
doesn't change while walkContinuationStackFrames is invoked i.e. the
VirtualThread shouldn't be mounted or unmounted. Otherwise, the values
of the VirtualThread and CarrierThread objects accessed via the
Continuation object and J9VMThread->carrierThreadObject can vary and
cause a crash during walkContinuationStackFrames.

Related: #17712

@babsingh
Copy link
Contributor Author

babsingh commented Oct 4, 2023

There are VM, GC and JIT changes in this PR. @fengxue-IS @0xdaryl @dmitripivkine Requesting your review.

fyi @tajila

runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
runtime/compiler/control/HookedByTheJit.cpp Outdated Show resolved Hide resolved
@babsingh babsingh force-pushed the fix_jvmtimounttransition branch 2 times, most recently from e7daeb7 to 3e67460 Compare October 4, 2023 20:13
@babsingh
Copy link
Contributor Author

babsingh commented Oct 4, 2023

@fengxue-IS I have addressed all of your feedback.

Copy link
Contributor

@fengxue-IS fengxue-IS left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@0xdaryl 0xdaryl left a comment

Choose a reason for hiding this comment

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

JIT changes look fine.

threadObject is required in jvmtiFollowReferences to get thread
specific details such as the TID.

So, threadObject needs to be passed via walkContinuationStackFrames
so that it is accessible in jvmtiFollowReferences.

walkContinuationStackFrames is invoked either for a
[1] J9VMContinuation in the Continuation object or
[2] J9VMThread->currentContinuation

In [1], if the Continuation is fully mounted, then it stores the state
of the CarrierThread. Otherwise, it stores the state of the
VirtualThread.

In [2], if J9VMThread->currentContinuation is not NULL, then it stores
the state of the CarrierThread because the VirtualThread is mounted on
the J9VMThread. The CarrierThread is retrieved from
J9VMThread->carrierThreadObject.

We have assumed that the state of the Continuation or J9VMContinuation
doesn't change while walkContinuationStackFrames is invoked i.e. the
VirtualThread shouldn't be mounted or unmounted. Otherwise, the values
of the VirtualThread and CarrierThread objects accessed via the
Continuation object and J9VMThread->carrierThreadObject can vary and
cause a crash during walkContinuationStackFrames.

Related: eclipse-openj9#17712

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
@babsingh
Copy link
Contributor Author

babsingh commented Oct 4, 2023

The last push just updated the commit message to reflect the latest changes. Also, the PR description was updated.

@babsingh
Copy link
Contributor Author

babsingh commented Oct 4, 2023

jenkins test sanity zlinux jdk11,jdk21

@babsingh
Copy link
Contributor Author

babsingh commented Oct 4, 2023

jenkins compile win jdk11,jdk21

@amicic
Copy link
Contributor

amicic commented Oct 5, 2023

While I see an improvement, isn't there still a problem JVMTI walking stack frames while they are potentially changing (of a mounted VT)?
Concurrent GCs have a whole protocol to ensure they don't walk it, while VT is mounted (and VT does not get mounted while GC walks it).
A more basic question for my understanding: how is this functionality safe even with just ordinary/platform threads without ensuring the thread does not have VM access (for example by acquiring exclusive VM access)?

@babsingh
Copy link
Contributor Author

babsingh commented Oct 5, 2023

re #18180 (comment):

There is synchronization code which prevents virtual threads to be mounted or unmounted while they are being inspected via JVMTI.

@babsingh
Copy link
Contributor Author

babsingh commented Oct 5, 2023

The PR builds look good except the known failure (more details below).

https://openj9-jenkins.osuosl.org/job/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal/38/

cmdLineTester_criu_random_0 failure is a known issue: #14974 (comment).

19:58:46  Testing: Second Restore of Criu Checkpoint Image
19:58:46  Test start time: 2023/10/04 23:58:46 Coordinated Universal Time
19:58:46  Running command: bash /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/criu/criuRandomScript.sh /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal_testList_0/aqa-tests/TKG/../../jvmtest/functional/cmdLineTests/criu /home/jenkins/workspace/Test_openjdk21_j9_sanity.functional_s390x_linux_Personal_testList_0/openjdkbinary/j2sdk-image/bin/java " -Denable.j9internal.checkpoint.security.api.debug=true " "SecondRestore"
19:58:46  Time spent starting: 12 milliseconds
19:58:46  Time spent executing: 35 milliseconds
19:58:46  Test result: FAILED
19:58:46  Output from test:
19:58:46   [OUT] start running script
19:58:46  >> Success condition was not found: [Output match: Different random values]
19:58:46  >> Success condition was not found: [Output match: Post-checkpoint]
19:58:46  >> Success condition was not found: [Output match: Second Restore]
19:58:46  >> Failure condition was not found: [Output match: CRIU is not enabled]
19:58:46  >> Failure condition was not found: [Output match: Operation not permitted]
19:58:46  >> Success condition was not found: [Output match: Thread pid mismatch]
19:58:46  >> Success condition was not found: [Output match: do not match expected]
19:58:46  >> Success condition was not found: [Output match: Unable to create a thread:]
19:58:46  >> Failure condition was not found: [Output match: Can't open dir cpData: No such file or directory]
19:58:46  >> Failure condition was not found: [Output match: Could not dump the JVM processes, err=-70]
19:58:46  >> Failure condition was not found: [Output match: User requested Java dump using]

@amicic
Copy link
Contributor

amicic commented Oct 5, 2023

re #18180 (comment):

There is synchronization code which prevents virtual threads to be mounted or unmounted while they are being inspected via JVMTI.

This is good, but my original concern was more about the other way around - walking even if VT is already mounted. This change makes sure that a proper threadObject was snapshotted ahead of walk, but seems like JVMTI proceeds with the walk (of a stack that is potentially changing).

Another thing I'm curios about is the details of that synchronization - is there something we can share with Concurrent GC synchronization. It is outside of scope of these change - something we can talk about later, in person.

@babsingh
Copy link
Contributor Author

babsingh commented Oct 5, 2023

but seems like JVMTI proceeds with the walk (of a stack that is potentially changing).

In this case, JVMTI will either halt the thread or acquire exclusive VM access to prevent the thread stack from changing. In jvmtiFollowReferences, we acquire exclusive VM access.

is there something we can share with Concurrent GC synchronization

@LinHu2016 and @fengxue-IS had a discussion about this in the past. JVMTI synchronization increments a counter or spins until the counter conditions are met. JVMTI synchronization is blocking and heavily dependent on spinning. The GC took a passive (non-blocking) approach to skip.

@babsingh
Copy link
Contributor Author

babsingh commented Oct 5, 2023

@amicic @dmitripivkine @0xdaryl Can I ask one of you to merge this PR if everything looks good?

@dmitripivkine
Copy link
Contributor

Failure in s390 build is unrelated (CRIU issue)

@dmitripivkine dmitripivkine merged commit 9036526 into eclipse-openj9:master Oct 5, 2023
7 of 9 checks passed
babsingh added a commit to babsingh/openj9 that referenced this pull request Oct 6, 2023
This PR is needed to enable the Skynet benchmark.

The changes in eclipse-openj9#18180 pass threadObject to
walkContinuationStackFrames.

The GC can walk the continuations until the associated native
J9VMContinuation structure is freed.

Since we rely on Continuation.vthread in walkContinuationStackFrames,
Continuation.vthread can only be unset once the native continuation
structure is freed.

Now, we set Continuation.vthread to NULL after the continuation is
freed. This will prevent a segfault which can occur due to
eclipse-openj9#18180.

Continuation.vthread is also used in JVMTI to not suspend virtual
threads once they enter the last unmount phase. This won't work anymore
because Continuation.vthread is no longer set to NULL at the start of
the last unmount phase. To address this issue, another continuation
state has been added to indicate that a virtual thread's continuation
has entered the last unmount phase.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
midronij pushed a commit to midronij/openj9 that referenced this pull request Oct 26, 2023
This PR is needed to enable the Skynet benchmark.

The changes in eclipse-openj9#18180 pass threadObject to
walkContinuationStackFrames.

The GC can walk the continuations until the associated native
J9VMContinuation structure is freed.

Since we rely on Continuation.vthread in walkContinuationStackFrames,
Continuation.vthread can only be unset once the native continuation
structure is freed.

Now, we set Continuation.vthread to NULL after the continuation is
freed. This will prevent a segfault which can occur due to
eclipse-openj9#18180.

Continuation.vthread is also used in JVMTI to not suspend virtual
threads once they enter the last unmount phase. This won't work anymore
because Continuation.vthread is no longer set to NULL at the start of
the last unmount phase. To address this issue, another continuation
state has been added to indicate that a virtual thread's continuation
has entered the last unmount phase.

Signed-off-by: Babneet Singh <sbabneet@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants