Re: RFR: 8287107: CgroupSubsystemFactory.setCgroupV2Path asserts with freezer controller [v3]
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]
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]
> 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]
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]
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
