RFR: 8349988: Change cgroup version detection logic to not depend on /proc/cgroups [v3]

Mon Mar 3 19:24:03 UTC 2025

On Thu, 27 Feb 2025 14:29:13 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Thomas Fitzsimmons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Replace literal tabs in procCgroupsCgroupsV1CpusetDisabledContent
>
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 42:
> 
>> 40: // Inlined from <linux/magic.h> for portability.
>> 41: #define CGROUP2_SUPER_MAGIC 0x63677270
>> 42: 
> 
> We may want to surround this with:
> 
> 
> #ifndef CGROUP2_SUPER_MAGIC
> ...
> #endif

Done.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 81:
> 
>> 79:   bool cgroups_v2_enabled = false;
>> 80: 
>> 81:   if (statfs(sys_fs_cgroup, &fsstat) != -1) {
> 
> This probably deserves a comment:
> 
> // Assume cgroups v2 iff /sys/fs/cgroup has the cgroup v2 file system magic.

Done.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 263:
> 
>> 261:   char buf[MAXPATHLEN+1];
>> 262:   char *p;
>> 263:   bool is_cgroupsV2 = true;
> 
> For all intents and purposes we can remove `is_cgroupsV2` here and use `cgroups_v2_enabled` instead.

OK, done.  That was what I tried locally with my first attempt, but I backed off because the `testCgroupv1SystemdOnly` and `testCgroupv1NoMounts` cases failed with hierarchy_id-checking assertion failures.  I assumed those tests represented incorrect in-the-wild system configurations that should not hit the debug-mode assertion failures.  Now that you have pointed out that those tests were likely using copy-n-pasted/invalid /proc/self/cgroup data, I am happy to replace `is_cgroupsV2` throughout `determine_type`.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 285:
> 
>> 283:     if ((p = fgets(buf, MAXPATHLEN, controllers)) != nullptr) {
>> 284:       char* controller = nullptr;
>> 285:       char* buf_ptr = buf;
> 
> Suggestion:
> 
>       char* buf_ptr = p;

Fixed.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 287:
> 
>> 285:       char* buf_ptr = buf;
>> 286:       int i;
>> 287:       while ((controller = strsep(&buf_ptr, " \n\t\r\f\v")) != nullptr) {
> 
> Consider defining the separators as `#define IS_SPACE_CHARS " \n\t\r\f\v"` or some such.

Done.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 288:
> 
>> 286:       int i;
>> 287:       while ((controller = strsep(&buf_ptr, " \n\t\r\f\v")) != nullptr) {
>> 288:         // Skip empty string due to line ending in delimiter, '\n'.
> 
> Suggestion:
> 
>         // Skip empty controllers. Be lean about the cgroups.controllers file,
>         // though we probably don't have to be.

I removed the `strcmp(controller, "") == 0` check since empty controllers will be rare and `cg_v2_controller_index` will return `-1` on an empty string anyway.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 299:
> 
>> 297:         } else {
>> 298:           log_debug(os, container)("v2 controller %s is enabled but not relevant", controller);
>> 299:         }
> 
> Do we really need this verbose logging? If you really think we need it, then please bump it to `trace` level. We'd be bailing out anyway if we are missing a required controller with a log.

Agreed, we do not need to log the enabled-but-irrelevant ones, removed.

I think it still makes sense to log JDK-relevant disabled controllers, and optional enabled controllers, at the debug level.  This iteration of the patch fixes the cg v2 branch's logging.  For example, here is `Fedora 41` before and after:

--- tt-old-f41.txt	2025-03-03 09:44:35.919255848 -0500
+++ tt-new-f41.txt	2025-03-03 09:44:36.224257732 -0500
@@ -1,7 +1,6 @@
 [trace][os,container] OSContainer::init: Initializing Container Support
-[debug][os,container] Detected optional pids controller entry in /proc/cgroups
-[debug][os,container] controller cpuset is not enabled
-                    ] 
+[debug][os,container] Detected optional cpuset controller entry in /sys/fs/cgroup/cgroup.controllers
+[debug][os,container] Detected optional pids controller entry in /sys/fs/cgroup/cgroup.controllers
 [debug][os,container] Detected cgroups v2 unified hierarchy
 [trace][os,container] Adjusting controller path for memory: /sys/fs/cgroup/user.slice/user-4215196.slice/user at 4215196.service/app.slice/app-org.gnome.Terminal.slice/vte-spawn-135086d6-2de4-4f2e-ad94-899b5eecaf83.scope
 [trace][os,container] Path to /memory.max is /sys/fs/cgroup/user.slice/user-4215196.slice/user at 4215196.service/app.slice/app-org.gnome.Terminal.slice/vte-spawn-135086d6-2de4-4f2e-ad94-899b5eecaf83.scope/memory.max

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 321:
> 
>> 319:   } else {
>> 320:     /*
>> 321:      * cgroups v2 is not enabled.  Read /proc/cgroups; for cgroups v1 hierarchy (hybrid or
> 
> Suggestion:
> 
>      * The /sys/fs/cgroup filesystem magic hint suggests we have cg v1.  Read /proc/cgroups; for cgroups v1 hierarchy (hybrid or

Done.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 361:
> 
>> 359:       // pids and cpuset controllers are optional. All other controllers are required
>> 360:       if (i != PIDS_IDX && i != CPUSET_IDX) {
>> 361:         is_cgroupsV2 = is_cgroupsV2 && cg_infos[i]._hierarchy_id == 0;
> 
> Fundamentally, we are changing the "hint" as to what constitutes cg v2. So this line needs to be removed. We've already determined at this point that we have cgroup v2 (via the magic check) and we need to use that here. https://github.com/jerboaa/jdk/commit/9958173dc03b66ae96227fb3579bc053cb911f06 would do that and fix a test-consistency issue.

Done.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978036635
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978042082
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978045429
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978039650
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978039029
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978047932
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978053794
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978040638
PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r1978059988
