RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v3]

[Severin Gehwolf]

On Thu, 19 May 2022 05:53:19 GMT, Ioi Lam <iklam at 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 at 1001.service",   // cgroup_path
>     "/sys/fs/cgroup/memory/user at 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