Skip to content

8355974: [CRaC] Move CPUFeatures verification to the parent process of JVM #227

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

Open
wants to merge 28 commits into
base: crac
Choose a base branch
from

Conversation

jankratochvil
Copy link
Contributor

@jankratochvil jankratochvil commented Apr 27, 2025

There was originally a mistake:

  • restoring JVM did restore the image
  • the restored JVM started checking whether CPU Features of the new host >= CPU Features of the checkpoint host

That is difficult as glibc is already configured (IFUNC) in the image for the checkpoint host and calling any such glibc functions in the restored image will crash (as the advanced instructions from misconfigured IFUNC are not available). Some glibc functions had to be reimplemented in a dummy way inside JVM due to this misdesign.

This patch changes it to:

  • restoring JVM checks cpufeatures user data in the image against current CPU Features
  • the restored JVM is started only if the CPU Features are satisfied, restored JVM no longer has to verify anything

The patch is a bit of a kitchen sink, there are various improvements of the CPU Features code.


Progress

  • Change must not contain extraneous whitespace

Issue

  • JDK-8355974: [CRaC] Move CPUFeatures verification to the parent process of JVM (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 227

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/crac/pull/227.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 27, 2025

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

@openjdk
Copy link

openjdk bot commented Apr 27, 2025

@jankratochvil This change is no longer ready for integration - check the PR body for details.

@openjdk
Copy link

openjdk bot commented Apr 27, 2025

⚠️ @jankratochvil This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 29, 2025
@mlbridge
Copy link

mlbridge bot commented Apr 29, 2025

@TimPushkin
Copy link
Collaborator

I have not finished the review, mainly just looked at the runtime changes. Will continue later, or maybe someone else will.

@jankratochvil
Copy link
Contributor Author

That one failure: Pre-submit tests - linux-x64 / test - Build / test = jdk/crac/fileDescriptors/LoggingVMlogOpenTestNegative.java will IMO disappear after the change gets accepted. The testcase has been changed so that if you run it from the freshly built JVM it works. But Github IIUC runs the parent JVM pre-built, not from the fresh build: /home/runner/work/crac/crac/bundles/jdk/jdk-25/bin/java. Or where is this bundle JVM from?

@TimPushkin
Copy link
Collaborator

I believe the tests in CI are running on the JDK built from the source branch. The bundle is built and uploaded here and is downloaded for testing here.

@rvansa
Copy link
Collaborator

rvansa commented Apr 30, 2025

@jankratochvil The test is failing for me locally, too - does it pass on your machine? I had the test fail for something unrelated.

Could you create JDK issue and refer to that in the title?

@TimPushkin
Copy link
Collaborator

@jankratochvil I think Radim meant creating a JBS issue for this PR itself and updating the title of the PR to be "<JBS issue №>: Move CPUFeatures...".

The test should ideally be fixed as part of this PR, not separately, but I haven't looked into it.

@jankratochvil jankratochvil changed the title Move CPUFeatures verification to the parent process of JVM 8355974: [CRaC] Move CPUFeatures verification to the parent process of JVM Apr 30, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2025
@openjdk openjdk bot added ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2025
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 30, 2025
return;
}
if (!VM_Version::cpu_features_binary_check(present ? &data : nullptr)) {
log_error(crac)("Image %s has incompatible CPU features in its user data %s", CRaCRestoreFrom, engine.cpufeatures_userdata_name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the user doesn't really need to know the name of the user data in this case, just that the image is incompatible with their CPU. Omitting this would also allow making cpufeatures_userdata_name a .cpp static as it was before.

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

Successfully merging this pull request may close these issues.

4 participants