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

[Thomas Stuefe]

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/ad824a8e...c41d3183

This is a really nice cleanup. Some more remarks inline, but its nearing the finish line from my point of view.

There is some potential for more concise code, (e.g. the many `is_ok` you tend to be using, with subsequent error handling, could mostly be combined to one line), but that is starting to be really nitpicky.

Is the file move of test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp necessary? Difficult to see what changed.

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

> 562:     log_debug(os, container)("read_string: filename is null");
> 563:     return false;
> 564:   }

I think we can just assume and assert that filename != nullptr. Its always a string literal AFAICS.

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

> 632:     log_debug(os, container)("read_numerical_key_value: filename is null");
> 633:     return false;
> 634:   }

Same here, would assert filename != null

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

> 641:     log_debug(os, container)("read_numerical_key_value: key or return pointer is null");
> 642:     return false;
> 643:   }

Same here, should be asserted.

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

> 666:     fclose(fp);
> 667:     return false;
> 668:   }

Do we really explicitly need to handle empty file here? If not, just scratch this section and rely on the block below.

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

> 671:   // File consists of multiple lines in a "key value"
> 672:   // fashion, we have to find the key.
> 673:   const int key_len = (int)strlen(key);

Use size_t. No need for the cast then.

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

> 685:         break;
> 686:       }
> 687:     }

Why use `strstr` for the key? If it has to start at line begin, you could just strncmp:

Proposal:

if (strncmp(line, key, key_len) == 0 && isspace(line[key_len])) {
   const char* value_substr = line + key_len + 1;
...
}

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

> 713:       break;
> 714:     }
> 715:   }

Section could be shortened to just:


const int matched = sscanf(buf, (tup == TupleValue::FIRST) ? "%1023s %*s" : "%*s %1023s", token);


Also consider just making it a bool instead of enum. E.g. `bool use_first`. Expression then is shortened to:


const int matched = sscanf(buf, use_first ? "%1023s %*s" : "%*s %1023s", token);

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

> 140:      * up to the first new line character ('\n') whichever comes first.
> 141:      */
> 142:     bool read_string(const char* filename, char* buf);

Nice comments.

I think the common convention is: either specify both output buffer and output buffer size, or - if you always expect a certain output buffer size - specify argument as `char buf[1024]`. Thinking this through, the former may be a bit cleaner since it allows you to scrap all the literal 1024 inside read_string for the bufsize argument.

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

PR Review: https://git.openjdk.org/jdk/pull/19060#pullrequestreview-2080563720
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615830305
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615843281
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615844605
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615847232
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615858810
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615861707
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615871045
PR Review Comment: https://git.openjdk.org/jdk/pull/19060#discussion_r1615832279