On Wed, 23 Jun 2021 13:37:59 GMT, Matthias Baesken <mbaes...@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 >> } > > Matthias Baesken has updated the pull request incrementally with one > additional commit since the last revision: > > Adjustments following Severins comments This looks pretty good now. Looking forward to seeing container tests for this new code. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 559: > 557: return OSCONTAINER_ERROR; > 558: } > 559: // Unlimited memory in Cgroups V2 is the literal string 'max' Please don't add version specific comments to version agnostic code. Suggestion: "Unlimited memory in cgroups is the literal string 'max' for some controllers, for example the pids controller." src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 268: > 266: char * pidsmax_str = pids_max_val(); > 267: jlong pidsmax = limit_from_str(pidsmax_str); > 268: return pidsmax; Do we need this local variable? Consider using `return limit_from_str(pidsmax_str);` instead. src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 250: > 248: char * pidsmax_str = pids_max_val(); > 249: jlong pidsmax = limit_from_str(pidsmax_str); > 250: return pidsmax; Same here. Use `return limit_from_str(pidsmax_str);` ------------- PR: https://git.openjdk.java.net/jdk/pull/4518