On Tue, 30 Jul 2024 07:47:55 GMT, Jan Kratochvil <jkratoch...@openjdk.org>
wrote:
>> The testcase requires root permissions.
>>
>> Fix by Severin Gehwolf.
>> Testcase by Jan Kratochvil.
>
> Jan Kratochvil has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Inline adjust_controller() twice
> - Revert "Unify 4 copies of adjust_controller()"
>
> This reverts commit 77a81d07d74c8ae9bf34bfd8df9bcaca451ede9a.
Changes seem OK. The test needs some cleanup, though.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 103:
> 101: Asserts.assertEQ(0, exitValue, "Process returned unexpected
> exit code: " + exitValue);
> 102: return output;
> 103: }
This could be simplified to just `ProcessTools.executeProcess(new
ProcessBuilder(args));`
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 120:
> 118: args.add(jdkTool);
> 119: args.add("-cp");
> 120: args.add(System.getProperty("java.class.path"));
Should probably be `test.classes` instead of `java.class.path`.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 145:
> 143: }
> 144: throw e;
> 145: }
This should no longer be needed since we have the `@requires` line.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 193:
> 191: System.err.println(LINE_DELIM + " " + (isCgroup2 ? "cgroup2"
> : "cgroup1") + " mount point: " + sysFsCgroup);
> 192: memory_max_filename = isCgroup2 ? "memory.max" :
> "memory.limit_in_bytes";
> 193: Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER +
> "/" + memory_max_filename), "" + MEMORY_MAX_OUTER);
This still doesn't work on cgv1 (hybrid). The reg-ex pattern matching is wrong.
I'd suggest to simplify this by using `cgget` and `cgset` with forked
processes. Something like this (depending on the version use `memory.max` or
`memory.limit_in_bytes`):
$ cgcreate -g memory:/jdktest128331
$ cgcreate -g memory:/jdktest128331/inner
$ cgset -r memory.limit_in_bytes=512000 /jdktest128331/inner
$ cgget -n -v -r memory.limit_in_bytes /jdktest128331/inner
512000
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 210:
> 208: public Limits hook(List<String> cgexec) throws IOException {
> 209: // CgroupV1Subsystem::read_memory_limit_in_bytes considered
> hierarchical_memory_limit only when inner memory.limit_in_bytes is unlimited.
> 210: Files.writeString(Path.of(sysFsCgroup + "/" + CGROUP_OUTER +
> "/" + CGROUP_INNER + "/" + memory_max_filename), "" + MEMORY_MAX_INNER);
This should be done using `cgset`.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 217:
> 215:
> 216: // KFAIL - verify the
> CgroupSubsystem::initialize_hierarchy() and
> jdk.internal.platform.CgroupSubsystem.initializeHierarchy() bug
> 217: // TestTwoLimits does not see the lower MEMORY_MAX_OUTER
> limit.
Remove this obsolete(?) comment.
test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 247:
> 245: self_verbose.add("-version");
> 246: pSystem(self_verbose);
> 247: }
Instead of disabling the memory controller that way, consider creating only the
cpu controller (using `cgcreate`) and test for memory limits?
test/jtreg-ext/requires/VMProps.java line 141:
> 139: map.put("vm.flagless", this::isFlagless);
> 140: map.put("jdk.foreign.linker", this::jdkForeignLinker);
> 141: map.put("vm.cgroup.tools", this::cgroupTools);
Why the `vm` prefix? Could be just `cgroup.tools` no? This isn't a JVM property.
-------------
PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2233709616
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714213497
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714218948
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714219641
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714181908
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221143
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714221815
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714226775
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1714228598
