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

CRIU restore with -Xdump using CRIUSupport.registerRestoreOptionsFile() #16926

Merged
merged 1 commit into from
Mar 17, 2023

Conversation

JasonFengJ9
Copy link
Member

Added an internal JVM checkpoint hook criuRestoreInitializeDump() invoking criuReloadXDumAgents() after CRIU restore;
Refactored configureDumpAgents() to handle both startup and CRIU restore scenarios;
Not disable dump agent hooks within JIT for CRIU builds;
Added a test to verify -Xdump:java:events=vmstop and -Xdump:java:events=throw,filter=java/lang/OutOfMemoryError,request=exclusive+prepwalk+serial+preempt.

Signed-off-by: Jason Feng fengj@ca.ibm.com

@JasonFengJ9 JasonFengJ9 added comp:vm criu Used to track CRIU snapshot related work labels Mar 15, 2023
@JasonFengJ9 JasonFengJ9 requested a review from tajila March 15, 2023 18:53
@JasonFengJ9
Copy link
Member Author

@dsouzai could you please review JIT changes?
FYI @vijaysun-omr

Copy link
Contributor

@dsouzai dsouzai left a comment

Choose a reason for hiding this comment

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

The JIT changes look fine; however I don't think they're needed. The change in J9Options.cpp only executes once during startup, so it doesn't really impact anything post-restore. As for the change in VMJ9.cpp, the call to ((*vmHooks)->J9HookDisable(vmHooks, J9HOOK_VM_EXCEPTION_CATCH) != 0); essentially boils down to

		if (HOOK_FLAGS(commonInterface, eventNum) & (J9HOOK_FLAG_RESERVED | J9HOOK_FLAG_HOOKED)) {
			rc = -1;
		}

in hookable.cpp which is basically what you've added in the J9VM_OPT_CRIU_SUPPORT guarded sections.

I think the calls to J9HookDisable in the compiler to determine whether the hook is enabled should be cleaned up, but you don't need to bother with it in this PR; I'll open a PR to clean it all up.

Also, on a related note, is it possible for J9HOOK_FLAG_HOOKED to be set but not J9HOOK_FLAG_RESERVED?

@JasonFengJ9
Copy link
Member Author

I think the calls to J9HookDisable in the compiler to determine whether the hook is enabled should be cleaned up, but you don't need to bother with it in this PR; I'll open a PR to clean it all up.

Assuming the cleanup is to replace J9HookDisable with J9_EVENT_IS_HOOKED() || J9_EVENT_IS_RESERVED(), -Xdump at restore won't work if the hook is disabled.
Will remove these JIT changes, and mark this PR depending on the JIT change.

is it possible for J9HOOK_FLAG_HOOKED to be set but not J9HOOK_FLAG_RESERVED?

J9HookRegisterWithCallSitePrivate always set J9HOOK_FLAG_HOOKED | J9HOOK_FLAG_RESERVED.
J9HookUnregister only clears J9HOOK_FLAG_HOOKED.
So it is possible for J9HOOK_FLAG_RESERVED set but not J9HOOK_FLAG_HOOKED.
On the other hand, it won't be the case that J9HOOK_FLAG_HOOKED is set but not J9HOOK_FLAG_RESERVED.

@dsouzai
Copy link
Contributor

dsouzai commented Mar 15, 2023

Spoke to Jason offline. Turns out the compiler changes are required. The reason being that if pre-checkpoint no -Xdump option is set then J9HookDisable will set the J9HOOK_FLAG_DISABLED when called (because J9HOOK_FLAG_RESERVED/J9HOOK_FLAG_HOOKED wont be set). However this means that post-restore, it won't be possible to enable (reserve) this event.

I'll open a PR to clean the JIT calls later, but for 0.38 the compiler changes ensure that we don't set the J9HOOK_FLAG_DISABLED flag.

@tajila
Copy link
Contributor

tajila commented Mar 16, 2023

@JasonFengJ9 Please rebase your changes

@JasonFengJ9 JasonFengJ9 force-pushed the criuxdump branch 2 times, most recently from 518147d to c605343 Compare March 16, 2023 13:25
@JasonFengJ9
Copy link
Member Author

@tajila just rebased, please have a look.

@tajila
Copy link
Contributor

tajila commented Mar 16, 2023

jenkins test sanity xlinux jdk17

@tajila
Copy link
Contributor

tajila commented Mar 16, 2023

jenkins compile win jdk8

runtime/nls/j9vm/j9vm.nls Outdated Show resolved Hide resolved
runtime/oti/j9dump.h Outdated Show resolved Hide resolved
@tajila
Copy link
Contributor

tajila commented Mar 17, 2023

jenkins test sanity xlinux jdk17

Added an internal JVM checkpoint hook criuRestoreInitializeDump()
invoking criuReloadXDumpAgents() after CRIU restore;
refactored configureDumpAgents() to handle both startup and CRIU restore
scenarios;
Not disable dump agent hooks within JIT for CRIU builds;
Added a test to verify -Xdump:java:events=vmstop and
-Xdump:java:events=throw,filter=java/lang/OutOfMemoryError,request=exclusive+prepwalk+serial+preempt.

Signed-off-by: Jason Feng <fengj@ca.ibm.com>
@tajila
Copy link
Contributor

tajila commented Mar 17, 2023

@JasonFengJ9 please rebase

@JasonFengJ9
Copy link
Member Author

@tajila just rebased

@tajila
Copy link
Contributor

tajila commented Mar 17, 2023

jenkins test sanity xlinux jdk17

@tajila
Copy link
Contributor

tajila commented Mar 17, 2023

Only needed compile, will merge now

@tajila tajila merged commit 1c112d1 into eclipse-openj9:master Mar 17, 2023
@tajila
Copy link
Contributor

tajila commented Mar 17, 2023

@JasonFengJ9 please make a PR for 0.38

@JasonFengJ9
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:vm criu Used to track CRIU snapshot related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants