On Wed, 23 Jun 2021 13:37:59 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
>> }
>
> 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
