RFR: 8322420: [Linux] cgroup v2: Limits in parent nested control groups are not detected [v12]

[Severin Gehwolf]

On Thu, 11 Jul 2024 06:54:27 GMT, Jan Kratochvil <jkratochvil at 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