On Thu, 11 Jul 2024 06:54:27 GMT, Jan Kratochvil <jkratoch...@openjdk.org> wrote:
>> The testcase requires root permissions. >> >> Designed by Severin Gehwolf, implemented by Jan Kratochvil. > > Jan Kratochvil has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 103 commits: > > - Fix the gtest > - fix compilation warning > - fix the gtest > - less refactorizations > - remove not a real backward compat. > - whitespace > - less refactorizations > - reduce refactorizations > - Fix caching > - Merge branch 'master' into master-cgroup > - ... and 93 more: https://git.openjdk.org/jdk/compare/537d20af...060e7688 Conceptually, I think it would be cleaner if we did the "trim" action at construction time of `Subsystem` on a per controller basis. The path is a property of the controller. It would be best do do it before caching is set up. A couple of other comments: - We should fix Hotspot first (this bug) and do the Metrics (java) change in a separate patch - As soon as we have found a lower limit we can stop. Hierarchies which have a higher limit than the parent are ill-formed, and we can ignore it. - We ought to also trim the path for the CPU controller. This patch only fixes the memory controller. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 836: > 834: > 835: void CgroupController::set_path(const char *cgroup_path) { > 836: __attribute__((unused)) bool _cgroup_path; // Do not use the member > variable. This seems an unusual pattern for Hotspot. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 910: > 908: memory_swap_limit_min = memory_swap_limit; > 909: best_level = dir_count; > 910: } There is no point in doing memory and memory and swap. Both are controlled by the same controller. So there is no chance that the paths would differ. src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 237: > 235: _metrics_cache = new CachedMetric(); > 236: return controller()->trim_path(dir_count); > 237: } We should get away with only doing it for the actual underlying controllers. I.e. we should do it before caching is set up. src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 78: > 76: return (jlong)hier_memlimit; > 77: } > 78: log_trace(os, container)("Hierarchical Memory Limit is: Unlimited"); It is the hope to no longer needing to do this hierarchical look-up since we know where in the hierarchy we ought to look for the memory limit. src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 116: > 114: log_trace(os, container)("Hierarchical Memory and Swap Limit is: > Unlimited"); > 115: } else { > 116: return (jlong)hier_memswlimit; Same here. Hierarchical look-ups shouldn't be needed if we do this correctly. src/hotspot/os/linux/cgroupV1Subsystem_linux.hpp line 36: > 34: class CgroupV1Controller: public CgroupController { > 35: public: > 36: using CgroupController::CgroupController; Inheriting constructors are not permitted as per the Hotspot style-guide: https://github.com/openjdk/jdk/blob/master/doc/hotspot-style.md#inheriting-constructors test/hotspot/gtest/runtime/test_cgroupSubsystem_linux.cpp line 1: > 1: /* Why are those test changes needed? test/hotspot/jtreg/containers/cgroup/NestedCgroup.java line 1: > 1: /* I think a more generic solution would be to use https://bugs.openjdk.org/browse/JDK-8333446 for testing (requires only systemd vs. this requiring libcg). A hierarchy setup of two limits should be possible with it too, though I'm doubtful that's needed. ------------- PR Review: https://git.openjdk.org/jdk/pull/17198#pullrequestreview-2171534036 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673955179 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673958522 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673952238 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673797261 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673798080 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673795471 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673800271 PR Review Comment: https://git.openjdk.org/jdk/pull/17198#discussion_r1673806004
