RFR: JDK-8293472: Potentially incorrect container resource limit detection if manual cgroup fs mounts present [v6]

[王超]

On Fri, 9 Sep 2022 09:43:42 GMT, Severin Gehwolf <sgehwolf at openjdk.org> wrote:

>> 王超 has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fix for cgroup v2
>
> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 126:
> 
>> 124: }
>> 125: 
>> 126: void CgroupSubsystemFactory::set_controller_pathes(CgroupInfo* cg_infos,
> 
> Typo: `set_controller_paths`

done

> src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 346:
> 
>> 344:                                    cg_infos[0]._mount_path, tmp_mount_point);
>> 345:         }
>> 346:       }
> 
> Couldn't we simplify this? For example by changing:
> 
> 
>       if (!cgroupv2_mount_point_found && strcmp("cgroup2", tmp_fs_type) == 0) {
>         cgroupv2_mount_point_found = true;
>         any_cgroup_mounts_found = true;
>         for (int i = 0; i < CG_INFO_LENGTH; i++) {
>           assert(cg_infos[i]._mount_path == NULL, "_mount_path memory stomping");
>           cg_infos[i]._mount_path = os::strdup(tmp_mount_point);
>         }
> 
> 
> to something like this:
> 
> 
>       if (strcmp("cgroup2", tmp_fs_type) == 0) {
>         cgroupv2_mount_point_found = true;
>         any_cgroup_mounts_found = true;
>         for (int i = 0; i < CG_INFO_LENGTH; i++) {
>           set_controller_paths(cg_infos, i, "(cg2, unified)", tmpmount, tmproot);
>         }
> 
> 
> It would entail modifying the `sscanf` to something like `sscanf(p, "%*d %*d %*d:%*d %s %s %*[^-]- %s %*s %*s", tmproot, tmpmount, tmp_fs_type) == 3`. We could also get rid of duplicate local var `tmp_mount_point`.

done

> test/hotspot/jtreg/containers/docker/TestCPUAwareness.java line 80:
> 
>> 78:             testCpuQuotaAndPeriod(100*1000, 100*1000, true);
>> 79:             testCpuQuotaAndPeriod(150*1000, 100*1000, true);
>> 80:             testCpuQuotaAndPeriod(400*1000, 100*1000, true);
> 
> It should be sufficient to test the `true` case once. I.e. one test `testCpuQuotaAndPeriod(50*1000, 100*1000, true);` should be enough (as this would extend container test runs needlessly).

done

> test/hotspot/jtreg/containers/docker/TestMemoryAwareness.java line 73:
> 
>> 71:             testMemoryLimit("500m", "524288000", true);
>> 72:             testMemoryLimit("1g", "1073741824", true);
>> 73:             testMemoryLimit("4g", "4294967296", true);
> 
> One test with the additional cgroup mounts should be sufficient. I suggest:
> 
> 
> testMemoryLimit("500m", "524288000", true /* additional cgroup mount */);

done

-------------

PR: https://git.openjdk.org/jdk/pull/10193