[crac] RFR: RFC: -XX:CPUFeatures=0xnumber for CPU migration [v14]

Thu May 4 18:13:43 UTC 2023

On Thu, 4 May 2023 09:54:05 GMT, Jan Kratochvil <duke at openjdk.org> wrote:

>> Currently if you `-XX:CRaCCheckpointTo` on a better CPU and `-XX:CRaCRestoreFrom` on a worse CPU the restored OpenJDK will crash.
>> 
>> 1. An obvious reason is that JIT-compiled code is using CPU features not implemented on the CPU where the image is restored.
>> 2. A second reason is that glibc has a similar problem, its PLT entries point to CPU optimized functions also crashing on the worse CPU. https://sourceware.org/glibc/wiki/GNU_IFUNC
>> 
>> (1) could be solved somehow automatically by deoptimizing and re-JITing all the JIT code. But that would defeat the performance goal of restoring a ready image in the first place. Therefore there had to be implemented a new OpenJDK option:
>> 
>>> use -XX:CPUFeatures=0xnumber with -XX:CRaCCheckpointTo when you get an error during -XX:CRaCRestoreFrom on a different machine
>> 
>> It is intended to specify the lowest common denominator of all CPUs in a farm. Instead of a possible crash of OpenJDK it will now refuse to run:
>> 
>>> Error occurred during initialization of VM
>>> You have to specify -XX:CPUFeatures=0x421801fcfbd7 during -XX:CRaCCheckpointTo making of the checkpoint; specified -XX:CRaCRestoreFrom file contains CPU features 0x7fff9dfcfbf7; this machine's CPU features are 0x421801fcfbd7; missing features of this CPU are 0x3de79c000020 = 3dnowpref, adx, avx512f, avx512dq, avx512cd, avx512bw, avx512vl, sha, avx512_vpopcntdq, avx512_vpclmulqdq, avx512_vaes, avx512_vnni, clflushopt, clwb, avx512_vbmi2, avx512_vbmi
>> 
>> (2) has been implemented according to Anton Kozlov's idea that glibc can just reset its IFUNC PLT entries any time later (after restore), not just during the first initialization of glibc. That has currently a problem that it has turned out to be very invasive into private glibc structures. It could work somehow with glibc debuginfo (*-debuginfo.rpm or *-dbg.deb) installed but that has been considered as unacceptable requirement just to run CRaC. Therefore I have provided this proof of concept while I will propose such feature for glibc upstream where it is sure easily implementable.
>> 
>> If upstream glibc maintainers do not like the IFUNC reset idea then I do not think this hacky IFUNC reset patching many glibc internal data structures is a good way forward for a 3rd party implementation like CRaC/OpenJDK. In such case I believe one should switch to using GLIBC_TUNABLES environment variable, re-execing OpenJDK after converting the `-XX:CPUFeatures` OpenJDK format into glibc GLIBC_TUNABLES format. Unfortunately there is a precedent OpenJDK upstream has already rejected such re-exec idea in the past: https://github.com/openjdk/crac/pull/31#issuecomment-1275707621
>> 
>> That IMO does not preclude trying the same for this case.
>> 
>> - Debian 11 x86_64: It does not work, glibc is too different and inlined there.
>> - Debian 12 x86_64: It works even without libc6-dbg as its offsets are the default.
>> - Fedora 36 x86_64: It works as on Fedoras glibc debuginfo is embedded.
>
> Jan Kratochvil has updated the pull request incrementally with one additional commit since the last revision:
> 
>   -altstack

> * On older glibcs not supporting macro `CPU_FEATURE_ACTIVE` the disabling of glibc features has no effect (and it may crash the migration even after using `-XX:CPUFeatures=generic`).

What is GLIBC version supporting the flag? We're used to build JDK on some older platform and assume that will work on every newer platform.

And it turns out on my platform used for the builds the option is not supported.

Since it's required to specify TUNABLES in the text form, can we just define needed options names?

I'll continue reviewing this PR.

src/hotspot/cpu/x86/vm_version_x86.cpp line 679:

> 677: 
> 678:   uint64_t disable_CPU   = 0;
> 679:   uint64_t disable_GLIBC = 0;

Are these used in EXCESSIVEx macro? Could you please move these below then, closer to the use.

src/hotspot/cpu/x86/vm_version_x86.cpp line 717:

> 715:         if ((excessive_CPU & CPU_SSE3) ||
> 716:             (excessive_GLIBC & (GLIBC_CMPXCHG16 | GLIBC_LAHFSAHF))) {
> 717:           assert(!(excessive_CPU & CPU_SSE4_2), "(_features & CPU_SSE4_2) cannot happen");

Failed assert prints the failed condition, no need to repeat in the message.

src/hotspot/cpu/x86/vm_version_x86.cpp line 731:

> 729:           // glibc:     && CPU_FEATURE_USABLE_P (cpu_features, FMA)
> 730:           // glibc:     && CPU_FEATURE_USABLE_P (cpu_features, LZCNT)
> 731:           // glibc:     && CPU_FEATURE_USABLE_P (cpu_features, MOVBE)) 

SKARA complians on this line

src/hotspot/cpu/x86/vm_version_x86.cpp line 767:

> 765: #else
> 766: # define IF_ASSERT(x)
> 767: #endif

Exactly the definition of `DEBUG_ONLY`, please use that macro.

src/hotspot/share/runtime/stubCodeGenerator.cpp line 62:

> 60: void StubCodeDesc::thaw() {
> 61:   assert(_frozen, "repeated thaw operation");
> 62:   _frozen = false;

Is it still necessary? I've tried to comment this line out, and checkpoint-restore succeded for me.

-------------

PR Review: https://git.openjdk.org/crac/pull/41#pullrequestreview-1413589870
PR Review Comment: https://git.openjdk.org/crac/pull/41#discussion_r1185335055
PR Review Comment: https://git.openjdk.org/crac/pull/41#discussion_r1185336541
PR Review Comment: https://git.openjdk.org/crac/pull/41#discussion_r1185347208
PR Review Comment: https://git.openjdk.org/crac/pull/41#discussion_r1185333573
PR Review Comment: https://git.openjdk.org/crac/pull/41#discussion_r1185318814