On Thu, 18 Jul 2024 14:48:02 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 one additional 
> commit since the last revision:
> 
>   Unify 4 copies of adjust_controller()

This patch seems OK (though, I'm biased). Please clean up the test.

src/hotspot/os/linux/cgroupUtil_linux.cpp line 64:

> 62:     return cpu->adjust_controller(cpu_total);
> 63:   }
> 64:   return cpu;

I guess an alternative - and maybe more readable solution - would be to inline 
`cpu->adjust_controller()` and `mem->adjust_controller()` code here. We have cg 
version agnostic api to query the limits. We'd just need accessors for 
`cgroup_path()` and a setter, `set_cgroup_path()` in 
`CgroupCpuController/CgroupMemoryController` impls.

test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 28:

> 26:  * @key cgroups
> 27:  * @requires os.family == "linux"
> 28:  * @requires vm.flagless

If you really want to keep that test, then we should add support for the 
`libcg` dependency in `jtreg-ext` lib so that we can write `requires os.family 
== "linux" & dep.libcgroup` or some such.

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

PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2191041991
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1686225373
PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1686230300

Reply via email to