RFR: 8334254: Cleanup CGroups initialization code [v2]

[Thomas Stuefe]

On Sun, 23 Jun 2024 08:46:20 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> Please consider this patch which attempts to improve the code quality of some of the cgroups code, somewhat.
>> 
>> 1. Get rid of `#define`s and upgrade to a `CG_INFO` enum, with some helpers
>> 2. Make some of the code prettier by removing copy-pasted and edited code
>> 3. Make `CgroupInfo` RAII, ridding us of the `cleanup` function
>> 4. `set_subsystem_path` is **only** used at construction time, only call it at that time and make those functions private. Also remove `virtual`ness, as this is unnecessary now.
>> 
>> Testing:
>> 
>> 1. cgroupTest group in gtest.
>> 
>> Thank you.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Review comments

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 92:

> 90:    */
> 91:   assert(is_cgroup_v1(&cg_type_flags), "Cgroup v1 expected");
> 92:   // The following are required to exist, determine_type will fail and return false otherwise.

I don't fully follow here. This is changed behavior compared to before?

src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 111:

> 109:   } else {
> 110:     log_debug(os, container)("CgroupInfo for %s not complete", cg_controller_name[PIDS_IDX]);
> 111:   }

?? I don't get it. If data_complete is false, we just leak these objects?

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 81:

> 79: }
> 80: 
> 81: void CgroupV1MemoryController::set_subsystem_path(char *cgroup_path) {

preexisting: This should be const char* (I was wondering if this modifies the memory handed in; it doesn't, but a non-const address really looks like an output parameter)

src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 44:

> 42: 
> 43:   public:
> 44:     void set_subsystem_path(char *cgroup_path);

const char*

src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 46:

> 44:     void set_subsystem_path(char *cgroup_path);
> 45: 
> 46:     CgroupV1Controller(char* root, char* mountpoint, char* cgroup_path)

pre-existing. All of these parameters should be const char*.

src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 52:

> 50:         set_subsystem_path(cgroup_path);
> 51:       }
> 52:       char *subsystem_path() { return _path; }

const char* , method const

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658548432
PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658554662
PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658529699
PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658534554
PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658531399
PR Review Comment: https://git.openjdk.org/jdk/pull/19709#discussion_r1658533663