RFR: 8331560: Refactor Hotspot container detection code so that subsystem delegates to controllers [v2]

[Severin Gehwolf]

On Wed, 22 May 2024 18:21:50 GMT, Jan Kratochvil <jkratochvil at openjdk.org> wrote:

>> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains five commits:
>> 
>>  - More fix-ups after merge
>>  - Fix limit_from_str usages
>>  - Merge branch 'pr/19060' into jdk-8331560-cgroup-controller-delegation
>>  - Fix whitespace
>>  - Refactor cgroup controller code
>
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 67:
> 
>> 65:     // Note: any index in cg_infos will do as the path is the same for
>> 66:     //       all controllers.
>> 67:     CgroupV2MemoryController* memory = new CgroupV2MemoryController(cg_infos[MEMORY_IDX]._mount_path, cg_infos[MEMORY_IDX]._cgroup_path);
> 
> Suggestion:
> 
>     CgroupV2MemoryController* memory = new CgroupV2MemoryController(cg_infos.memory._mount_path, cg_infos.memory._cgroup_path);
> 
> First could there be:
> class CgroupInfos {
>   CgroupInfo[5];
>   CgroupInfo &cpuset = CgroupInfo[0];
>   CgroupInfo &cpu = CgroupInfo[1];
>   etc.
> };
> and remove:
> #define CG_INFO_LENGTH 5
> #define CPUSET_IDX     0
> #define CPU_IDX        1
> ...
> Second the constructor should have `const CgroupInfo &` as a parameter.

Feel free to propose this cleanup as a separate patch. I'd like to keep this one on-topic to do controller delegation if that's ok.

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 70:
> 
>> 68:     CgroupV2CpuController* cpu = new CgroupV2CpuController(cg_infos[CPU_IDX]._mount_path, cg_infos[CPU_IDX]._cgroup_path);
>> 69:     log_debug(os, container)("Detected cgroups v2 unified hierarchy");
>> 70:     cleanup(cg_infos);
> 
> Could this be a dtor?

It could probably, but I'd like to keep this patch minimal. We could refactor `cg_info` handling in another patch of course.

> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 214:
> 
>> 212:     jlong delta_swap = memory_sw_limit - memory_limit;
>> 213:     if (delta_swap > 0) {
>> 214:       return memory_swap_usage_impl(static_cast<CgroupV1Controller*>(this));
> 
> `memory_swap_usage_impl` could be a method. Likewise for `mem_soft_limit_val`, `mem_swp_limit_val`, `mem_swp_current_val` and `mem_limit_val`.

While true those should intentionally be local to the compilation unit. Adding them as methods clutters the code needlessly, IMO.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1619218922
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1619217636
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1619215983