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

[Severin Gehwolf]

On Tue, 1 Apr 2025 11:34:50 GMT, Thomas Fitzsimmons <duke at openjdk.org> wrote:

>> If we really must, I'd rather have a function pointer for the statfs call which we can replace in test code. It doesn't seem worth the extra complexity in my opinion though.
>
> 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,
> -                                            const char* controllers_file,
> +                                            bool ...

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.

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

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