On Thu, 19 May 2022 05:53:19 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> Severin Gehwolf has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Refactor hotspot gtest >> - Separate into function. Fix comment. > > src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 60: > >> 58: strncpy(buf, _mount_point, MAXPATHLEN); >> 59: buf[MAXPATHLEN-1] = '\0'; >> 60: _path = os::strdup(buf); > > Comment on the old code: why this cannot be simply > > > _path = os::strdup(_mount_point); > > > Also, all the strncat calls in this function seem problematic, and should be > converted to stringStream for consistency.
Agreed. I've filed https://bugs.openjdk.java.net/browse/JDK-8287007 to track this. I'd like to limit the changes of this patch to a minimum. It seems orthogonal. > 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/8629
