RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests

[Johan Sjölen]

On Wed, 14 Dec 2022 10:39:04 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> Hi!
>> 
>> Citing the ticket directly:
>> 
>> The function subsystem_file_line_contents does the simple job of parsing lines in a file, but does so in a fairly complex way. This function can be simplified. The function also has a surprising API, as you need to know that in one case the format string needs to take 2 specifiers, instead of 1. 
>> 
>> Some more context:
>> 
>> `subsystem_file_line_contents` either parses files that look like this:
>> 
>> 
>> one_value
>> 
>> 
>> Called as: `subsystem_file_line_contents(ctrl, fname, nullptr, "%s")`
>> 
>> Or like this:
>> 
>> 
>> key1 val1
>> key2 val2
>> 
>> 
>> Called as:  `subsystem_file_line_contents(ctrl, fname, "key1", "%s %s")`
>> 
>> The API for the key/value case is changed to: `subsystem_file_line_contents(ctrl, fname, "key1", "%s")`. Note: `"%s"`, not `"%s %s"`.
>
> src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 139:
> 
>> 137:       char* key_substr = strstr(line, key);
>> 138:       // key should be at beginning of line and next character space
>> 139:       if (key_substr == line && line[key_len] == ' ') {
> 
> This now expects files to be space (` `) separated. I'm not sure we can be sure of it. How about using `isspace(line[key_len]) != 0` instead?

We can't use `isspace`, because it returns true for `\n`. This was probably a bug in the previous implementation, as it'd match `foo\nbar` as `"%s %s"`.

I can only find an actual spec for the interface files for v2, which does state that space (` `) is the expected separator:

https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#interface-files

Even here it is specified as "whenever possible", so I guess tabs might be on the table. We could always do `isspace(line[key_len]) != 0 && line[key_len] != '\n'`?

As an aside: Each of these types should ideally be implemented as separate functions, and not supported by a single monolith. It also seems like we only support 2 of these

> src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 154:
> 
>> 152:     return 0;
>> 153:   }
>> 154:   log_debug(os, container)("Type %s not found in file %s", scan_fmt, absolute_path);
> 
> Previously the log would include more information as the `scan_fmt` now only includes the format of the value to read in. Perhaps we should changes this to the following for better diagnostics:
> 
> 
> log_debug(os, container)("Type %s (key == %s) not found in file %s", scan_fmt, (key == nullptr ?  "null" : key), absolute_path);

Good idea!

> src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 115:
> 
>> 113:   return os::strdup(mems);
>> 114: }
>> 115: 
> 
> This seems a no-op change, please remove from the patch.

This puts `cpu_period` and `cpu_quota_val` next to each other, as they both read from `cpu.max` in opposite order (`"%*s %d"` and `"%1023s %*d"`). I think it improves readability, as it's not obvious why `%*d` is there otherwise. Still remove?

> test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp line 81:
> 
>> 79: }
>> 80: 
>> 81: TEST(CgroupTest, SubSystemFileLineContentsTests) {
> 
> `test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp` uses tag `os_linux_cgroup` and underscore-separated (over camelCase) name. Could we make this consistent? Ideally, all cgroup related gtests should get selected with something like `gtest:os_linux_cgroup` or `gtest:cgroup` or a better name you come up with.

Yeah, snake_case for Gtests is a Hotspot-ism, Gtest's documentation recommends PascalCase. I can always convert all to `cgroupTest`?

Gtest seems to say this:

>. Both names must be valid C++ identifiers, and they should not contain any underscores (_).  

(The  reason for this is that Gtest uses _ for name mangling, and so if we avoid _ then we also avoid accidental collisions)

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

PR: https://git.openjdk.org/jdk/pull/11667