RFR: 8298730: Refactor subsystem_file_line_contents and add docs and tests
Johan Sjölen
jsjolen at openjdk.org
Wed Dec 14 13:02:03 UTC 2022
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
More information about the hotspot-runtime-dev
mailing list