On Thu, 19 May 2022 09:06:18 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 63: >> >>> 61: } else { >>> 62: char *p = strstr(cgroup_path, _root); >>> 63: if (p != NULL && p == cgroup_path) { >> >> What happens if cgroup_path is "/foo/bar" and _root is "/fo"? >> >> Maybe this block should be combined with the new `else` block you're adding? >> However, the behavior seems opposite between these two blocks of code: >> >> The upper block: _root is a prefix of cgroup_path, we take the **tail** of >> cgroup_path >> >> >> TestCase substring_match = { >> "/sys/fs/cgroup/memory", // mount_path >> "/user.slice/user-1000.slice", // root_path >> "/user.slice/user-1000.slice/user@1001.service", // cgroup_path >> "/sys/fs/cgroup/memory/user@1001.service" // expected_path >> }; >> >> >> The lower block: The beginning of _root is a prefix of cgroup_path, we take >> the **head** of cgroup_path >> >> >> TestCase prefix_matched_cg = { >> "/sys/fs/cgroup/memory", // mount_path >> "/user.slice/user-1000.slice/session-50.scope", // root_path >> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path >> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path >> }; >> >> >> I find the behavior hard to understand. I think the rules (and reasons) >> should be added to the comment block above the function. > >> What happens if cgroup_path is "/foo/bar" and _root is "/fo"? > > With a mount path of `/bar` this ends up being `/bar/o/bar`. Pretty strange, > but then again it's a bit of a contrived example as those paths come from > `/proc` parsing. Anyway, this is code that got added with > [JDK-8146115](https://bugs.openjdk.java.net/browse/JDK-8146115). It's not > something I've written and to be honest, I'm not sure this branch is needed, > but I didn't want to change the existing behaviour with this patch. I have no > more insight than you in terms of why that approach has been taken. > >> Maybe this block should be combined with the new `else` block you're adding? > > Maybe, but I'm not sure if it would break something. > >> However, the behavior seems opposite between these two blocks of code: >> >> The upper block: _root is a prefix of cgroup_path, we take the **tail** of >> cgroup_path >> >> ``` >> TestCase substring_match = { >> "/sys/fs/cgroup/memory", // mount_path >> "/user.slice/user-1000.slice", // root_path >> "/user.slice/user-1000.slice/user@1001.service", // cgroup_path >> "/sys/fs/cgroup/memory/user@1001.service" // expected_path >> }; >> ``` > > Yes. Though, I cannot comment on why that has been chosen. It's been there > since day one :-/ > >> The lower block: The beginning of _root is a prefix of cgroup_path, we take >> the **head** of cgroup_path >> >> ``` >> TestCase prefix_matched_cg = { >> "/sys/fs/cgroup/memory", // mount_path >> "/user.slice/user-1000.slice/session-50.scope", // root_path >> "/user.slice/user-1000.slice/session-3.scope", // cgroup_path >> "/sys/fs/cgroup/memory/user.slice/user-1000.slice" // expected_path >> }; >> ``` >> >> I find the behavior hard to understand. I think the rules (and reasons) >> should be added to the comment block above the function. > > The reason why I've gone down the path of adding the head of cgroup_path is > because of this document (in conjunction to what the user was observing on an > affected system): > https://www.freedesktop.org/wiki/Software/systemd/ControlGroupInterface/ > > The user was observing paths as listed in the test: > > "/user.slice/user-1000.slice/session-50.scope", // root_path > "/user.slice/user-1000.slice/session-3.scope", // cgroup_path > > This very much looks like systemd managed. Given that and knowing that > systemd adds processes into `scopes` or `services` and groups them via > `slices` and also knowing that cgroups are hierarchical (i.e. limits of > `/foo/bar` also apply to `/foo/bar/baz`), it seems likely that if there are > any limits, then it'll be on `/user.slice/user-1000.slice` within the mounted > controller. Unfortunately, I'm not able to reproduce this myself. I am wondering if the problem is this: We have systemd running on the host, and a different copy of systemd that runs inside the container. - They both set up `/user.slice/user-1000.slice/session-??.scope` within their own file systems - For some reason, when you're looking inside the container, `/proc/self/cgroup` might use a path in the containerized file system whereas `/proc/self/mountinfo` uses a path in the host file system. These two paths may look alike but they have absolutely no relation to each other. I have asked the reporter for more information: https://gist.github.com/gaol/4d96eace8290e6549635fdc0ea41d0b4?permalink_comment_id=4172593#gistcomment-4172593 Meanwhile, I think the current method of finding "which directory under /sys/fs/cgroup/memory controls my memory usage" is broken. As mentioned about, the path you get from `/proc/self/cgroup` and `/proc/self/mountinfo` have no relation to each other, but we use them anyway to get our answer, with many ad-hoc methods that are not documented in the code. Maybe we should do this instead? - Read /proc/self/cgroup - Find the `10:memory:<path>` line - If `/sys/fs/cgroup/memory/<path>/tasks` contains my PID, this is the path - Otherwise, scan all `tasks` files under `/sys/fs/cgroup/memory/`. Exactly one of them contains my PID. For example, here's a test with docker: INSIDE CONTAINER # cat /proc/self/cgroup | grep memory 10:memory:/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050 # cat /proc/self/mountinfo | grep memory 801 791 0:42 /docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050 /sys/fs/cgroup/memory ro,nosuid,nodev,noexec,relatime master:23 - cgroup cgroup rw,memory # cat /sys/fs/cgroup/memory/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050/tasks cat: /sys/fs/cgroup/memory/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050/tasks: No such file or directory # cat /sys/fs/cgroup/memory/tasks | grep $$ 1 ON HOST # cat /sys/fs/cgroup/memory/docker/40ea0ab8eaa0469d8d852b7f1d264b6a451a1c2fe20924cd2de874da5f2e3050/tasks 37494 # cat /proc/37494/status | grep NSpid NSpid: 37494 1 ------------- PR: https://git.openjdk.java.net/jdk/pull/8629