Configure the wait time for checkpoint safety #18489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
singh264
commented
Nov 20, 2023
runtime/criusupport/criusupport.cpp
Outdated
| @@ -844,7 +851,12 @@ Java_org_eclipse_openj9_criu_CRIUSupport_checkpointJVMImpl(JNIEnv *env, | |||
| for (UDATA i = 0; (0 != notSafeToCheckpoint) && (i <= maxRetries); i++) { | |||
| releaseSafeOrExcusiveVMAccess(currentThread, vmFuncs, safePoint); | |||
| vmFuncs->internalExitVMToJNI(currentThread); | |||
| omrthread_nanosleep(10000); | |||
There was a problem hiding this comment.
I observe that the thread is unable to sleep for less than 1000000 ns (1 ms) on a Linux x86_64 machine.
|
@tajila requesting your review. |
singh264
force-pushed
the
openj9_issues_15806
branch
from
d833405
to
648fad4
Compare
|
@tajila requesting your review. |
tajila
reviewed
Nov 30, 2023
runtime/criusupport/criusupport.cpp
Outdated
| @@ -690,6 +691,12 @@ Java_org_eclipse_openj9_criu_CRIUSupport_checkpointJVMImpl(JNIEnv *env, | |||
| UDATA success = 0; | |||
| bool safePoint = J9_ARE_ANY_BITS_SET(vm->extendedRuntimeFlags, J9_EXTENDED_RUNTIME_OSR_SAFE_POINT); | |||
| UDATA maxRetries = vm->checkpointState.maxRetryForNotCheckpointSafe; | |||
| U_64 sleepNanoseconds = vm->checkpointState.sleepNanosecondsForNotCheckpointSafe; | |||
There was a problem hiding this comment.
i dont think we need this complexity. By toggling max retry and nanosleep we can express the flexibility needed for this.
runtime/criusupport/criusupport.cpp
Outdated
| @@ -844,7 +851,12 @@ Java_org_eclipse_openj9_criu_CRIUSupport_checkpointJVMImpl(JNIEnv *env, | |||
| for (UDATA i = 0; (0 != notSafeToCheckpoint) && (i <= maxRetries); i++) { | |||
| releaseSafeOrExcusiveVMAccess(currentThread, vmFuncs, safePoint); | |||
| vmFuncs->internalExitVMToJNI(currentThread); | |||
| omrthread_nanosleep(10000); | |||
There was a problem hiding this comment.
you can use omrthread_sleep and set it to a parameter for which the default is 10ms
runtime/vm/jvminit.c
Outdated
| @@ -3926,6 +3926,9 @@ processVMArgsFromFirstToLast(J9JavaVM * vm) | |||
| vm->checkpointState.lastRestoreTimeMillis = -1; | |||
| /* Its unclear if we need an option for this, so we can keep the init here for the time being */ | |||
| vm->checkpointState.maxRetryForNotCheckpointSafe = 100; | |||
| vm->checkpointState.sleepNanosecondsForNotCheckpointSafe = 10000; | |||
There was a problem hiding this comment.
I think you only need sleepNanosecondsForNotCheckpointSafe. But this can be milliseconds instead.
singh264
force-pushed
the
openj9_issues_15806
branch
from
648fad4
to
9da089f
Compare
I have simplified the code that increases the wait time for checkpoint safety from 1 ms to 2 s by sleeping for a fixed time per checkpoint safety check. |
|
You can add two options And then we can increase those values (higher than default) for this test |
|
What would be the default, lower bound, and upper bound of the Would throwing an error be appropriate if the |
0 <= maxRetryForNotCheckpointSafe < ∞ 1 <= sleepMillisecondsForNotCheckpointSafe < ∞
Ya we can throw the standard errors. Note, we won't be documenting these options. Its not something we intend users will need. The default should be enough. |
|
A stacktrace of |
singh264
force-pushed
the
openj9_issues_15806
branch
from
9da089f
to
467be28
Compare
singh264
changed the title
singh264
force-pushed
the
openj9_issues_15806
branch
from
467be28
to
5b8e8e7
Compare
|
@tajila I added options to configure the checkpoint safety wait time with Re-requesting your review. |
|
Can you do some grinders on x, p and z to verify that the test will not fail. Do 50x for each and try to spread them out on different machines. |
The grinder on x passed, the cause of the failures for the grinders on all of the p machines and on some of the z machines seems to be due to the setup of CRIU on the machines, and the grinders do not indicate failures that occur due to a timeout while code is being executed in @NotCheckpointSafe frames or failures that occur due to the test timing out by the race condition. x: https://hyc-runtimes-jenkins.swg-devops.com/job/Grinder/36552 |
singh264
force-pushed
the
openj9_issues_15806
branch
from
b7e395e
to
157c145
Compare
singh264
changed the title
|
okay, enable the test on X only for now. |
Increase the wait time for checkpoint safety from 1 ms to 2 s for threads initializing a class, and fix the failure in the re-enabled MethodTypeDeadlockTest that occurs due to an insufficient wait time for checkpoint safety for threads initializing a class. Issue: eclipse-openj9#15806 Signed-off-by: Amarpreet Singh <[email protected]>
Simplify the code that increases the wait time for checkpoint safety from 1 ms to 2 s by sleeping for a fixed time per checkpoint safety check. Issue: eclipse-openj9#15806 Signed-off-by: Amarpreet Singh <[email protected]>
Add -XX:maxRetryForNotCheckpointSafe= and -XX:sleepMillisecondsForNotCheckpointSafe= to control the checkpoint safety wait time for MethodTypeDeadlockTest. Issue: eclipse-openj9#15806 Signed-off-by: Amarpreet Singh <[email protected]>
Fix the race condition in MethodTypeDeadlockTest for the shared resource testResult.lockStatus by modifying its type from volatile int to AtomicInteger, and avoid the test from timing out when the main thread is blocked while waiting for testResult.lockStatus to reflect the count of the worker threads that were started by the main thread. Issue: eclipse-openj9#15806 Signed-off-by: Amarpreet Singh <[email protected]>
Disable criu_nonPortableRestore tests on s390x due to the limited number of s390x machines with proper CRIU setup. Issue: eclipse-openj9#15806 Signed-off-by: Amarpreet Singh <[email protected]>
singh264
force-pushed
the
openj9_issues_15806
branch
from
157c145
to
daf7103
Compare
I have disabled the test on z, it is now enabled only for x. |
|
jenkins test sanity alinux64 jdk17 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Configure the checkpoint safety wait time with
-XX:maxRetryForNotCheckpointSafe= and
-XX:sleepMillisecondsForNotCheckpointSafe=, and increase
the default checkpoint safety wait time from 1 ms to 1
s.
Configure the checkpoint safety wait time for the
re-enabled MethodTypeDeadlockTest to be 2 s to allow the
threads to initialize a class, and fix the failure in
the MethodTypeDeadlockTest that occurs due to an
insufficient checkpoint safety wait time for the threads
initializing a class.
Fix the race condition in MethodTypeDeadlockTest for the
shared resource testResult.lockStatus by modifying its
type from volatile int to AtomicInteger, and avoid the
test from timing out when the main thread is blocked
while waiting for testResult.lockStatus to reflect the
count of the worker threads that were started by the
main thread.
Issue: #15806
Signed-off-by: Amarpreet Singh [email protected]