Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-25 Thread Ioi Lam
On Tue, 24 May 2022 19:36:57 GMT, Severin Gehwolf  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   @jerboaa comments
>
> Looks good. Thanks for the thorough analysis.

Thanks @jerboaa and @mseledts for the review.

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v4]

2022-05-25 Thread Ioi Lam
> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  fixed typo in comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8858/files
  - new: https://git.openjdk.java.net/jdk/pull/8858/files/9134182e..a13afe47

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8858=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8858=02-03

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8858.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8858/head:pull/8858

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

2022-05-25 Thread Ioi Lam
On Wed, 25 May 2022 15:50:32 GMT, Severin Gehwolf  wrote:

>> It confused me, fwiw. Anyway up to you. It's not super important.
>
> works for me. +1. Note the typo 
> `anyCgroupsV1Controller/anyCgroupsV2Controller` not **V1** twice.

Oops, I'll fixed that. Thanks!

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

2022-05-25 Thread Severin Gehwolf
On Wed, 25 May 2022 15:51:04 GMT, Ioi Lam  wrote:

>> This PR fixes a bug found on an Ubuntu host that's mostly running with 
>> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 
>> mode.
>> 
>> The container support code in the VM and JDK checks if we have 
>> simultaneously mounted v1 and v2 containers. If so, we revert to "hybrid" 
>> mode (which is essentially v1).
>> 
>> However, the "hybrid checks" only considers the following controllers that 
>> Java cares about:
>> 
>> - cpu
>> - cpuacct
>> - cpuset
>> - blkio
>> - memory
>> - pids
>> 
>> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
>> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
>> into the assert.
>> 
>> 
>> $ cat /proc/self/cgroup
>> 1:freezer:/
>> 0::/user.slice/user-1001.slice/session-85.scope
>> 
>> 
>> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
>> decided that the system has not mounted any v1 controllers that we care 
>> about, so we should just ignore all the v1 controllers (which has a 
>> non-empty name).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   fixed comments

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 149:

> 147: // other controllers (such as freezer) are ignored and
> 148: // are not considered in the checks below for
> 149: // anyCgroupsV1Controller/anyCgroupsV1Controller.

It still has the `anyCgroupsV1Controller/anyCgroupsV1Controller` typo. Not 
**V1** twice?

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

2022-05-25 Thread Ioi Lam
> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  fixed comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8858/files
  - new: https://git.openjdk.java.net/jdk/pull/8858/files/1f17b6e8..9134182e

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8858=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8858=01-02

  Stats: 8 lines in 1 file changed: 4 ins; 4 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8858.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8858/head:pull/8858

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

2022-05-25 Thread Severin Gehwolf
On Wed, 25 May 2022 08:40:48 GMT, Severin Gehwolf  wrote:

>> If you don't like the `default:` coding style, how about this:
>> 
>> 
>> switch (info.getName()) {
>> // Only the following controllers are important to Java. All
>> // other controllers (such as freezer) are ignored and
>> // are not considered in the checks below for
>> // anyCgroupsV1Controller/anyCgroupsV1Controller.
>> case CPU_CTRL:  infos.put(CPU_CTRL, info); break;
>> case CPUACCT_CTRL:  infos.put(CPUACCT_CTRL, info); break;
>> case CPUSET_CTRL:   infos.put(CPUSET_CTRL, info); break;
>> case MEMORY_CTRL:   infos.put(MEMORY_CTRL, info); break;
>> case BLKIO_CTRL:infos.put(BLKIO_CTRL, info); break;
>> case PIDS_CTRL: infos.put(PIDS_CTRL, info); break;
>> }
>
> It confused me, fwiw. Anyway up to you. It's not super important.

works for me. +1. Note the typo `anyCgroupsV1Controller/anyCgroupsV2Controller` 
not **V1** twice.

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]

2022-05-25 Thread Ioi Lam
On Tue, 24 May 2022 19:49:35 GMT, Ioi Lam  wrote:

>> My bad. How about `Intentional incomplete switch. There are ...`? Anyway, 
>> why is the empty `default` case needed other than for the comment?
>
> To me, the `default:` switch is a clear indication that "everything else 
> comes here". So you won't be confused whether the comment is related to the 
> last line just above the comment.

If you don't like the `default:` coding style, how about this:


switch (info.getName()) {
// Only the following controllers are important to Java. All
// other controllers (such as freezer) are ignored and
// are not considered in the checks below for
// anyCgroupsV1Controller/anyCgroupsV1Controller.
case CPU_CTRL:  infos.put(CPU_CTRL, info); break;
case CPUACCT_CTRL:  infos.put(CPUACCT_CTRL, info); break;
case CPUSET_CTRL:   infos.put(CPUSET_CTRL, info); break;
case MEMORY_CTRL:   infos.put(MEMORY_CTRL, info); break;
case BLKIO_CTRL:infos.put(BLKIO_CTRL, info); break;
case PIDS_CTRL: infos.put(PIDS_CTRL, info); break;
}

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-25 Thread Severin Gehwolf
On Tue, 24 May 2022 19:49:35 GMT, Ioi Lam  wrote:

>> My bad. How about `Intentional incomplete switch. There are ...`? Anyway, 
>> why is the empty `default` case needed other than for the comment?
>
> To me, the `default:` switch is a clear indication that "everything else 
> comes here". So you won't be confused whether the comment is related to the 
> last line just above the comment.

It confused me, fwiw. Anyway up to you. It's not super important.

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-24 Thread Ioi Lam
On Tue, 24 May 2022 19:34:16 GMT, Severin Gehwolf  wrote:

>> This is not a fall-through because the previous line ends with a `break`.
>
> My bad. How about `Intentional incomplete switch. There are ...`? Anyway, why 
> is the empty `default` case needed other than for the comment?

To me, the `default:` switch is a clear indication that "everything else comes 
here". So you won't be confused whether the comment is related to the last line 
just above the comment.

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-24 Thread Severin Gehwolf
On Tue, 24 May 2022 16:09:54 GMT, Ioi Lam  wrote:

>> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
>>  line 155:
>> 
>>> 153: // There are some controllers (such as freezer) that 
>>> Java doesn't
>>> 154: // care about. Just ignore them. These are not 
>>> considered in the
>>> 155: // anyCgroupsV1Controller/anyCgroupsV1Controller 
>>> checks.
>> 
>> It's not clear why this `default` is necessary. Could we just add the 
>> comment like so:
>> 
>> 
>> // Intentional fall-through. There are controllers (such as freezer) that
>> // Java doesn't care about. Just ignore them. Only listed controllers are
>> // considered in the anyCgroupsV1Controller/anyCgroupsV2Controller checks.
>
> This is not a fall-through because the previous line ends with a `break`.

My bad. How about `Intentional incomplete switch. There are ...`? Anyway, why 
is the empty `default` case needed other than for the comment?

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-24 Thread Severin Gehwolf
On Tue, 24 May 2022 16:15:03 GMT, Ioi Lam  wrote:

>> This PR fixes a bug found on an Ubuntu host that's mostly running with 
>> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 
>> mode.
>> 
>> The container support code in the VM and JDK checks if we have 
>> simultaneously mounted v1 and v2 containers. If so, we revert to "hybrid" 
>> mode (which is essentially v1).
>> 
>> However, the "hybrid checks" only considers the following controllers that 
>> Java cares about:
>> 
>> - cpu
>> - cpuacct
>> - cpuset
>> - blkio
>> - memory
>> - pids
>> 
>> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
>> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
>> into the assert.
>> 
>> 
>> $ cat /proc/self/cgroup
>> 1:freezer:/
>> 0::/user.slice/user-1001.slice/session-85.scope
>> 
>> 
>> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
>> decided that the system has not mounted any v1 controllers that we care 
>> about, so we should just ignore all the v1 controllers (which has a 
>> non-empty name).
>
> Ioi Lam has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   @jerboaa comments

Looks good. Thanks for the thorough analysis.

-

Marked as reviewed by sgehwolf (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-24 Thread Ioi Lam
On Tue, 24 May 2022 10:12:31 GMT, Severin Gehwolf  wrote:

>> Ioi Lam has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   @jerboaa comments
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
> line 155:
> 
>> 153: // There are some controllers (such as freezer) that 
>> Java doesn't
>> 154: // care about. Just ignore them. These are not 
>> considered in the
>> 155: // anyCgroupsV1Controller/anyCgroupsV1Controller checks.
> 
> It's not clear why this `default` is necessary. Could we just add the comment 
> like so:
> 
> 
> // Intentional fall-through. There are controllers (such as freezer) that
> // Java doesn't care about. Just ignore them. Only listed controllers are
> // considered in the anyCgroupsV1Controller/anyCgroupsV2Controller checks.

This is not a fall-through because the previous line ends with a `break`.

> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
> line 229:
> 
>> 227: String name = tokens[1];
>> 228: if (!name.equals("")) {
>> 229: // This is probably a v1 controller that we have ignored 
>> (e.g., freezer)
> 
> Could we add an assertion that the controller isn't in the `infos` map?
> 
>if (!name.equals("")) {
> // This must be a v1 controller that we have ignored (e.g., 
> freezer)
> assert infos.get(name) == null;
> return;
>  }

Fixed.

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v2]

2022-05-24 Thread Ioi Lam
> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Ioi Lam has updated the pull request incrementally with one additional commit 
since the last revision:

  @jerboaa comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/8858/files
  - new: https://git.openjdk.java.net/jdk/pull/8858/files/c4d8354d..1f17b6e8

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=8858=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8858=00-01

  Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/8858.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/8858/head:pull/8858

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller

2022-05-24 Thread Severin Gehwolf
On Mon, 23 May 2022 22:11:47 GMT, Ioi Lam  wrote:

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 155:

> 153: // There are some controllers (such as freezer) that 
> Java doesn't
> 154: // care about. Just ignore them. These are not 
> considered in the
> 155: // anyCgroupsV1Controller/anyCgroupsV1Controller checks.

It's not clear why this `default` is necessary. Could we just add the comment 
like so:


// Intentional fall-through. There are controllers (such as freezer) that
// Java doesn't care about. Just ignore them. Only listed controllers are
// considered in the anyCgroupsV1Controller/anyCgroupsV2Controller checks.

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java 
line 229:

> 227: String name = tokens[1];
> 228: if (!name.equals("")) {
> 229: // This is probably a v1 controller that we have ignored 
> (e.g., freezer)

Could we add an assertion that the controller isn't in the `infos` map?

   if (!name.equals("")) {
// This must be a v1 controller that we have ignored (e.g., freezer)
assert infos.get(name) == null;
return;
 }

-

PR: https://git.openjdk.java.net/jdk/pull/8858


Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller

2022-05-23 Thread Mikhailo Seledtsov
On Mon, 23 May 2022 22:11:47 GMT, Ioi Lam  wrote:

> This PR fixes a bug found on an Ubuntu host that's mostly running with 
> cgroupv2, but there's a controller (freezer) that is mounted in cgroupv1 mode.
> 
> The container support code in the VM and JDK checks if we have simultaneously 
> mounted v1 and v2 containers. If so, we revert to "hybrid" mode (which is 
> essentially v1).
> 
> However, the "hybrid checks" only considers the following controllers that 
> Java cares about:
> 
> - cpu
> - cpuacct
> - cpuset
> - blkio
> - memory
> - pids
> 
> If only "freezer" is mounted in v1 mode, Java will start in V2 mode. Later, 
> when `setCgroupV2Path()` sees the first line in /proc/self/cgroup, it runs 
> into the assert.
> 
> 
> $ cat /proc/self/cgroup
> 1:freezer:/
> 0::/user.slice/user-1001.slice/session-85.scope
> 
> 
> The fix is simple -- when we get to `setCgroupV2Path()`, we have already 
> decided that the system has not mounted any v1 controllers that we care 
> about, so we should just ignore all the v1 controllers (which has a non-empty 
> name).

Marked as reviewed by mseledtsov (Committer).

Change looks good to me. Thanks for the fix.

-

PR: https://git.openjdk.java.net/jdk/pull/8858