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
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -245,6 +245,7 @@ class CgroupSubsystem: public CHeapObj<mtInternal> {
virtual int cpu_period() = 0;
virtual int cpu_shares() = 0;
virtual jlong pids_max() = 0;
virtual jlong pids_current() = 0;
virtual jlong memory_usage_in_bytes() = 0;
virtual jlong memory_and_swap_limit_in_bytes() = 0;
virtual jlong memory_soft_limit_in_bytes() = 0;
@@ -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.

GET_CONTAINER_INFO_CPTR(cptr, _pids, "/pids.current",
"Current number of tasks is: %s", "%s %*d", pids_current, 1024);
if (pids_current == NULL) {
return NULL;
}
return os::strdup(pids_current);
}

/* pids_max
*
* Return the maximum number of tasks available to the process
@@ -266,3 +275,17 @@ jlong CgroupV1Subsystem::pids_max() {
char * pidsmax_str = pids_max_val();
return limit_from_str(pidsmax_str);
}

/* pids_current
*
* The number of tasks currently in the cgroup (and its descendants) of the process
*
* return:
* 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.

if (_pids == NULL) return OSCONTAINER_ERROR;
char * pidscurrent_str = pids_current_val();
return limit_from_str(pidscurrent_str);
}
@@ -88,6 +88,7 @@ class CgroupV1Subsystem: public CgroupSubsystem {
int cpu_shares();

jlong pids_max();
jlong pids_current();

const char * container_type() {
return "cgroupv1";
@@ -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.


public:
CgroupV1Subsystem(CgroupV1Controller* cpuset,
@@ -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.

GET_CONTAINER_INFO_CPTR(cptr, _unified, "/pids.current",
"Current number of tasks is: %s", "%s %*d", pids_current, 1024);
if (pids_current == NULL) {
return NULL;
}
return os::strdup(pids_current);
}


/* pids_max
*
* Return the maximum number of tasks available to the process
@@ -249,3 +259,15 @@ jlong CgroupV2Subsystem::pids_max() {
return limit_from_str(pidsmax_str);
}

/* pids_current
*
* The number of tasks currently in the cgroup (and its descendants) of the process
*
* return:
* 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.

char * pidscurrent_str = pids_current_val();
return limit_from_str(pidscurrent_str);
}
@@ -61,6 +61,7 @@ class CgroupV2Subsystem: public CgroupSubsystem {
char *mem_soft_limit_val();
char *cpu_quota_val();
char *pids_max_val();
char *pids_current_val();

public:
CgroupV2Subsystem(CgroupController * unified) {
@@ -80,6 +81,7 @@ class CgroupV2Subsystem: public CgroupSubsystem {
char * cpu_cpuset_cpus();
char * cpu_cpuset_memory_nodes();
jlong pids_max();
jlong pids_current();

const char * container_type() {
return "cgroupv2";
@@ -134,3 +134,8 @@ jlong OSContainer::pids_max() {
assert(cgroup_subsystem != NULL, "cgroup subsystem not available");
return cgroup_subsystem->pids_max();
}

jlong OSContainer::pids_current() {
assert(cgroup_subsystem != NULL, "cgroup subsystem not available");
return cgroup_subsystem->pids_current();
}
@@ -63,6 +63,7 @@ class OSContainer: AllStatic {
static int cpu_shares();

static jlong pids_max();
static jlong pids_current();
};

inline bool OSContainer::is_containerized() {
@@ -2327,6 +2327,16 @@ bool os::Linux::print_container_info(outputStream* st) {
st->print_cr("%s", j == OSCONTAINER_ERROR ? "not supported" : "unlimited");
}

j = OSContainer::OSContainer::pids_current();
st->print("current number of tasks: ");
if (j > 0) {
st->print_cr(JLONG_FORMAT, j);
} else {
if (j == OSCONTAINER_ERROR) {
st->print_cr("not supported");
}
}

return true;
}

@@ -1,5 +1,5 @@
/*
* Copyright (c) 2020, Red Hat Inc.
* Copyright (c) 2020, 2021, Red Hat Inc.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -154,6 +154,11 @@ public long getPidsMax() {
return subsystem.getPidsMax();
}

@Override
public long getPidsCurrent() {
return subsystem.getPidsCurrent();
}

@Override
public long getBlkIOServiceCount() {
return subsystem.getBlkIOServiceCount();
@@ -416,6 +416,11 @@ public long getPidsMax() {
return CgroupSubsystem.limitFromString(pidsMaxStr);
}

public long getPidsCurrent() {
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.

}

/*****************************************************************
* BlKIO Subsystem
****************************************************************/
@@ -311,6 +311,12 @@ public long getPidsMax() {
return CgroupSubsystem.limitFromString(pidsMaxStr);
}

@Override
public long getPidsCurrent() {
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.

}

@Override
public long getBlkIOServiceCount() {
return sumTokensIOStat(CgroupV2Subsystem::lineToRandWIOs);
@@ -365,6 +365,14 @@ public static Metrics systemMetrics() {
*/
public long getPidsMax();

/**
* Returns the current number of tasks in the Isolation Group.
*
* @return The current number of tasks or -2 if not supported
*
*/
public long getPidsCurrent();

/*****************************************************************
* BlKIO Subsystem
****************************************************************/
@@ -408,6 +408,11 @@ public static void printSystemMetrics() {
limit = c.getPidsMax();
ostream.println(formatLimitString(limit, INDENT + "Maximum Processes Limit: ",
longRetvalNotSupported, false));

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.

ostream.println("");
}

@@ -106,7 +106,8 @@ private static void checkContainerInfo(OutputAnalyzer out) throws Exception {
"Memory Usage",
"Maximum Memory Usage",
"memory_max_usage_in_bytes",
"maximum number of tasks"
"maximum number of tasks",
"current number of tasks"
};

for (String s : expectedToContain) {
@@ -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?

break;
}

Asserts.assertEquals(parts.length, 2);
String actual = parts[1].replaceAll("\\s","");
@@ -137,6 +141,8 @@ private static void testPids(String value) throws Exception {
} else {
checkResult(lines, "Maximum number of tasks is: ", value);
}
// current number of tasks value is hard to predict, so better expect no value
checkResult(lines, "Current number of tasks is: ", "no_value_expected");
}

}