On Fri, 22 Nov 2024 13:00:14 GMT, Sergey Chernyshev <schernys...@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
