RFR: 8343191: Cgroup v1 subsystem fails to set subsystem path [v3]

[Severin Gehwolf]

On Fri, 22 Nov 2024 13:00:14 GMT, Sergey Chernyshev <schernyshev at openjdk.org> wrote:

>>> Here, `limit` at line 64 is not stored as a possible lowest limit, so if the inner group has lower limit than the outer group, it won't be detected (cg v2 is affected too).
>> 
>> Good spot! How about this to fix it?
>> 
>> 
>> jlong limit = mem->read_memory_limit_in_bytes(phys_mem); 
>> jlong lowest_limit = limit < 0 ? phys_mem: limit;
>> 
>> 
>>> The cgroup path will be adjusted to the outer group (when it's limited). Another issue is in the loop at line 66 that reduces the path. In cg v1 in `cgroupns=host` mode (default) the cgroup_path includes the base cgroup and the subgroup (when moved). It may be not quite obvious which part of the path is the actual subgroup (and the CloudFoundry case has demonstrated it). It is suggested to determine the actual subgroup (path suffix) before reducing the group path. Please see the updated patch.
>> 
>> I'm worried about the added complexity. 1.) Is this something that's needed in cg v2 too? If no, we are changing cg version agnostic code for a version specific issue. 2.) Wouldn't setting the cgroup path to `<mount_point>/<cgroup_path>` achieve the same thing, when it's currently `null` (in current master)?
>> 
>> After all, those are corner cases which don't seem very common.
>
>> Good spot! How about this to fix it?
>> 
>> ```
>> jlong limit = mem->read_memory_limit_in_bytes(phys_mem); 
>> jlong lowest_limit = limit < 0 ? phys_mem: limit;
>> ```
> 
> Make sense to me.
> 
>> I'm worried about the added complexity. 1.) Is this something that's needed in cg v2 too? If no, we are changing cg version agnostic code for a version specific issue. 2.) Wouldn't setting the cgroup path to `<mount_point>/<cgroup_path>` achieve the same thing, when it's currently `null` (in current master)?
> 
> The added complexity only kicks in in cg v1 when `_root != cgroup_path`, so exactly in that corner case. In cg v2 it only works with `--cgroupns=host`, it ends up calling stat() once and then continues normally, so the added overhead is minimal.

The added code in `CgroupUtil::adjust_controller` runs for cg v1 and cg v2 when path adjustment is deemed needed. So I'm not clear why it's needed for cg v2. I thought we have established that for cg v2 && `--cgroupns=host` the current path adjustment is sufficient? What am I missing?

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21808#discussion_r1854277359