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

[Johan Sjölen]

On Thu, 23 May 2024 14:00:30 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 16 additional commits since the last revision:
> 
>  - Add proper comments for parsing utility functions
>  - Add a test for a large string read
>  - Add convenience function for 'max' handling
>    
>    This now makes limit_from_str a private method
>  - Fix general read string cases.
>  - Get rid of the templated function
>  - Use enum class for TupleValue
>  - Merge branch 'master' into jdk-8302744-cleanup-getcontainer-info
>  - 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
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/60138569...c41d3183

Just a couple of style comments from me. I'll give it another round of looking over soon.

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

> 704:   char token[1024];
> 705:   int matched = -1;
> 706:   switch(tup) {

Style: Add a space between switch and `(tup)`.

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

> 109:  * tuple value to a constant string format for sscanf reading.
> 110:  */
> 111: enum class TupleValue { FIRST, SECOND };

Style, nit: No reason for all-caps for these names, can just be "First", "Second".

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

> 150:   bool is_ok = _memory->controller()->
> 151:                     read_numerical_key_value("/memory.stat", "anon", &rss);
> 152:   if (!is_ok) {

Style, nit: A bit unconventional (for Hotspot) to line break after `->`, maybe this is preferable:

```c++
bool is_ok =
  _memory->controller()->read_numerical_key_value("/memory.stat", "anon", &rss);

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

PR Review: https://git.openjdk.org/jdk/pull/19060#pullrequestreview-2080850445
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616007442
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616008257
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1616011912