Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JDK-8273526: Extend the OSContainer API pids controller with pids.current #5437

Closed
wants to merge 4 commits into from

Conversation

MBaesken
Copy link
Member

@MBaesken MBaesken commented Sep 9, 2021

https://bugs.openjdk.java.net/browse/JDK-8266490
extended the OSContainer API in order to also support the pids controller of cgroups. However only pids.max output was added with 8266490.
There is a second parameter pids.current , see https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html#pid
that would be helpful too and can be added to the OSContainer API .
pids.current :
A read-only single value file which exists on all cgroups.
The number of processes currently in the cgroup and its descendants.

Best regards, Matthias


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8273526: Extend the OSContainer API pids controller with pids.current

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5437/head:pull/5437
$ git checkout pull/5437

Update a local copy of the PR:
$ git checkout pull/5437
$ git pull https://git.openjdk.java.net/jdk pull/5437/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5437

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5437.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 9, 2021

👋 Welcome back mbaesken! 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 openjdk bot added the rfr label Sep 9, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 9, 2021

@MBaesken The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-runtime

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

@openjdk openjdk bot added hotspot-runtime core-libs labels Sep 9, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 9, 2021

Webrevs

Copy link
Contributor

@jerboaa jerboaa left a comment

This could get simplified a bit as we don't need to consider max values for pids.current.

* current number of tasks
* OSCONTAINER_ERROR for not supported
*/
jlong CgroupV1Subsystem::pids_current() {
Copy link
Contributor

@jerboaa jerboaa Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pids.current never contains a string max (for unlimited). Therefore, we shouldn't need to do the pids_current_val -> limit_from_str() trick. We should be able to use GET_CONTAINER_INFO(int, [...] akin to cpu_quota. int or long would both be suitable. Up to you to decide which data type to use. I don't think it'll ever be beyond the maximum integrer value.

String pidsCurrentStr = CgroupSubsystemController.getStringValue(pids, "pids.current");
return CgroupSubsystem.limitFromString(pidsCurrentStr);
Copy link
Contributor

@jerboaa jerboaa Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return getLongValue(pids, "pids.current"); should be sufficient here.

@@ -93,6 +93,10 @@ private static void checkResult(List<String> lines, String lineMarker, String ex
String[] parts = line.split(":");
System.out.println("DEBUG: line = " + line);
System.out.println("DEBUG: parts.length = " + parts.length);
if (expectedValue.equals("no_value_expected")) {
System.out.println("No value expected for " + lineMarker);
Copy link
Contributor

@jerboaa jerboaa Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this debug print could say System.out.println("Found '" + lineMarker + "' with value: '" + <value> +"'); and actually parse the number as we'd expect an integer there?

@MBaesken
Copy link
Member Author

@MBaesken MBaesken commented Sep 10, 2021

Hi Severin, I added another commit dealing with your comments.
Thanks, Matthias

Copy link
Contributor

@jerboaa jerboaa left a comment

Sorry for not being clear earlier. I meant that we can do those simplifications throughout.

@@ -106,6 +107,7 @@ class CgroupV1Subsystem: public CgroupSubsystem {
CgroupV1Controller* _pids = NULL;

char * pids_max_val();
char * pids_current_val();
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed.

@@ -252,6 +252,15 @@ char* CgroupV1Subsystem::pids_max_val() {
return os::strdup(pidsmax);
}

char* CgroupV1Subsystem::pids_current_val() {
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function can get removed. It's unused now.

@@ -235,6 +235,16 @@ char* CgroupV2Subsystem::pids_max_val() {
return os::strdup(pidsmax);
}

char* CgroupV2Subsystem::pids_current_val() {
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need that function.

* current number of tasks
* OSCONTAINER_ERROR for not supported
*/
jlong CgroupV2Subsystem::pids_current() {
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use something akin to memory_usage_in_bytes in this class. pids_current_val() isn't needed.

String pidsCurrentStr = CgroupSubsystemController.getStringValue(unified, "pids.current");
return CgroupSubsystem.limitFromString(pidsCurrentStr);
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use: return getLongVal("pids.current"); instead.


long val = c.getPidsCurrent();
ostream.println(formatLimitString(val, INDENT + "Current number of processes: ",
longRetvalNotSupported, false));

Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should add this to -XshowSettings:system output at all. What's reported there is enforced limits. Note that current memory usage isn't shown either.

System.out.println("Found " + lineMarker + " with value: " + ivalue);
try {
int ai = Integer.parseInt(ivalue);
Copy link
Contributor

@jerboaa jerboaa Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move the debug print line to after line 101, please. It could say:

System.out.println("Found " + lineMarker + " with value: " + ai + ". PASS.");

Copy link
Member Author

@MBaesken MBaesken Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the debug println has been moved down.

@MBaesken
Copy link
Member Author

@MBaesken MBaesken commented Sep 14, 2021

Hi Severin, I adjusted my change following your latest comments.

Thanks, Matthias

Copy link
Contributor

@jerboaa jerboaa left a comment

Looks fine. I suggest to move the debug printing from line 99 to after line 101 as suggested here:
#5437 (comment)

Copy link
Member

@iklam iklam left a comment

HotSpot changes look good to me. I have a comment on the test.

@@ -93,6 +93,17 @@ private static void checkResult(List<String> lines, String lineMarker, String ex
String[] parts = line.split(":");
System.out.println("DEBUG: line = " + line);
System.out.println("DEBUG: parts.length = " + parts.length);
if (expectedValue.equals("no_value_expected")) {
Asserts.assertEquals(parts.length, 2);
Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "no_value_expected" generated by Docker? I searched the entire HotSpot source code and couldn't find it. I also couldn't find "WARNING: Your kernel does not support pids limit capabilities".

To make it easier to understand this test, I would suggest grouping all messages that were generated outside of HotSpot into something like:

// These messages are generated by Docker
static final String warning_kernel_no_pids_support = "WARNING: Your kernel does not support pids limit capabilities";
static final String no_value_expected = "no_value_expected";

Copy link
Member

@iklam iklam Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I misunderstood the test. "no_value_expected" was passed in from testPids() in this file, but that's confusing because you are expecting an integer value. Maybe it should be "any_integer"?

Copy link
Contributor

@jerboaa jerboaa Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should be "any_integer"?

+1

Copy link
Member Author

@MBaesken MBaesken Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "no_value_expected" generated by Docker? I searched the entire HotSpot source code and couldn't find it. I also couldn't find "WARNING: Your kernel does not support pids limit capabilities".

Hi, this warning is showing up on some of our Linux ppc64le machines where the pids limit capabilities is not supported.

Best regards, Matthias

Copy link
Member Author

@MBaesken MBaesken Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I adjusted to any_integer, and introduced the "final String warning_kernel_no_pids_support" . I think the message is more related to the kernel features so I did not add the add the 'generated by Docker' comment.

Best regards, Matthias

@openjdk
Copy link

@openjdk openjdk bot commented Sep 14, 2021

@MBaesken 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:

8273526: Extend the OSContainer API pids controller with pids.current

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 85 new commits pushed to the master branch:

  • 7b2beb6: 8273823: Problemlist gc/stringdedup tests timing out on ZGC
  • 8290424: 8272771: frame::pd_ps() is not implemented on any platform
  • a3ca770: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null]
  • 8132bfd: 8273559: Shenandoah: Shenandoah should support multi-threaded heap dump
  • f531b5c: 8273514: java/util/DoubleStreamSums/CompensatedSums.java failure
  • 4c673df: 8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage
  • 8fbcc82: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
  • 92c30c9: 8273599: Remove cross_threshold method usage around GC
  • 02af541: 8273605: VM Exit does not abort concurrent mark
  • febcc72: 8273366: [testbug] javax/swing/UIDefaults/6302464/bug6302464.java fails on macOS12
  • ... and 75 more: https://git.openjdk.java.net/jdk/compare/6eba443428ff7abe7c0509047ec76432affebe65...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.

@openjdk openjdk bot added the ready label Sep 14, 2021
iklam
iklam approved these changes Sep 15, 2021
@MBaesken
Copy link
Member Author

@MBaesken MBaesken commented Sep 16, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

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

  • 74ffe12: 8273575: memory leak in appendBootClassPath(), paths must be deallocated
  • cbffecc: 8273832: gc/shenandoah/TestJcmdHeapDump.java does not have a @requires vm.gc.shenandoah
  • 7b2beb6: 8273823: Problemlist gc/stringdedup tests timing out on ZGC
  • 8290424: 8272771: frame::pd_ps() is not implemented on any platform
  • a3ca770: 8272815: jpackage --type rpm produces an error: Invalid or unsupported type: [null]
  • 8132bfd: 8273559: Shenandoah: Shenandoah should support multi-threaded heap dump
  • f531b5c: 8273514: java/util/DoubleStreamSums/CompensatedSums.java failure
  • 4c673df: 8273656: Improve java.lang.invoke.MethodType.parameterList() and its usage
  • 8fbcc82: 8273494: Zero: Put libjvm.so into "zero" folder, not "server"
  • 92c30c9: 8273599: Remove cross_threshold method usage around GC
  • ... and 77 more: https://git.openjdk.java.net/jdk/compare/6eba443428ff7abe7c0509047ec76432affebe65...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 16, 2021
@openjdk openjdk bot added integrated and removed ready labels Sep 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

@MBaesken Pushed as commit d4546b6.

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

@openjdk openjdk bot removed the rfr label Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs hotspot-runtime integrated
3 participants