On Thu, 17 Jun 2021 12:27:25 GMT, Matthias Baesken <[email protected]> wrote:
> Hello, please review this PR; it extend the OSContainer API in order to also
> support the pids controller of cgroups.
>
> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct",
> "memory" on some older Linux distros (SLES 12.1, RHEL 7.1) the pids
> controller might not be there (or not fully supported) so it was added as
> optional , see the coding
>
>
> if (!cg_infos[PIDS_IDX]._data_complete) {
> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
> // keep the other controller info, pids is optional
> }
Thanks for this work. How did you test this? Did you run container tests on a
cgroups v1 and cgroups v2 system?
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 136:
> 134: char *p;
> 135: bool is_cgroupsV2;
> 136: // true iff all required controllers, memory, cpu, cpuset, cpuacct
> enabled
*are* enabled, please.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 193:
> 191: all_required_controllers_enabled = true;
> 192: for (int i = 0; i < CG_INFO_LENGTH; i++) {
> 193: // the pids controller is not there on older Linux distros
Suggestion: Change the code comment to `// pids controller is optional. All
other controllers are required`
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 198:
> 196: all_required_controllers_enabled =
> all_required_controllers_enabled && cg_infos[i]._enabled;
> 197: }
> 198: if (! cg_infos[i]._enabled) {
This if is only present for debug logging and should be guarded to that effect.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 424:
> 422: return false;
> 423: }
> 424: if (!cg_infos[PIDS_IDX]._data_complete) {
Same here, this if should be guarded with debug logging being enabled.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 252:
> 250: * maximum number of tasks
> 251: * -1 for no setup
> 252: * -3 for "max" (special value)
I'd suggest to use:
-1 if unlimited
OSCONTAINER_ERROR for not supported
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 261:
> 259: "Maximum number of tasks is: " JLONG_FORMAT,
> JLONG_FORMAT, pidsmax);
> 260: if (pidsmax < 0) {
> 261: // check for potential special value
It would be clearer if this comment mentioned that the value might be `max`
and, thus, wouldn't be parseable with `GET_CONTAINER_INFO`.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 266:
> 264: err2 = subsystem_file_line_contents(_pids, "/pids.max", NULL,
> "%1023s", myline);
> 265: if (err2 != 0) {
> 266: if (strncmp(myline, "max", 3) == 0) return -3;
We use `-1` for "unlimited" elsewhere and should probably do the same here.
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 266:
> 264: err2 = subsystem_file_line_contents(_pids, "/pids.max", NULL,
> "%1023s", myline);
> 265: if (err2 != 0) {
> 266: if (strncmp(myline, "max", 3) == 0) return -3;
This looks like it should use `GET_CONTAINER_INFO_CPTR` macro and then
`limit_from_str` from cgroups v2 code. Perhaps move `limit_from_str` method to
the base class.
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 260:
> 258: }
> 259: }
> 260: return pidsmax;
We have this pattern of needing to handle `max` elsewhere in cgroups v2 code.
See for example: `CgroupV2Subsystem::cpu_quota()`. We should handle it
similarly here.
src/hotspot/os/linux/os_linux.cpp line 2319:
> 2317: st->print_cr("max");
> 2318: } else {
> 2319: st->print_cr("%s", j == OSCONTAINER_ERROR ? "not supported" :
> "unlimited");
We should treat the unlimited case similar to how we handle them elsewhere. I'm
not sure this magic constant of `-3` gives us any more info that we'd get with
`-1` that we use elsewhere.
src/java.base/linux/classes/jdk/internal/platform/cgroupv1/CgroupV1Subsystem.java
line 415:
> 413: ****************************************************************/
> 414: public long getPidsMax() {
> 415: return
> CgroupV1SubsystemController.longValOrUnlimited(getLongValue(pids,
> "pids.max"));
Since this value may be `max` we should use the same logic than for v2. I.e.:
String pidsMaxStr = CgroupSubsystemController.getStringValue(pids, "pids.max");
return CgroupSubsystemController.limitFromString(pidsMaxStr);
test/hotspot/jtreg/containers/cgroup/CgroupSubsystemFactory.java line 172:
> 170: "net_prio 5 1 1\n" +
> 171: "hugetlb 6 1 1\n" +
> 172: "pids 9 80 1"; // the 3 did not match 9
This comment leaves the reader none the wiser. I think you are alluding to
controller id matching between `/proc/cgroups` and `/proc/self/cgroup`. If so,
please use that info.
-------------
Changes requested by sgehwolf (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/4518
