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

[Johan Sjölen]

On Wed, 29 May 2024 17:42:13 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 30 commits:
> 
>  - Review feedback
>  - Clean up
>  - Appropriately handle version specific printing
>  - Fix inheritance hierarchy
>  - Fixup after merge
>  - Merge branch 'master' into jdk-8331560-cgroup-controller-delegation
>  - Resolve ambiguity with static_cast
>  - Remove limit_from_str from util class
>  - Merge branch 'jdk-8302744-cleanup-getcontainer-info' into jdk-8331560-cgroup-controller-delegation
>  - Add proper comments for parsing utility functions
>  - ... and 20 more: https://git.openjdk.org/jdk/compare/9b64ece5...e177a086

OK, I'm getting a handle of this change. Some smaller review comments. I'll give it another round soon.

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

> 183: class CachingCgroupController : public CHeapObj<mtInternal> {
> 184:   private:
> 185:     T _controller;

We always expect `T` to be a `T*`, so make it so.

Change:

```c++
T* _controller;


and remove the `*` from all instantiations.

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

> 187: 
> 188:   public:
> 189:     CachingCgroupController(T cont) {

`T*`

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

> 193: 
> 194:     CachedMetric* metrics_cache() { return _metrics_cache; }
> 195:     T controller() { return _controller; }

`T*`

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

> 51: class CgroupV2CpuController: public CgroupV2Controller, public CgroupCpuController {
> 52:   public:
> 53:     CgroupV2CpuController(char * mount_path, char *cgroup_path) : CgroupV2Controller(mount_path, cgroup_path) {

Style: `char*`

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

> 60: class CgroupV2MemoryController: public CgroupV2Controller, public CgroupMemoryController {
> 61:   public:
> 62:     CgroupV2MemoryController(char * mount_path, char *cgroup_path) : CgroupV2Controller(mount_path, cgroup_path) {

Style: `char*`

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

> 87:       _unified = memory; // Use memory for now, should have all separate later
> 88:       _memory = new CachingCgroupController<CgroupMemoryController*>(memory);
> 89:       _cpu = new CachingCgroupController<CgroupCpuController*>(cpu);

Style: hug the asterisk to the name

Pre-existing: Please initialize the members in intiializer list.

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

PR Review: https://git.openjdk.org/jdk/pull/19085#pullrequestreview-2116810835
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638818700
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638822572
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638822309
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638815605
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638815887
PR Review Comment: https://git.openjdk.org/jdk/pull/19085#discussion_r1638816991