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

[Ashutosh Mehra]

On Tue, 1 Apr 2025 14:11:31 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> This is what I had so far (not yet fully tested), but it adds a null check to the non-testing code path...; I can try an approach with a function pointer too if necessary; let me know how to proceed:
>> 
>> 
>> diff --git a/src/hotspot/os/linux/cgroupSubsystem_linux.cpp b/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> index 612cb9a9302..6479853ac04 100644
>> --- a/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> +++ b/src/hotspot/os/linux/cgroupSubsystem_linux.cpp
>> @@ -66,26 +66,10 @@ CgroupSubsystem* CgroupSubsystemFactory::create() {
>>    CgroupV1Controller* pids = nullptr;
>>    CgroupInfo cg_infos[CG_INFO_LENGTH];
>>    u1 cg_type_flags = INVALID_CGROUPS_GENERIC;
>> -  const char* proc_cgroups = "/proc/cgroups";
>> -  const char* sys_fs_cgroup_cgroup_controllers = "/sys/fs/cgroup/cgroup.controllers";
>> -  const char* controllers_file = proc_cgroups;
>>    const char* proc_self_cgroup = "/proc/self/cgroup";
>>    const char* proc_self_mountinfo = "/proc/self/mountinfo";
>> -  const char* sys_fs_cgroup = "/sys/fs/cgroup";
>> -  struct statfs fsstat = {};
>> -  bool cgroups_v2_enabled = false;
>>  
>> -  // Assume cgroups v2 is usable by the JDK iff /sys/fs/cgroup has the cgroup v2
>> -  // file system magic.  If it does not then heuristics are required to determine
>> -  // if cgroups v1 is usable or not.
>> -  if (statfs(sys_fs_cgroup, &fsstat) != -1) {
>> -    cgroups_v2_enabled = (fsstat.f_type == CGROUP2_SUPER_MAGIC);
>> -    if (cgroups_v2_enabled) {
>> -      controllers_file = sys_fs_cgroup_cgroup_controllers;
>> -    }
>> -  }
>> -
>> -  bool valid_cgroup = determine_type(cg_infos, cgroups_v2_enabled, controllers_file, proc_self_cgroup, proc_self_mountinfo, &cg_type_flags);
>> +  bool valid_cgroup = determine_type(cg_infos, true, NULL, proc_self_cgroup, proc_self_mountinfo, &cg_type_flags);
>>  
>>    if (!valid_cgroup) {
>>      // Could not detect cgroup type
>> @@ -249,9 +233,16 @@ static inline bool match_mount_info_line(char* line,
>>                 tmpcgroups) == 5;
>>  }
>>  
>> +/*
>> + * If controllers_file_mock is non-NULL use it as the controllers file
>> + * and respect cgroups_v2_enabled_mock.  This is used by WhiteBox to
>> + * mock the statfs call.  If controllers_file_mock is NULL, ignore
>> + * cgroups_v2_enabled_mock and determine using statfs what to use as
>> + * the controllers file.
>> + */
>>  bool CgroupSubsystemFactory::determine_type(CgroupInfo* cg_infos,
>> -                                            bool cgroups_v2_enabled,
>> -                          ...
>
> This doesn't make the code any better to read, IMO. The previous version seems better considering the non-mock code path isn't tested any more/less.

@jerboaa @fitzsim I agree this doesn't look good.
But I have another suggestion. Given that we now determine if we are in v2 or v1 by using `statfs` before calling `determine_type`, we should IMO rename `determine_type` to be something like `validate_and_populate_cgroup`.
In addition to that, I also feel it would be better if the logic in `determine_type` can be broken down into two functions - one for v1 and another for v2. That would simplify the code and make it more readable as well.
The tests can also be updated to call the function that corresponds to the cgroup version being tested.
And I understand this is quite a bit of refactoring so I don't mind if this is done in a subsequent PR.
What do you think?
Apart from this I don't have any more comments. Rest looks good.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/23811#discussion_r2023394878