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

[Johan Sjölen]

On Fri, 14 Jun 2024 15:54:36 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 with a new target base due to a merge or a rebase. The pull request now contains 36 commits:
> 
>  - Tabs
>  - Templated class use references
>  - initializer lists
>  - Style nit
>  - Get rid of multiple inheritance
>  - Merge branch 'master' into jdk-8331560-cgroup-controller-delegation
>  - Review feedback
>  - Clean up
>  - Appropriately handle version specific printing
>  - Fix inheritance hierarchy
>  - ... and 26 more: https://git.openjdk.org/jdk/compare/cc64aeac...6aff2076

Hi,

Some more review work here. Unfortunately, mostly nits. I'll take a more big-picture look next.

src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 217:

> 215:     virtual jlong rss_usage_in_bytes() = 0;
> 216:     virtual jlong cache_usage_in_bytes() = 0;
> 217:     virtual void print_version_specific_info(outputStream* st, julong host_mem);

`= 0`?

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

> 91: 
> 92: static inline
> 93: void do_trace_log(julong read_mem_limit, julong host_mem) {

Should it be called trace when we log on a debug level?

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

> 51: };
> 52: 
> 53: class CgroupV1MemoryController : public CgroupMemoryController {

Do we expect this class to be subclassed later on/in the future? If not, then we can declare it final as so:

```c++
class CgroupV1MemoryController final : public CgroupMemoryController

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

> 54: 
> 55:   private:
> 56:     CgroupV1Controller* _reader;

It seems simpler to me to have this as a `CgroupV1Controller` and perhaps have a `reader()` accessor that returns a pointer. This class currently has a memory leak as the malloc-allocated `_reader` is never freed in the destructor. Up to you what you want to do.

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

> 142:       _cpuacct(cpuacct),
> 143:       _pids(pids) {
> 144:     }

Pre-existing: No destructor freeing these.

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

PR Review: https://git.openjdk.org/jdk/pull/19085#pullrequestreview-2122534104
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1642599106
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1642589031
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1642596642
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1642593715
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1642602940