8385359: [CRaC] aarch64: c7g.medium <-> c8g.medium FAIL: vector length

[jankratochvil]

It is a draft src/hotspot/share/runtime/crac_engine.cpp needs to be reworked. I believe crlib_image_constraints::get_failed_bitmap should become get_bitmap as this code needs to query image's CPUFeatures to find out its vector length to configure the CPU before restore. Otherwise one could also create some new user data. @rvansa?

---

• I confirm that I make this contribution in accordance with the OpenJDK Interim AI Policy.

---

Progress

• Change must not contain extraneous whitespace

Issue

• JDK-8385359: [CRaC] aarch64: c7g.medium <-> c8g.medium FAIL: vector length (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 331

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 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]

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

[rvansa]

The image constraints was meant as structured data that is validated by the engine. The check is performed in the engine as to move JVM out of the loop when there are more images and score-based criteria. On the other hand, user_data is meant to be data opaque to the engine - so this fits better into the picture.

The only downside of adding get_bitmap and get_label methods to the API is that in practice the values will have to be stored in the configuration until it is destroyed - this is not a strong con, the data is minimal. Duplicating entry storage in user data ain't ideal either, anyway. Let's not change get_failed_bitmap, though - the API should evolve append-only.

@TimPushkin what's your opinion on the suggested API change?

[TimPushkin]

Adding get_bitmap and get_label to the constraints API LGTM: if we have setters it feels natural to have getters.

[TimPushkin]

Nitpick: consider using pre_restore instead of restore_pre — the former convention seems more common in HotSpot

[rvansa]

It seems to me that if require_label/bitmap copies the name it would be pretty inconsistent to not copy it here. On the other hand it is not possible to copy an arbitrary user pointer. One way to work this around would be to not specify the name at all, and let engine pass it as a hook argument (for any tag stored).

However, there is a bigger problem with this API design: the whole point of image constraints API extension is to validate the image & current environment compatibility externally. The image lookup can happen out of the process, e.g. on a remote server storing multiple images - with this hook API we would need to retrieve the tag for each image, send it over to this process, invoke the hook (looking for a first match, or doing that will all images, in case we are validating multiple tags?) and then forward the choice back to the server, and then get the right image... The API does not say if the hook is mutating, or idempotent. This is totally invalidating the point of the API.

I think that we need to break VM_Version::process_image_cpu_features into two parts: one that does the compatibility check (and that would be 'descriptor match'), and the other part tries to invoke prctl and fails the restore when the result is unexpected.

[rvansa]

Mostly LGTM.

In the future it would be nice to move the static stubs into a base class, so we don't need to repeat then for every platform, but let's not slow down the integration here.

[jankratochvil]

It seems to me that if require_label/bitmap copies the name it would be pretty inconsistent to not copy it here. On the other hand it is not possible to copy an arbitrary user pointer. One way to work this around would be to not specify the name at all, and let engine pass it as a hook argument (for any tag stored).

I do not get all the parts but regarding the copying we were discussing it here with @TimPushkin and I find it would complicate the current code and the caller currently does not need it at all. If it is ever needed in the future one can start copying it in a fully backward compatible way.

Which reminds me also that I introduced the new API call get_bitmap to preserve existing get_failed_bitmap but IMO it would be perfectly backward compatible to just rename existing get_failed_bitmap to get_bitmap.

TBH I also do not understand why to currently be so strict with backward compatibility as it is all shipped in one bundle (even downstream). One can start considering backward compatibility if there is some 3rd party API implementation.