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

2022-05-19 Thread Severin Gehwolf
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]

2022-05-19 Thread Thomas Stuefe
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]

2022-05-18 Thread Severin Gehwolf
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]

2022-05-18 Thread Severin Gehwolf
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]

2022-05-17 Thread Severin Gehwolf
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]

2022-05-17 Thread Severin Gehwolf
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]

2022-05-17 Thread Thomas Stuefe
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]

2022-05-17 Thread Ioi Lam
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]

2022-05-16 Thread Ioi Lam
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]

2022-05-13 Thread Thomas Stuefe
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]

2022-05-12 Thread Severin Gehwolf
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]

2022-05-12 Thread Severin Gehwolf
> 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=8629=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8629=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