Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 19 May 2022 07:02:13 GMT, Thomas Stuefe wrote: >> I'm not convinced the extra function makes the code more readable, but here >> it is. I can revert back if this is too much. > > @jerboaa Feel free to go back to your original variant. This was only a > proposal. understood. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Wed, 18 May 2022 18:10:45 GMT, Severin Gehwolf wrote: >> @iklam yes I meant `Find the longest common prefix`. Fixed the comment. > > I'm not convinced the extra function makes the code more readable, but here > it is. I can revert back if this is too much. @jerboaa Feel free to go back to your original variant. This was only a proposal. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Tue, 17 May 2022 06:18:25 GMT, Ioi Lam wrote: >> Severin Gehwolf has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Use stringStream to simplify controller path assembly > > src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92: > >> 90: } >> 91: ss.print_raw(_root, last_matching_slash_pos); >> 92: _path = os::strdup(ss.base()); > > Do you mean `Find the longest common prefix`? Maybe give an example in the > comments? Text parsing in C code is really difficult to understand. @iklam yes I meant `Find the longest common prefix`. Fixed the comment. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Wed, 18 May 2022 18:09:54 GMT, Severin Gehwolf wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92:
>>
>>> 90: }
>>> 91: ss.print_raw(_root, last_matching_slash_pos);
>>> 92: _path = os::strdup(ss.base());
>>
>> Do you mean `Find the longest common prefix`? Maybe give an example in the
>> comments? Text parsing in C code is really difficult to understand.
>
> @iklam yes I meant `Find the longest common prefix`. Fixed the comment.
I'm not convinced the extra function makes the code more readable, but here it
is. I can revert back if this is too much.
>> test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp line 63:
>>
>>> 61: ASSERT_STREQ(expected_cg_paths[i], ctrl->subsystem_path());
>>> 62: }
>>> 63: }
>>
>> I found it hard to relate the different paths. Could you create a new struct
>> like this?
>>
>>
>> struct TestCase {
>> char* mount_path;
>> char* root_paths;
>> char* cgroup_path;
>> char* expected_cg_paths;
>> } = {
>> { "/sys/fs/cgroup/memory", // mount
>>"/", // root,
>>
>
> Yes, makes sense. Will do.
Done now.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Tue, 17 May 2022 07:14:34 GMT, Thomas Stuefe wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92:
>>
>>> 90: }
>>> 91: ss.print_raw(_root, last_matching_slash_pos);
>>> 92: _path = os::strdup(ss.base());
>>
>> Do you mean `Find the longest common prefix`? Maybe give an example in the
>> comments? Text parsing in C code is really difficult to understand.
>
> Maybe factor out the search like this
>
> // Return length of common starting substring. E.g. "cat" for ("cattle",
> "catnip");
> static int find_common_starting_substring(const char* s1, const char* s2) {
> ...
> }
>
> That way its clearer and we can find and move this to utilities if we ever
> need this in other places.
OK.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Tue, 17 May 2022 05:55:47 GMT, Ioi Lam wrote:
>> Severin Gehwolf has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use stringStream to simplify controller path assembly
>
> test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp line 63:
>
>> 61: ASSERT_STREQ(expected_cg_paths[i], ctrl->subsystem_path());
>> 62: }
>> 63: }
>
> I found it hard to relate the different paths. Could you create a new struct
> like this?
>
>
> struct TestCase {
> char* mount_path;
> char* root_paths;
> char* cgroup_path;
> char* expected_cg_paths;
> } = {
> { "/sys/fs/cgroup/memory", // mount
>"/", // root,
>
Yes, makes sense. Will do.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Tue, 17 May 2022 06:18:25 GMT, Ioi Lam wrote:
>> Severin Gehwolf has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use stringStream to simplify controller path assembly
>
> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92:
>
>> 90: }
>> 91: ss.print_raw(_root, last_matching_slash_pos);
>> 92: _path = os::strdup(ss.base());
>
> Do you mean `Find the longest common prefix`? Maybe give an example in the
> comments? Text parsing in C code is really difficult to understand.
Maybe factor out the search like this
// Return length of common starting substring. E.g. "cat" for ("cattle",
"catnip");
static int find_common_starting_substring(const char* s1, const char* s2) {
...
}
That way its clearer and we can find and move this to utilities if we ever need
this in other places.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 18:09:40 GMT, Severin Gehwolf wrote: >> Please review this change to the cgroup v1 subsystem which makes it more >> resilient on some of the stranger systems. Unfortunately, I wasn't able to >> re-create a similar system as the reporter. The idea of using the longest >> substring match for the cgroupv1 file paths was based on the fact that on >> systemd systems processes run in separate scopes and the maven forked test >> runner might exhibit this property. For that it makes sense to use the >> common ancestor path. Nothing changes in the common cases where the >> `cgroup_path` matches `_root` and when the `_root` is `/` (container the >> former, host system the latter). >> >> In addition, the code paths which are susceptible to throw NPE have been >> hardened to catch those situations. Should it happen in the future it makes >> more sense (to me) to not have accurate container detection in favor of >> continuing to keep running. >> >> Finally, with the added unit-tests a bug was uncovered on the "substring" >> match case of cgroup paths in hotspot. `p` returned from `strstr` can never >> point to `_root` as it's used as the "needle" to find in "haystack" >> `cgroup_path` (not the other way round). >> >> Testing: >> - [x] Added unit tests >> - [x] GHA >> - [x] Container tests on cgroups v1 Linux. Continue to pass > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Use stringStream to simplify controller path assembly src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 92: > 90: } > 91: ss.print_raw(_root, last_matching_slash_pos); > 92: _path = os::strdup(ss.base()); Do you mean `Find the longest common prefix`? Maybe give an example in the comments? Text parsing in C code is really difficult to understand. - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 18:09:40 GMT, Severin Gehwolf wrote:
>> Please review this change to the cgroup v1 subsystem which makes it more
>> resilient on some of the stranger systems. Unfortunately, I wasn't able to
>> re-create a similar system as the reporter. The idea of using the longest
>> substring match for the cgroupv1 file paths was based on the fact that on
>> systemd systems processes run in separate scopes and the maven forked test
>> runner might exhibit this property. For that it makes sense to use the
>> common ancestor path. Nothing changes in the common cases where the
>> `cgroup_path` matches `_root` and when the `_root` is `/` (container the
>> former, host system the latter).
>>
>> In addition, the code paths which are susceptible to throw NPE have been
>> hardened to catch those situations. Should it happen in the future it makes
>> more sense (to me) to not have accurate container detection in favor of
>> continuing to keep running.
>>
>> Finally, with the added unit-tests a bug was uncovered on the "substring"
>> match case of cgroup paths in hotspot. `p` returned from `strstr` can never
>> point to `_root` as it's used as the "needle" to find in "haystack"
>> `cgroup_path` (not the other way round).
>>
>> Testing:
>> - [x] Added unit tests
>> - [x] GHA
>> - [x] Container tests on cgroups v1 Linux. Continue to pass
>
> Severin Gehwolf has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Use stringStream to simplify controller path assembly
test/hotspot/gtest/runtime/test_os_linux_cgroups.cpp line 63:
> 61: ASSERT_STREQ(expected_cg_paths[i], ctrl->subsystem_path());
> 62: }
> 63: }
I found it hard to relate the different paths. Could you create a new struct
like this?
struct TestCase {
char* mount_path;
char* root_paths;
char* cgroup_path;
char* expected_cg_paths;
} = {
{ "/sys/fs/cgroup/memory", // mount
"/", // root,
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 18:09:40 GMT, Severin Gehwolf wrote: >> Please review this change to the cgroup v1 subsystem which makes it more >> resilient on some of the stranger systems. Unfortunately, I wasn't able to >> re-create a similar system as the reporter. The idea of using the longest >> substring match for the cgroupv1 file paths was based on the fact that on >> systemd systems processes run in separate scopes and the maven forked test >> runner might exhibit this property. For that it makes sense to use the >> common ancestor path. Nothing changes in the common cases where the >> `cgroup_path` matches `_root` and when the `_root` is `/` (container the >> former, host system the latter). >> >> In addition, the code paths which are susceptible to throw NPE have been >> hardened to catch those situations. Should it happen in the future it makes >> more sense (to me) to not have accurate container detection in favor of >> continuing to keep running. >> >> Finally, with the added unit-tests a bug was uncovered on the "substring" >> match case of cgroup paths in hotspot. `p` returned from `strstr` can never >> point to `_root` as it's used as the "needle" to find in "haystack" >> `cgroup_path` (not the other way round). >> >> Testing: >> - [x] Added unit tests >> - [x] GHA >> - [x] Container tests on cgroups v1 Linux. Continue to pass > > Severin Gehwolf has updated the pull request incrementally with one > additional commit since the last revision: > > Use stringStream to simplify controller path assembly Thanks for taking my suggestion! Much simpler now :-) - PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
On Thu, 12 May 2022 15:58:57 GMT, Severin Gehwolf wrote:
>> src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 113:
>>
>>> 111: }
>>> 112: buf[MAXPATHLEN-1] = '\0';
>>> 113: _path = os::strdup(buf);
>>
>> I think this code can be simplified a lot with stringStream and without
>> strtok, so no need for fixed buffers (which may fail with longer path names)
>> and no need for writable string copies on the stack.
>>
>> Something like this:
>>
>> stringStream ss;
>> ss.print_raw(_mount_point);
>> const char* p1 = _root;
>> const char* p2 = cgroup_path;
>> int last_matching_dash_pos = -1;
>> for (int i = 0; *p1 == *p2 && *p1 != 0; i ++) {
>> if (*p1 == '/') {
>> last_matching_dash_pos = i;
>> }
>> p1++; p2++;
>> }
>> ss.print_raw(_root, last_matching_dash_pos);
>> // Now use ss.base() to access the assembled string
>
> Nice, thanks! I'll update it.
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/8629
Re: RFR: 8286212: Cgroup v1 initialization causes NPE on some systems [v2]
> Please review this change to the cgroup v1 subsystem which makes it more > resilient on some of the stranger systems. Unfortunately, I wasn't able to > re-create a similar system as the reporter. The idea of using the longest > substring match for the cgroupv1 file paths was based on the fact that on > systemd systems processes run in separate scopes and the maven forked test > runner might exhibit this property. For that it makes sense to use the common > ancestor path. Nothing changes in the common cases where the `cgroup_path` > matches `_root` and when the `_root` is `/` (container the former, host > system the latter). > > In addition, the code paths which are susceptible to throw NPE have been > hardened to catch those situations. Should it happen in the future it makes > more sense (to me) to not have accurate container detection in favor of > continuing to keep running. > > Finally, with the added unit-tests a bug was uncovered on the "substring" > match case of cgroup paths in hotspot. `p` returned from `strstr` can never > point to `_root` as it's used as the "needle" to find in "haystack" > `cgroup_path` (not the other way round). > > Testing: > - [x] Added unit tests > - [x] GHA > - [x] Container tests on cgroups v1 Linux. Continue to pass Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: Use stringStream to simplify controller path assembly - Changes: - all: https://git.openjdk.java.net/jdk/pull/8629/files - new: https://git.openjdk.java.net/jdk/pull/8629/files/bc873b3f..66241aa5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8629&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8629&range=00-01 Stats: 32 lines in 1 file changed: 0 ins; 21 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8629.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8629/head:pull/8629 PR: https://git.openjdk.java.net/jdk/pull/8629
