RFR: 8302744: Refactor Hotspot container detection code [v2]

[Johan Sjölen]

On Tue, 21 May 2024 13:58:22 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Please review this container detection code refactoring in hotspot. The main point of this is to
>> 
>> - get rid of the `GET_CONTAINER_INFO` macros which hide too many things under the hood
>> - prevent refactoring of the code (since `GET_CONTAINER_INFO` macros short-return and are therefore not portable; at least not without some risk)
>> - make the code easier to understand
>> - allow for better testing via `gtest`
>> - separate multi-line parsing from single line parsing for clarity.
>> 
>> Testing:
>> - [x] GHA
>> - [x] `gtest:cgroupTest` tests
>> - [x] Container tests on Linux with cgroup v1 (legacy) and cgroup v2. All pass.
>> 
>> Thoughts?
>
> Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision:
> 
>  - Fix TestMemoryAwareness for cgroup v2
>    
>    It only reads the swap value, thus doesn't print:
>    "Memory and Swap Limit is" but rather prints:
>    "Swap Limit is". This is fine, since for cg v2
>    the memory limit and the swap limits are in different
>    files.
>  - Fix whitespace
>  - Handle cpu quota for cgroups v1 specially
>  - Try a different approach.
>    
>    Hybrid MACRO usage and locked down reading primitives.
>  - Add key-value read function
>  - Add basic testing for read functions
>  - Add generic julong/char* read functions
>  - Merge branch 'master' into jdk-8302744-cleanup-getcontainer-info
>  - 8302744: Refactor Hotspot container detection code

This looks pretty good. Just the right amount of macrology, to me. I'll do another sweep, including the tests, later on.

Thank you for working on this.

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

> 657: PRAGMA_FORMAT_NONLITERAL_IGNORED // Only string/number literal formats used. See read_*() functions.
> 658: template <typename T>
> 659: bool CgroupController::read_from_file(const char* filename, const char* scan_fmt, T result) {

`read_value_from_ file`?

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

> 93: 
> 94: 
> 95: enum TupleValue { FIRST, SECOND };

Suggestion/nit: Enum class + `First`, `Second`. Your choice to keep it as is, however.

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

PR Review: https://git.openjdk.org/jdk/pull/19060#pullrequestreview-2069046452
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1608585282
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1608587282