[crac] RFR: 8378446: [CRaC] Replace libcrexec with libcriuengine [v2]
Timofei Pushkin
tpushkin at openjdk.org
Wed Feb 25 11:35:51 UTC 2026
On Tue, 24 Feb 2026 13:33:46 GMT, Radim Vansa <rvansa at openjdk.org> wrote:
>> After [JDK-8376959](https://bugs.openjdk.org/browse/JDK-8376959) the only use of libcrexec is running CRIU; in fact the code already is quite CRIU-dependent. The point of this task is to stop pretending that libcrexec is generic, and move code from criuengine binary (now removed) into libcriuengine implementing the C/R API.
>>
>> This removes smuggling of some parameters through environment variables and execution of the criuengine. We still require anexecuteable (now called `criuhelper`) to become parent of the restored process, but this has significantly simplified implementation.
>>
>> Replacement of communication between the restoring and restored process through signals and temporary files is out of scope of this change.
>
> Radim Vansa has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains three commits:
>
> - Merge remote-tracking branch 'origin/crac' into crexec_to_criuengine
> - Fix Windows build
> - 8378446: [CRaC] Replace libcrexec with libcriuengine
make/modules/java.base/Launcher.gmk line 119:
> 117: STATIC_LIB := false, \
> 118: ))
> 119: TARGETS += $(BUILD_CRIUHELPER)
Nitpick: indentation changed from 2 to 4, looking at the code above it should stay 2, except for eval args where it is 4
make/modules/java.base/Launcher.gmk line 123:
> 121: endif
> 122:
> 123: ################################################################################
Nitpick:
- this line should remain at the end because [the mainline has it there](https://github.com/openjdk/crac/blob/e452d47867ca76449365d14f61332d0eb1a096ac/make/modules/java.base/Launcher.gmk#L106),
- between targets there are not single lines now but these headers:
##############################################################################
## Build criuhelper
##############################################################################
make/modules/java.base/Lib.gmk line 249:
> 247: JDK_LIBS := java.base:libcrcommon, \
> 248: ))
> 249: TARGETS += $(BUILD_LIBCRIUENGINE)
Indentation should be 2, except for named args
src/hotspot/share/runtime/crac_engine.cpp line 97:
> 95: log_error(crac)("CRaCEngine path %s is not a library", CRaCEngine);
> 96: return false;
> 97: }
The error message should explain the expected naming.
Or maybe we can drop this naming check at all? If it's not a library we'll fail when trying to link it.
src/hotspot/share/runtime/crac_engine.cpp line 313:
> 311: if (is_vm_statically_linked()) {
> 312: log_warning(crac)("Cannot load CRaC engine API entrypoint '%s' from %s", resolved_engine_func, path);
> 313: // Maybe the deprecated short name was used, in that case find_engine did not amend the 'engine'
Maybe I miss a case where this may happen but shouldn't it be handled here: https://github.com/rvansa/crac/blob/807167d37ca7cf6e2b079d18a45a768e24478083/src/hotspot/share/runtime/crac_engine.cpp#L147?
src/java.base/linux/native/libcriuengine/README.md line 3:
> 1: # CRIU Engine Library
> 2:
> 3: This is an implementation the C/R API (see `src/hotspot/share/include/crlib/`) that is used as Checkpoint/Restore Engine for CRaC. This implements the checkpoint and restore through invocation of CRIU executable.
Suggestion:
This is an implementation of the C/R API (see `src/hotspot/share/include/crlib/`) that is used as Checkpoint/Restore Engine for CRaC. This implements the checkpoint and restore through invocation of CRIU executable.
src/java.base/linux/native/libcriuengine/criuengine.cpp line 2:
> 1: /*
> 2: * Copyright (c) 2023-2025, Azul Systems, Inc. All rights reserved.
Suggestion:
* Copyright (c) 2023, 2026, Azul Systems, Inc. All rights reserved.
src/java.base/linux/native/libcriuengine/criuengine.cpp line 60:
> 58: "path to a directory with checkpoint/restore files.") \
> 59: OPT(criu_location, const char *, nullptr, CRLIB_OPTION_FLAG_CHECKPOINT | CRLIB_OPTION_FLAG_RESTORE, "path", "no default", \
> 60: "path to this engine library (self).") \
>From how it is used in the code I believe it is a path to a CRIU executable, not this engine library. And it looks safe to say that the default is `../lib/criu`.
src/java.base/linux/native/libcriuengine/criuengine.cpp line 353:
> 351: char *criu = _criu_location;
> 352: if (criu == nullptr) {
> 353: criu = getenv("CRAC_CRIU_PATH");
Now that we have `criu_location` as an alternative for `CRAC_CRIU_PATH` and `args` as an alternative for `CRAC_CRIU_OPTS` maybe we should start deprecating these env variables? E.g. deprecate now (i.e. in 27), obsolete in 28, remove in 29?
src/java.base/linux/native/libcriuengine/criuengine.cpp line 363:
> 361: const char *last_slash = strrchr(fname, '/');
> 362: if (last_slash == nullptr) {
> 363: LOG("Invalid engine library location (missing '/'): %s", fname);
Naming the library leaves less room for interpretation
Suggestion:
LOG("Invalid %s library location (missing '/'): %s", crcommon_log_prefix(), fname);
src/java.base/linux/native/libcriuengine/criuengine.cpp line 369:
> 367: // in the bin folder.
> 368: static constexpr const char criu_suffix[] = "../lib/criu";
> 369: assert(!strcmp("libcriuengine.so", last_slash + 1) || !strcmp("java", last_slash + 1));
Although unexpected, I believe the code would not break if the library file gets renamed, so this assert is redundant
src/java.base/linux/native/libcriuengine/criuengine.cpp line 373:
> 371: _criu_location = static_cast<char*>(malloc(last_slash - fname + 1 + sizeof(criu_suffix)));
> 372: if (_criu_location == nullptr) {
> 373: LOG("Cannot allocate memory for criu location");
For consistency with other logs
Suggestion:
LOG("Cannot allocate memory for CRIU location");
src/java.base/linux/native/libcriuengine/criuengine.cpp line 380:
> 378: return check_criu_executable(_criu_location);
> 379: } else {
> 380: LOG("Failed to find engine library location: %s", dlerror());
Suggestion:
LOG("Failed to find %s library location: %s", crcommon_log_prefix(), dlerror());
src/java.base/linux/native/libcriuengine/criuengine.cpp line 394:
> 392: }
> 393:
> 394: static void print_args_to_stderr(const char **args) {
Maybe make it use `LOG` and rename to `criuengine::log_criu_args`?
src/java.base/linux/native/libcriuengine/criuengine.cpp line 428:
> 426: }
> 427:
> 428: static const char *path_abs(const char *rel) {
I know it's probably just copied from the old code, but is there a reason not to use `realpath` instead?
If there is, I'm a bit worried that this and `join_path` exit on error which is fine when they are called in the forked process, but it is easy to call them in code that runs inside JVM by mistake and start crashing the JVM.
src/java.base/linux/native/libcriuengine/criuengine.cpp line 490:
> 488: LOG("Cannot execute CRIU");
> 489: print_args_to_stderr(argv);
> 490: fprintf(stderr, "\": %s\n", strerror(errno));
If `print_args_to_stderr` will use `LOG` this should as well
src/java.base/linux/native/libcriuengine/criuengine.cpp line 516:
> 514: }
> 515:
> 516: class ArgsAppender {
It would be cleaner if the class owned the argv array being constructed and had two methods: a variadic one for known args and another one with a single string argument that copies and splits the string onto arguments like it does now (it may accept the number of allowed copies in its constructor to pre-allocate an array for copies).
src/java.base/linux/native/libcriuengine/criuengine.cpp line 624:
> 622:
> 623: if (info.si_code != SI_QUEUE) {
> 624: return false;
Suggestion:
return -1;
src/java.base/linux/native/libcriuengine/criuengine.cpp line 674:
> 672: memcpy(criuhelper, fname, dir_length);
> 673: memcpy(criuhelper + dir_length, criuhelper_str, sizeof(criuhelper_str));
> 674: }
This is similar to how we look for CRIU, maybe it can be extracted into a function?
src/java.base/linux/native/libcriuengine/criuhelper.c line 55:
> 53: }
> 54:
> 55: #define MSGPREFIX ""
Maybe make it `"criuengine-helper: "` so that users can understand that the messages come from criuengine's implementation?
src/java.base/linux/native/libcriuengine/criuhelper.c line 75:
> 73: }
> 74:
> 75: static int restorewait(void) {
Do I understand correctly that this is needed because CRIU's executable (which this helper replaces as the restoree's parent) does not handle signals and return values in the same manner?
src/java.base/linux/native/libcriuengine/criuhelper.c line 85:
> 83: sigfillset(&sigact.sa_mask);
> 84: sigact.sa_flags = SA_SIGINFO;
> 85: sigact.sa_sigaction = sighandler;
The handler ignores everything except the signal number so we could've used `sa_handler` instead
-------------
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2846609220
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2846788079
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2846837347
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2846912682
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2847360550
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851833565
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851261410
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2847561208
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851274642
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851292435
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851306261
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851313985
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851315604
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851396643
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851352252
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851400430
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851660836
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851439995
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851571738
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851770480
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851785200
PR Review Comment: https://git.openjdk.org/crac/pull/297#discussion_r2851801443
More information about the crac-dev
mailing list