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

[Johan Sjölen]

On Wed, 19 Jun 2024 14:43:26 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this preparatory PR which is an enabler for some bugfix/alignment work (e.g. [JDK-8322420](https://bugs.openjdk.org/browse/JDK-8322420)). The idea is to delegate limit lookup to controllers (`memory`, `cpu`, etc.) from the overarching `CgroupSubsystem` class. This way we can - once the cg type has been detected - "adjust" a controller's path to the limit files when the controller is being created (on init) and then left alone. In this case, the two preparatory entry points are `CgroupUtil::processor_count()` taking a version-agnostic `CgroupCpuController` to do the actual look-up and `CgroupMemoryController::read_memory_limit_in_bytes()` for the same in terms of memory limits.
>> 
>> This enables setting the contoller's path to the interface files (wherever it might be in the hierarchy of it's original cgroup path), look up the limit and "freeze" the path once it found a - lower - limit.
>> 
>> It also ensures that both cgroup versions return `-1` or `-2` (`OSCONTAINER_ERROR`) - in both cases negative - for some notion of unlimited. I.e. it makes the upper bound by the host's physical memory apparent in the version agnostic classes.
>> 
>> Testing:
>> - [x] GHA
>> - [x] gtest:cgroupTest tests
>> - [x] container tests on Linux cgv1 and cgv2 on x86_64
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Rename log function
>  - Review feedback

Hi! Some more review comments.

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 174:

> 172: // compound value we need to sum the two values. Setting a swap limit
> 173: // without also setting a memory limit is not allowed.
> 174: jlong CgroupV2MemoryController::memory_and_swap_limit_in_bytes(julong phys_mem, julong host_swap) {

Potentially note that `host_swap` is unused on purpose in a `/* */` comment next to the argument.

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 195:

> 193: // memory.swap.current : total amount of swap currently used by the cgroup and its descendants
> 194: static
> 195: jlong mem_swp_current_val(CgroupV2Controller* ctrl) {

I think we can have this be called `memory_swap_current_value` :-).

src/hotspot/os/linux/cgroupV2Subsystem_linux.hpp line 42:

> 40: 
> 41:   public:
> 42:     CgroupV2Controller(char * mount_path, char *cgroup_path) :

Style, pre-existing: `char *` -> `char*`.

src/hotspot/os/linux/cgroupV2Subsystem_linux.hpp line 77:

> 75:     CgroupV2Controller* reader() { return &_reader; }
> 76:   public:
> 77:     CgroupV2MemoryController(CgroupV2Controller reader) : _reader(reader) {

Take this as a reference to avoid needless copying.

src/hotspot/os/linux/cgroupV2Subsystem_linux.hpp line 107:

> 105:         _unified(unified),
> 106:         _memory(new CachingCgroupController<CgroupMemoryController>(memory)),
> 107:         _cpu(new CachingCgroupController<CgroupCpuController>(cpu)) {

@jerboaa, Github doesn't let me comment on unchanged lines, so I'm not trying to talk about this line, but right beneath it :-). There are a lot of methods which are still declared which I think have been moved to the `CgroupV2MemoryController`, and so these should be removed, right?

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

PR Review: https://git.openjdk.org/jdk/pull/19085#pullrequestreview-2138127500
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1652520390
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1652514763
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1652483459
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1652431550
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1652482562