RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups

[Severin Gehwolf]

On Thu, 17 Jun 2021 12:27:25 GMT, Matthias Baesken <mbaesken at openjdk.org> 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