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 

> 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):

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

Reply via email to