8298730: Refactor subsystem_file_line_contents and add docs and tests #11667

jdksjolen · 2022-12-14T10:18:32Z

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".

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8298730: Refactor subsystem_file_line_contents and add docs and tests

Reviewers

Severin Gehwolf (@jerboaa - Reviewer) ⚠️ Review applies to 6488af3b
Ioi Lam (@iklam - Reviewer) ⚠️ Review applies to 5a17527d

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11667/head:pull/11667
$ git checkout pull/11667

Update a local copy of the PR:
$ git checkout pull/11667
$ git pull https://git.openjdk.org/jdk pull/11667/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11667

View PR using the GUI difftool:
$ git pr show -t 11667

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11667.diff

No need for manual string handling

This avoids the callsites having to remember to skip the key in their scan_fmt

Fix key

jdksjolen · 2022-12-14T10:22:02Z

Some preliminary comments (due to @jerboaa):

There are already cgroup tests in test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp. They should be merged. But shouldn't the tests be placed so that they mirror the directory structure where the source code they're testing is?
Tests should be added that correspond to the usages of this function in the real code.

There are a lot of other improvements that can be made, but I'm leaving all of these to a future RFE :-).

bridgekeeper · 2022-12-14T10:33:31Z

👋 Welcome back jsjolen! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-12-14T10:36:33Z

@jdksjolen The following label will be automatically applied to this pull request:

hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-12-14T10:40:39Z

Webrevs

jerboaa

Some initial comments. I'll run my container testing on it and will let you know.

jerboaa · 2022-12-14T10:30:50Z

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

+char * CgroupV2Subsystem::cpu_cpuset_memory_nodes() {
+  GET_CONTAINER_INFO_CPTR(cptr, _unified, "/cpuset.mems",
+                     "cpuset.mems is: %s", "%1023s", mems, 1024);
+  return os::strdup(mems);
+}
+


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?

Yes please, it's got nothing to do with this change. Feel free to move it in separate a RFE.

Alright, reverted.

jerboaa · 2022-12-14T10:39:04Z

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

+    for (; line != nullptr; line = fgets(buf, buf_len, fp)) {
+      char* key_substr = strstr(line, key);
+      // key should be at beginning of line and next character space
+      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

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'?

That seems most appropriate here.

jerboaa · 2022-12-14T10:49:24Z

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

+  if (found_match) {
+    return 0;
+  }
+  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);

jerboaa · 2022-12-14T10:51:39Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

@@ -286,7 +284,7 @@ int CgroupV1Subsystem::cpu_shares() {

 char* CgroupV1Subsystem::pids_max_val() {
  GET_CONTAINER_INFO_CPTR(cptr, _pids, "/pids.max",
-                     "Maximum number of tasks is: %s", "%s %*d", pidsmax, 1024);
+                     "Maximum number of tasks is: %s", "%s", pidsmax, 1024);


To keep the format specifiers in sync, please use %1023s over %s here instead.

jerboaa · 2022-12-14T10:57:15Z

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

+  fclose(fp);
+}
+
+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)

OK with cgroupTest for all.

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

jerboaa · 2022-12-14T14:36:34Z

I'll run my container testing on it and will let you know.

FWIW, container testing looks good on my end for cg v1 and cg v2 with this change.

jdksjolen

I've responded to and fixed the comments.

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

jdksjolen · 2022-12-14T18:04:59Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

@@ -286,7 +284,7 @@ int CgroupV1Subsystem::cpu_shares() {

 char* CgroupV1Subsystem::pids_max_val() {
  GET_CONTAINER_INFO_CPTR(cptr, _pids, "/pids.max",
-                     "Maximum number of tasks is: %s", "%s %*d", pidsmax, 1024);
+                     "Maximum number of tasks is: %s", "%s", pidsmax, 1024);


jdksjolen · 2022-12-14T18:05:11Z

src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp

+char * CgroupV2Subsystem::cpu_cpuset_memory_nodes() {
+  GET_CONTAINER_INFO_CPTR(cptr, _unified, "/cpuset.mems",
+                     "cpuset.mems is: %s", "%1023s", mems, 1024);
+  return os::strdup(mems);
+}
+


Alright, reverted.

jdksjolen · 2022-12-14T19:15:42Z

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

+  fill_file(test_file, "foo\tbar");
+  err = subsystem_file_line_contents(&my_controller, test_file, "foo", "%s", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "bar") << "Incorrect!";


I added your test here, @jerboaa

jdksjolen · 2022-12-14T19:24:52Z

I missed the other two test cases you suggested, and have now added them.

jerboaa

Looks fine, thanks!

jerboaa · 2022-12-15T09:39:15Z

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

+  EXPECT_STREQ(s, "bar") << "Incorrect!";
+
+  s[0] = '\0';
+  fill_file(test_file, "foo\tbar");


Since this is in the multi-line test (SubSystemFileLineContentsMultipleLines), perhaps actually make it multi-line? Something like fill_file(test_file, "foo\tbar\nfoot car");

jerboaa · 2022-12-15T09:41:04Z

test/hotspot/gtest/os/linux/test_cgroupSubsystem_linux.cpp

+  s[0] = '\0';
+  fill_file(test_file, "max 10000");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%s %*d", &s);
+  EXPECT_EQ(err, 0);
+  EXPECT_STREQ(s, "max");
+
+  x = -3;
+  fill_file(test_file, "max 10001");
+  err = subsystem_file_line_contents(&my_controller, test_file, nullptr, "%*s %d", &x);
+  EXPECT_EQ(err, 0);
+  EXPECT_EQ(x, 10001);


These are single-line tests, perhaps move them to SubSystemFileLineContentsSingleLine?

openjdk · 2022-12-15T09:47:57Z

@jdksjolen This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8298730: Refactor subsystem_file_line_contents and add docs and tests

Reviewed-by: sgehwolf, iklam

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 71 new commits pushed to the master branch:

1e99729: 8299274: Add elements to resolved_references consistently
8cc1669: 8299721: [Vector API] assert in switch-default of LibraryCallKit::arch_supports_vector_rotate is too weak to catch bugs
5598acc: 8294403: [REDO] make test should report only on executed tests
88f0ea7: 8299726: [cleanup] Some code cleanup in opto/compile.hpp
0234f81: 8298447: Unnecessary Vector usage in DocPrintJob implementations
cc4936a: 8298720: Insufficient error handling when CodeBuffer is exhausted
b5b7948: 8298240: Replace the usage of ImageLayoutException by the CMMException
99be740: 8299306: Test "javax/swing/JFileChooser/FileSystemView/CustomFSVLinkTest.java" fails on Windows 10 x64 because there are some buttons did not display button name
775da84: 8299412: JNI call of getAccessibleActionCount on a wrong thread
7845b0d: 8296934: Write a test to verify whether Undecorated Frame can be iconified or not
... and 61 more: https://git.openjdk.org/jdk/compare/11fd651ab1820770e3c65cd49589416098987a87...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iklam · 2022-12-15T18:19:06Z

src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp

      GET_CONTAINER_INFO_LINE(julong, _memory->controller(), "/memory.stat", matchline,
-                             "Hierarchical Memory Limit is: " JULONG_FORMAT, format, hier_memlimit)
+                             "Hierarchical Memory Limit is: " JULONG_FORMAT, JULONG_FORMAT, hier_memlimit)


For consistency, I think the "matchline" variable should be renamed to 'key', or just passed as a literal string in the GET_CONTAINER_INFO_LINE call.

iklam · 2022-12-15T18:41:47Z

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

-  int filelen = strlen(file);
-  if ((filelen + strlen(filename)) > (MAXPATHLEN-1)) {
-    log_debug(os, container)("File path too long %s, %s", file, filename);
+  const int buf_len = MAXPATHLEN+1;


This is an existing bug. The contents of a line may theoretically be longer than MAXPATHLEN. It's possible to have a line like this:

somekey /some/file

Or it could just be some arbitrary values that have a very long string.

iklam · 2022-12-15T18:53:27Z

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

+      if (key_substr == line
+          && isspace(after_key) != 0
+          && after_key != '\n') {
+        // Skip key, skip space


When reading a string (scanning for "%s"), I am not sure if cgroup allows empty values for keys. If the input is like this:

char line1[] = "key "; char line2[] = "key";

Then this function will return OSCONTAINER_ERROR for both cases. (This behavior is the same before and after this PR).

This might be OK (if cgroup will never have an empty value), but I think this should be documented to avoid surprises.

Also a test case is needed.

iklam · 2022-12-15T18:58:57Z

src/hotspot/os/linux/cgroupSubsystem_linux.hpp

-  char discard[MAXPATHLEN+1];
-  bool found_match = false;
+  if (c == nullptr) {
+    log_debug(os, container)("subsystem_file_line_contents: CgroupController* is nullptr");


The function name subsystem_file_line_contents is a bit vague. Maybe it should be changed to something like parse_subsystem_file()?

I'd like for a future RFE to split this function into two (with more reasonable names), separating the two parsing cases.

iklam

I think the current changeset is good. The remaining issues are pre-existing and can be fixed in future RFEs.

jdksjolen · 2023-01-06T15:22:09Z

Last test addition added a type issue with regards to signedness (1337 is now cast into a julong). The current changeset passes tier1 testing. I'll avoid integrating this until Monday, however.

jdksjolen · 2023-01-09T10:00:03Z

/integrate

Time to integrate :-).

openjdk · 2023-01-09T10:20:29Z

Going to push as commit 500c3c1.
Since your change was applied there have been 99 commits pushed to the master branch:

4072412: 8298876: Swing applications do not get repainted coming out of sleep on Windows 10
a503ec2: 8299608: Add Register + imm32 orq to x86_64 assembler
d2827ec: 8299671: speed up compiler/intrinsics/string/TestStringLatin1IndexOfChar.java
05a0a71: 8297933: [REDO] Compiler should only use verified interface types for optimization
9b1ade8: 8295406: C1 crash with -XX:TypeProfileArgsLimit=0 -XX:TypeProfileLevel=222
8d17d1e: 6914801: IPv6 unavailable if stdin is a socket
d03a5d9: 8299593: getprotobyname should not be used
c444922: 8218474: JComboBox display issue with GTKLookAndFeel
7607c07: 8299774: SYNTH_BUTTON_UI_KEY field is unused
d53cac3: 8299481: Remove metaprogramming/removePointer.hpp
... and 89 more: https://git.openjdk.org/jdk/compare/11fd651ab1820770e3c65cd49589416098987a87...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-09T10:22:44Z

@jdksjolen Pushed as commit 500c3c1.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

jdksjolen added 14 commits December 9, 2022 12:13

Invert checks so that unexpected ones come first

9bea49f

Put local variables at their usage sites

45abd0b

Check for empty file earlier in the code

a9821d7

Use stringStream for file

f10b1bf

No need for manual string handling

Move buf and discard to usage sites

075874f

Use nullptr, not NULL

d3219c4

Split single line and multi line cases into two

1ed1a17

Simplify file name handling and use buf_len constant

fe42849

Do manual substringing of prefix

050c8e9

This avoids the callsites having to remember to skip the key in their scan_fmt

Check key is at beginning of line, rename file to file_path

1493930

Fix key

Add tests

6d0922c

Change callsites

872f94b

Document code

ed74f24

Freeze stringStream to variable to avoid calling ::base()

ce308c6

openjdk bot added the rfr Pull request is ready for review label Dec 14, 2022

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Dec 14, 2022

jerboaa reviewed Dec 14, 2022

View reviewed changes

jdksjolen added 7 commits December 14, 2022 19:19

More tests, including JULONG and JLONG.

b01d709

Allow other whitespace than space, add test for \t

1819123

Fix log string

2c565ae

Specify buffer limit in format specifier

4dd605c

Revert

2c393bb

Rename tests

3f373be

Split into single-line and multiple-line tests

149646f

jdksjolen commented Dec 14, 2022

View reviewed changes

Add two more test cases

6488af3

jerboaa approved these changes Dec 15, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 15, 2022

iklam suggested changes Dec 15, 2022

View reviewed changes

jdksjolen added 5 commits December 27, 2022 11:53

Move test case to correct spot

0dafedc

Inline literal string

7a40165

Add test

a01fba9

Separate success and error cases

aa3a0ff

Merge remote-tracking branch 'origin/master' into refactor-container

5a17527

iklam approved these changes Jan 3, 2023

View reviewed changes

Fix type signedness issue

0569ed2

openjdk bot added the integrated Pull request has been integrated label Jan 9, 2023

openjdk bot closed this Jan 9, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 9, 2023

8298730: Refactor subsystem_file_line_contents and add docs and tests #11667

8298730: Refactor subsystem_file_line_contents and add docs and tests #11667

Conversation

jdksjolen commented Dec 14, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

jdksjolen commented Dec 14, 2022

bridgekeeper bot commented Dec 14, 2022

openjdk bot commented Dec 14, 2022

mlbridge bot commented Dec 14, 2022 • edited Loading

Webrevs

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerboaa Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerboaa commented Dec 14, 2022

jdksjolen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdksjolen commented Dec 14, 2022

jerboaa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Dec 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

jdksjolen commented Jan 6, 2023

jdksjolen commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

openjdk bot commented Jan 9, 2023

jdksjolen commented Dec 14, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 14, 2022 •

edited

Loading

jerboaa Dec 14, 2022 •

edited

Loading

openjdk bot commented Dec 15, 2022 •

edited

Loading