RFR: 8365606: Container code should not be using jlong/julong
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values. It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned. All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for. All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code. While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large. Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command. Thoughts? Opinions? ------------- Commit messages: - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong Changes: https://git.openjdk.org/jdk/pull/27743/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8365606 Stats: 1247 lines in 16 files changed: 477 ins; 111 del; 659 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
## Testing details: **cgroup v2 F42 (docker):** Passed: containers/cgroup/CgroupSubsystemFactory.java Passed: containers/cgroup/TestContainerized.java Passed: containers/docker/DockerBasicTest.java Passed: containers/docker/ShareTmpDir.java Passed: containers/docker/TestContainerInfo.java Passed: containers/docker/TestCPUAwareness.java Passed: containers/docker/TestCPUSets.java Passed: containers/docker/TestJcmd.java Passed: containers/docker/TestJcmdWithSideCar.java Passed: containers/docker/TestJFREvents.java Passed: containers/docker/TestJFRNetworkEvents.java Passed: containers/docker/TestJFRWithJMX.java Passed: containers/docker/TestLimitsUpdating.java Passed: containers/docker/TestMemoryAwareness.java Passed: containers/docker/TestMemoryWithCgroupV1.java Passed: containers/docker/TestMemoryWithSubgroups.java Passed: containers/docker/TestMisc.java Passed: containers/docker/TestPids.java Passed: containers/systemd/SystemdMemoryAwarenessTest.java Passed: jdk/internal/platform/cgroup/CgroupV1SubsystemControllerTest.java Passed: jdk/internal/platform/cgroup/CgroupV2SubsystemControllerTest.java Passed: jdk/internal/platform/cgroup/TestCgroupMetrics.java Passed: jdk/internal/platform/cgroup/TestCgroupSubsystemController.java Passed: jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java Passed: jdk/internal/platform/cgroup/TestSystemSettings.java Passed: jdk/internal/platform/docker/TestDockerBasic.java Passed: jdk/internal/platform/docker/TestDockerCpuMetrics.java Passed: jdk/internal/platform/docker/TestDockerMemoryMetrics.java Passed: jdk/internal/platform/docker/TestDockerMemoryMetricsSubgroup.java Passed: jdk/internal/platform/docker/TestGetFreeSwapSpaceSize.java Passed: jdk/internal/platform/docker/TestLimitsUpdating.java Passed: jdk/internal/platform/docker/TestPidsLimit.java Passed: jdk/internal/platform/docker/TestSystemMetrics.java Passed: jdk/internal/platform/docker/TestUseContainerSupport.java Test results: passed: 34 **cgroup v2 F39 (docker):** Passed: containers/cgroup/CgroupSubsystemFactory.java Passed: containers/cgroup/TestContainerized.java Passed: containers/docker/DockerBasicTest.java Passed: containers/docker/ShareTmpDir.java Passed: containers/docker/TestContainerInfo.java Passed: containers/docker/TestCPUAwareness.java Passed: containers/docker/TestCPUSets.java Passed: containers/docker/TestJcmd.java Passed: containers/docker/TestJcmdWithSideCar.java Passed: containers/docker/TestJFREvents.java Passed: containers/docker/TestJFRNetworkEvents.java Passed: containers/docker/TestJFRWithJMX.java Passed: containers/docker/TestLimitsUpdating.java Passed: containers/docker/TestMemoryAwareness.java Passed: containers/docker/TestMemoryWithCgroupV1.java Passed: containers/docker/TestMemoryWithSubgroups.java Passed: containers/docker/TestMisc.java Passed: containers/docker/TestPids.java Passed: containers/systemd/SystemdMemoryAwarenessTest.java Test results: passed: 19 **cgroup v1 RHEL 8 (podman):** Passed: containers/cgroup/CgroupSubsystemFactory.java Passed: containers/cgroup/TestContainerized.java Passed: containers/docker/DockerBasicTest.java Passed: containers/docker/ShareTmpDir.java Passed: containers/docker/TestContainerInfo.java Passed: containers/docker/TestCPUAwareness.java Passed: containers/docker/TestCPUSets.java Passed: containers/docker/TestJcmd.java Passed: containers/docker/TestJcmdWithSideCar.java Passed: containers/docker/TestJFREvents.java Passed: containers/docker/TestJFRNetworkEvents.java Passed: containers/docker/TestJFRWithJMX.java Passed: containers/docker/TestLimitsUpdating.java Passed: containers/docker/TestMemoryAwareness.java Passed: containers/docker/TestMemoryWithCgroupV1.java Passed: containers/docker/TestMemoryWithSubgroups.java Passed: containers/docker/TestMisc.java Passed: containers/docker/TestPids.java Passed: containers/systemd/SystemdMemoryAwarenessTest.java Test results: passed: 19; skipped: 1 Note: The skipped test on RHEL 8 is `TestContainerInfo` which checks `cgv2` functionality only. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3390060127
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
GHA test failure on Windows x64 is unrelated (this is a Linux only patch): [gc/shenandoah/oom/TestThreadFailure](https://github.com/jerboaa/jdk/actions/runs/18406968978#user-content-gc_shen...) ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3390913305
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
There seems to be some collision here with JDK-[8367319](https://github.com/openjdk/jdk/pull/27646/) ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3395619340
On Mon, 13 Oct 2025 01:46:43 GMT, David Holmes <dholmes@openjdk.org> wrote:
There seems to be some collision here with JDK-[8367319](https://github.com/openjdk/jdk/pull/27646/)
Thanks, yes, I'm aware. We'll resolve conflicts once we know which one goes in first. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3396358387
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Could anybody please help review this! Thank you. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3441752392
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
I'm not surprised about the lack of reviews because it's long and the result is rather ugly. (Sorry, but I had to say it.) This is less jarring to read: struct Result { const int _value; const bool _ok; Result(int value, bool ok) : _value(value), _ok(ok) {} }; ... Result CgroupSubsystem::active_processor_count() { ... cpu_count = os::Linux::active_processor_count(); if (!CgroupUtil::processor_count(contrl->controller(), cpu_count, value)) { return Result(value, false); } assert(value > 0 && value <= cpu_count, "must be"); // Update cached metric to avoid re-reading container settings too often cpu_limit->set_value(value, OSCONTAINER_CACHE_TIMEOUT); return Result(value, true); } called as: auto [result, ok] = cgroup_subsystem->active_processor_count(); if (ok) ... ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3442136772
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision: - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong ------------- Changes: - all: https://git.openjdk.org/jdk/pull/27743/files - new: https://git.openjdk.org/jdk/pull/27743/files/e906490e..917a51c6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=00-01 Stats: 39117 lines in 885 files changed: 23412 ins; 10970 del; 4735 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
On Fri, 24 Oct 2025 09:45:25 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
I'm currently looking at this, just as a quick FYI. I'm fine with the proposed interface as it is (boolean return + passed reference). Since the adjacent `os` functions recently changed to use this interface, and the disproportionate (to me) amount of bikeshedding it took to reach a consensus on that, introducing yet another interface here, for what's essentially a subcomponent, doesn't make much sense. It's clearer if the whole `os` stack sticks to the same interface. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3442470839
On Fri, 24 Oct 2025 10:46:52 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
I'm currently looking at this, just as a quick FYI.
I'm fine with the proposed interface as it is (boolean return + passed reference). Since the adjacent `os` functions recently changed to use this interface, and the disproportionate (to me) amount of bikeshedding it took to reach a consensus on that, introducing yet another interface here, for what's essentially a subcomponent, doesn't make much sense. It's clearer if the whole `os` stack sticks to the same interface.
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3442620964
On Fri, 24 Oct 2025 11:24:48 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again.
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3442865789
On Fri, 24 Oct 2025 12:25:36 GMT, Andrew Haley <aph@openjdk.org> wrote:
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again.
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus.
FWIW, I'd be interested in seeing a small example of what that would look like with C++17. There were a lot of discussion about the style, but it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3450057392
On Mon, 27 Oct 2025 08:33:22 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:
it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation.
This. A function that returns its value as a side effect on a reference parameter is (at best) a code smell. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3450229373
On Mon, 27 Oct 2025 09:17:02 GMT, Andrew Haley <aph@openjdk.org> wrote:
it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation.
This. A function that returns its value as a side effect on a reference parameter is (at best) a code smell.
Thanks for the comments. So what's the consensus then? As far as API surface is concerned I've modelled it after [JDK-8357086](https://bugs.openjdk.org/browse/JDK-8357086). It [introduces](https://github.com/openjdk/jdk/commit/d5d94db12a6d82a6fe9da18b5f8ce3733a6ee7...) the side-effect/code smell issue. Do we want to re-open this discussion or proceed with this here. It's not clear to me. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3469549344
On Mon, 27 Oct 2025 09:17:02 GMT, Andrew Haley <aph@openjdk.org> wrote:
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again.
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus.
FWIW, I'd be interested in seeing a small example of what that would look like with C++17. There were a lot of discussion about the style, but it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation.
it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation.
This. A function that returns its value as a side effect on a reference parameter is (at best) a code smell.
@theRealAph @stefank OK to integrate this? ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3543074389
On Mon, 27 Oct 2025 08:33:22 GMT, Stefan Karlsson <stefank@openjdk.org> wrote:
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again.
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus.
I agree. Its not pretty, but consistent with what we did elsewhere. Nobody wants to do that discussion again.
Sorry, I was unaware of any previous discussion. I was suggesting a less impactful way to make the change, taking advantage of the recent adoption of C++17, which allows for cleaner code. But I won't stand in the way of consensus.
FWIW, I'd be interested in seeing a small example of what that would look like with C++17. There were a lot of discussion about the style, but it wasn't because we wanted to figure out the color of the bike shed but rather how to write safer code that makes it less likely to accidentally introduce bugs because of type conflation.
@stefank OK to integrate this?
Yes. I took a quick glance at the changes and it looks like the previous style that was made for the other os:: memory APIs. I'm deferring the responsibility to do a full Review to Casper and Thomas. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3543621156
On Fri, 24 Oct 2025 09:45:25 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
Mostly looks good to me. Some remnants of raw UINT64_FORMAT are left. I did not see any type clashes. I hold off the final review until it is clear that the interfaces are agreed upon. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 638:
636: bool CgroupSubsystem::active_processor_count(int& value) { 637: int cpu_count; 638: int result = -1;
Why not get rid of result and use `value` throughout like you did in the cached case? src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 80:
78: return false; \ 79: } \ 80: log_trace(os, container)(log_string " is: " UINT64_FORMAT, retval); \
Here and in other places: don't use raw UINT64_FORMAT; use `PHYS_MEM_TYPE_FORMAT` instead. src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 105:
103: is_ok = controller->read_string(filename, retval, buf_size); \ 104: if (!is_ok) { \ 105: log_trace(os, container)(log_string " failed: -2"); \
Why this change? Did the constant value change? ------------- PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3375466876 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2459590069 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2459894453 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2459898806
On Fri, 24 Oct 2025 09:50:33 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 638:
636: bool CgroupSubsystem::active_processor_count(int& value) { 637: int cpu_count; 638: int result = -1;
Why not get rid of result and use `value` throughout like you did in the cached case?
It's useful to do assertions on the value retrieved by `CgroupUtil::processor_count()` before the actual result is being changed. Using `value` has the issue of not knowing what the reference default value was set to. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510522176
On Fri, 24 Oct 2025 11:22:01 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 80:
78: return false; \ 79: } \ 80: log_trace(os, container)(log_string " is: " UINT64_FORMAT, retval); \
Here and in other places: don't use raw UINT64_FORMAT; use `PHYS_MEM_TYPE_FORMAT` instead.
This is intentional since the processor_count API doesn't use `physical_memory_size_type` (as it doesn't make sense in this context). See, for example, `CgroupV2CpuController::cpu_period()`. The common denominator is `uint64_t`. This is a bit awkward, but I don't know a better way to deal with this. The reading functions are shared, most of the API is used for memory value reading (but not exclusively, exceptions are `pid`, `cpu`). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510577587
On Mon, 10 Nov 2025 13:27:59 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 80:
78: return false; \ 79: } \ 80: log_trace(os, container)(log_string " is: " UINT64_FORMAT, retval); \
Here and in other places: don't use raw UINT64_FORMAT; use `PHYS_MEM_TYPE_FORMAT` instead.
This is intentional since the processor_count API doesn't use `physical_memory_size_type` (as it doesn't make sense in this context). See, for example, `CgroupV2CpuController::cpu_period()`. The common denominator is `uint64_t`. This is a bit awkward, but I don't know a better way to deal with this. The reading functions are shared, most of the API is used for memory value reading (but not exclusively, exceptions are `pid`, `cpu`).
Okay ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2527193808
On Fri, 24 Oct 2025 11:23:05 GMT, Thomas Stuefe <stuefe@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 105:
103: is_ok = controller->read_string(filename, retval, buf_size); \ 104: if (!is_ok) { \ 105: log_trace(os, container)(log_string " failed: -2"); \
Why this change? Did the constant value change?
Motivation was getting rid of the OSCONTAINER_ERROR constant. The only place where a negative number was still in use. I've just dropped the `: -2` suffix now. It's not very useful (other than in tests). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510615109
On Fri, 24 Oct 2025 09:45:25 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
_I am not a reviewer._ This looks good to me, overall. I commented with some items to consider. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 627:
625: * 626: * If quotas have not been specified, return the 627: * number of active processors in the system.
This paragraph uses the "return" language that you adjusted in the next paragraph. It should probably also refer to the reference argument instead. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 629:
627: * number of active processors in the system. 628: * 629: * If quotas have been specified, the resulting number
Tiny nit, but "the resulting number" => "the number", since you say "the result reference" on the next line. src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 653:
651: cpu_count = os::Linux::active_processor_count(); 652: if (!CgroupUtil::processor_count(contrl->controller(), cpu_count, result)) { 653: return false;
`value` will be returned unchanged from its passed-in value here. I wonder if it would be safer to explicitly set it to `0` when returning `false`. Also, could `value` be given an unsigned type, like `uint64_t`? src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 667:
665: * Return the limit of available memory for this process in the provided 666: * physical_memory_size_type reference. If there was no limit value set in the underlying 667: * interface files value_unlimited is returned.
I think quote `value_unlimited` here to hint that it is a constant defined elsewhere. Can the limit ever be `0`, and if not, should there be a new assert for `> 0` like for `cpu_count`? src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 167:
165: /* memory_and_swap_limit_in_bytes 166: * 167: * Determine the memory and swap limit metric. Returns a positive limit value or
"Returns" language should probably be updated here too. src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 470:
468: // cast to int since the read value might be negative 469: // and we want to avoid logging -1 as a large unsigned value. 470: int quota_int = static_cast<int>(quota);
It seems like quota is either a positive number or disabled. I wonder if `result` can be treated as a `uint64_t`, and this log message special-cased to detect `-1` read from `/cpu.cfs_quota_us` as disabled. I guess the calling code would need another way to differentiate "disabled" from other values... maybe with `0`? Just a thought to maybe simplify the type logic here. Likewise for `period` and `shares`. ------------- Marked as reviewed by fitzsim (Author). PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3383739312 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465851974 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465858316 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465872342 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465907552 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2466836781 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2466892281
On Mon, 27 Oct 2025 14:13:57 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 627:
625: * 626: * If quotas have not been specified, return the 627: * number of active processors in the system.
This paragraph uses the "return" language that you adjusted in the next paragraph. It should probably also refer to the reference argument instead.
Thanks, fixed.
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 629:
627: * number of active processors in the system. 628: * 629: * If quotas have been specified, the resulting number
Tiny nit, but "the resulting number" => "the number", since you say "the result reference" on the next line.
Fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510514731 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510515207
On Mon, 27 Oct 2025 14:19:19 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.cpp line 653:
651: cpu_count = os::Linux::active_processor_count(); 652: if (!CgroupUtil::processor_count(contrl->controller(), cpu_count, result)) { 653: return false;
`value` will be returned unchanged from its passed-in value here. I wonder if it would be safer to explicitly set it to `0` when returning `false`. Also, could `value` be given an unsigned type, like `uint64_t`?
The general contract in those functions is that the result reference is unchanged when `false` is being returned. So this is intentional.
Also, could value be given an unsigned type, like uint64_t
I've tried to keep the `int` based processor_count API as is. Not sure if we need an unsigned type here. We could if that's the consensus, but then it would make the patch even larger. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510540986
On Mon, 27 Oct 2025 14:29:11 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
I think quote value_unlimited here to hint that it is a constant defined elsewhere.
OK.
Can the limit ever be 0, and if not, should there be a new assert for > 0 like for cpu_count?
The limit could theoretically be `0`. I'd try to avoid an overzealous assert here. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510555699
On Mon, 27 Oct 2025 19:28:08 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 167:
165: /* memory_and_swap_limit_in_bytes 166: * 167: * Determine the memory and swap limit metric. Returns a positive limit value or
"Returns" language should probably be updated here too.
Thanks! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510646920
On Mon, 27 Oct 2025 19:48:58 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 470:
468: // cast to int since the read value might be negative 469: // and we want to avoid logging -1 as a large unsigned value. 470: int quota_int = static_cast<int>(quota);
It seems like quota is either a positive number or disabled. I wonder if `result` can be treated as a `uint64_t`, and this log message special-cased to detect `-1` read from `/cpu.cfs_quota_us` as disabled. I guess the calling code would need another way to differentiate "disabled" from other values... maybe with `0`? Just a thought to maybe simplify the type logic here. Likewise for `period` and `shares`.
Yes, there is opportunity to change the API. This patch was done to do a 1-to-1 translation of the previous version as much as possible. So I've refrained from doing this in this patch as well. It kept the size of the patch a bit more manageable. Happy to file a follow-up RFE to do this in a separate patch. Thoughts? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510666794
On Mon, 10 Nov 2025 13:53:11 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 470:
468: // cast to int since the read value might be negative 469: // and we want to avoid logging -1 as a large unsigned value. 470: int quota_int = static_cast<int>(quota);
It seems like quota is either a positive number or disabled. I wonder if `result` can be treated as a `uint64_t`, and this log message special-cased to detect `-1` read from `/cpu.cfs_quota_us` as disabled. I guess the calling code would need another way to differentiate "disabled" from other values... maybe with `0`? Just a thought to maybe simplify the type logic here. Likewise for `period` and `shares`.
Yes, there is opportunity to change the API. This patch was done to do a 1-to-1 translation of the previous version as much as possible. So I've refrained from doing this in this patch as well. It kept the size of the patch a bit more manageable. Happy to file a follow-up RFE to do this in a separate patch. Thoughts?
Maybe the API as-is is clearer, because it matches the actual `/proc` values. Having thought about it more, it probably doesn't make sense to change the API just to make the implementation's type handling cleaner, so I'd say don't bother with the follow-up RFE. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510770596
On Mon, 10 Nov 2025 14:24:48 GMT, Thomas Fitzsimmons <fitzsim@openjdk.org> wrote:
Yes, there is opportunity to change the API. This patch was done to do a 1-to-1 translation of the previous version as much as possible. So I've refrained from doing this in this patch as well. It kept the size of the patch a bit more manageable. Happy to file a follow-up RFE to do this in a separate patch. Thoughts?
Maybe the API as-is is clearer, because it matches the actual `/proc` values. Having thought about it more, it probably doesn't make sense to change the API just to make the implementation's type handling cleaner, so I'd say don't bother with the follow-up RFE.
OK. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2511346360
On Fri, 24 Oct 2025 09:45:25 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
Thanks for taking this on! I think this is a solid improvement, and it's great to finally see the Java types phased out. I had a few thoughts/comments, mostly around the os-level code and the logging of constants, since those constants don't really have much meaning anymore. Otherwise great work! src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 90:
88: if (!is_ok) { \ 89: log_trace(os, container)(log_string " failed: -2"); \ 90: return false; \
Do we need to keep the `-2` here? Or could we perhaps change to a better message? src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 93:
91: } \ 92: if (retval == value_unlimited) { \ 93: log_trace(os, container)(log_string " is: -1"); \
Same here, could perhaps do `log_trace(os, container)(log_string " is: unlimited")`instead. src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 465:
463: // negative value as a large unsiged int 464: if (!reader()->read_number("/cpu.cfs_quota_us", quota)) { 465: log_trace(os, container)("CPU Quota failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message? src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 61:
59: * true if the result reference got updated 60: * false if there was an error 61: */
We set result to `-1` and return true on a no share setup here, but return `false` and don't on cgroup v1. The comment is contradicting. src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 115:
113: * true if the result reference has been set 114: * false on error 115: */
The beginning part of the comment isn't updated to mention the `result` reference, unlike the other comments. src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 178:
176: bool is_ok = reader()->read_numerical_key_value("/cpu.stat", "usage_usec", value); 177: if (!is_ok) { 178: log_trace(os, container)("CPU Usage failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message? src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 237:
235: if (!reader()->read_number_handle_max("/memory.swap.max", swap_limit_val)) { 236: // Some container tests rely on this trace logging to happen. 237: log_trace(os, container)("Swap Limit failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message? src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 379:
377: * Calculate the maximum number of tasks available to the process. Set the 378: * value in the passed in 'value' reference. The value might be -1 when 379: * there is no limit.
How can we get `-1`? Or do you mean `(uint64_t)-1`? src/hotspot/os/linux/os_linux.cpp line 220:
218: if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(avail_mem)) { 219: log_trace(os)("available container memory: " PHYS_MEM_TYPE_FORMAT, avail_mem); 220: value = avail_mem;
Should be able to pass in `value` directly instead of using `avail_mem`. src/hotspot/os/linux/os_linux.cpp line 261:
259: if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(free_mem)) { 260: log_trace(os)("free container memory: " PHYS_MEM_TYPE_FORMAT, free_mem); 261: value = free_mem;
Should be able to pass in `value` directly instead of using `free_mem`. src/hotspot/os/linux/os_linux.cpp line 348:
346: return true; 347: } 348: }
This whole function is getting a bit too long in my opinion. Maybe everything inside the `if OSContainer::is_containerized() {}` could be moved into a new function `OSContainer::available_swap_in_bytes`, similar to the already existing `OSContainer::available_memory_in_bytes`. That way, we could abstract away all the `OSContainer` calls. The only consequence would be that the `log_trace` wouldn't work any more. I couldn't find any test that depends on the exact output, so it could perhaps be split up instead. src/hotspot/os/linux/os_linux.cpp line 4863:
4861: if (OSContainer::is_containerized() && OSContainer::active_processor_count(active_cpus)) { 4862: log_trace(os)("active_processor_count: determined by OSContainer: %d", 4863: active_cpus);
When running containerized, we would now always fetch the os cpu count at least once. `CgroupSubsystem::active_processor_count`, which this calls down to has the cache to actively avoid getting the cpu count too frequently, and only gets the number of cpus with `os::Linux::active_processor_count` when the cache expires. I don't know if this is still an issue today, but since it's there I still think we should avoid getting the cpus if unnecessary. src/hotspot/share/runtime/os.cpp line 2215:
2213: } 2214: value = mem_usage; 2215: return true;
Can we collapse this and just set the `value` reference directly instead in the container functions? Something like: ```c++ if (OSContainer::is_containerized()) { return OSContainer::memory_usage_in_bytes(mem_usage); } ------------- PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3365531156 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2460075401 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2460081327 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2468689176 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465321137 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465329926 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465347559 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465361453 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2465754752 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2452045546 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2452052597 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2452159278 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2452239511 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2451900698
On Fri, 24 Oct 2025 12:03:26 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 90:
88: if (!is_ok) { \ 89: log_trace(os, container)(log_string " failed: -2"); \ 90: return false; \
Do we need to keep the `-2` here? Or could we perhaps change to a better message?
We don't need the `-2` here. This was an attempt to keep backwards compatible, but I guess we can change testing code as well (at least those that rely on those trace logs). ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510588965
On Fri, 24 Oct 2025 12:04:51 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupSubsystem_linux.hpp line 93:
91: } \ 92: if (retval == value_unlimited) { \ 93: log_trace(os, container)(log_string " is: -1"); \
Same here, could perhaps do `log_trace(os, container)(log_string " is: unlimited")`instead.
OK. This will likely need some test adjustment, but I'll do that instead of hard-coding those numbers. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510609021
On Tue, 28 Oct 2025 09:26:09 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV1Subsystem_linux.cpp line 465:
463: // negative value as a large unsiged int 464: if (!reader()->read_number("/cpu.cfs_quota_us", quota)) { 465: log_trace(os, container)("CPU Quota failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message?
I've dropped `: -2` suffix now. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510649413
On Mon, 27 Oct 2025 11:32:34 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 61:
59: * true if the result reference got updated 60: * false if there was an error 61: */
We set result to `-1` and return true on a no share setup here, but return `false` and don't on cgroup v1. The comment is contradicting.
Good catch. Fixed the cgroup v1 code to match the old behaviour (set `-1` in the result reference and return `true` if we read the default value). I think this fixes the issue. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510712320
On Mon, 27 Oct 2025 11:43:34 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 178:
176: bool is_ok = reader()->read_numerical_key_value("/cpu.stat", "usage_usec", value); 177: if (!is_ok) { 178: log_trace(os, container)("CPU Usage failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message?
Thanks. I've removed the `-2`.
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 237:
235: if (!reader()->read_number_handle_max("/memory.swap.max", swap_limit_val)) { 236: // Some container tests rely on this trace logging to happen. 237: log_trace(os, container)("Swap Limit failed: -2");
Do we need to keep the `-2` here? Or could we perhaps change to a better message?
I've removed the `-2`. I.e. `Swap Limit failed` is the log message. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510722882 PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510726448
On Mon, 27 Oct 2025 13:49:03 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 379:
377: * Calculate the maximum number of tasks available to the process. Set the 378: * value in the passed in 'value' reference. The value might be -1 when 379: * there is no limit.
How can we get `-1`? Or do you mean `(uint64_t)-1`?
This was meant to say `value_unlimited` if there is `max` in the `pids.max` interface file. Updated the comment and changed the code handling for `VM.info` to handle `value_unlimited`. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510823182
On Wed, 22 Oct 2025 13:09:36 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/os_linux.cpp line 220:
218: if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(avail_mem)) { 219: log_trace(os)("available container memory: " PHYS_MEM_TYPE_FORMAT, avail_mem); 220: value = avail_mem;
Should be able to pass in `value` directly instead of using `avail_mem`.
Sure, thanks! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510872717
On Wed, 22 Oct 2025 13:11:49 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/os_linux.cpp line 261:
259: if (OSContainer::is_containerized() && OSContainer::available_memory_in_bytes(free_mem)) { 260: log_trace(os)("free container memory: " PHYS_MEM_TYPE_FORMAT, free_mem); 261: value = free_mem;
Should be able to pass in `value` directly instead of using `free_mem`.
Fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510884240
On Wed, 22 Oct 2025 14:09:48 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/os_linux.cpp line 4863:
4861: if (OSContainer::is_containerized() && OSContainer::active_processor_count(active_cpus)) { 4862: log_trace(os)("active_processor_count: determined by OSContainer: %d", 4863: active_cpus);
When running containerized, we would now always fetch the os cpu count at least once.
`CgroupSubsystem::active_processor_count`, which this calls down to has the cache to actively avoid getting the cpu count too frequently, and only gets the number of cpus with `os::Linux::active_processor_count` when the cache expires.
I don't know if this is still an issue today, but since it's there I still think we should avoid getting the cpus if unnecessary.
Thanks, fixed. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510972683
On Wed, 22 Oct 2025 12:21:10 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/share/runtime/os.cpp line 2215:
2213: } 2214: value = mem_usage; 2215: return true;
Can we collapse this and just set the `value` reference directly instead in the container functions? Something like:
```c++ if (OSContainer::is_containerized()) { return OSContainer::memory_usage_in_bytes(mem_usage); }
Sure. Thanks! ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2510986032
On Wed, 22 Oct 2025 13:44:56 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits:
- Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - Better logging for -1 (cpu_shares) - Fix cg v1 cpu_shares to match old behaviour - More comment fixes. - Drop -1 (unlimited) and -2 (failed) constants
Will likely need corresponding test changes - ... and 11 more: https://git.openjdk.org/jdk/compare/d5803aa7...08f1c185
src/hotspot/os/linux/os_linux.cpp line 348:
346: return true; 347: } 348: }
This whole function is getting a bit too long in my opinion. Maybe everything inside the `if OSContainer::is_containerized() {}` could be moved into a new function `OSContainer::available_swap_in_bytes`, similar to the already existing `OSContainer::available_memory_in_bytes`. That way, we could abstract away all the `OSContainer` calls.
The only consequence would be that the `log_trace` wouldn't work any more. I couldn't find any test that depends on the exact output, so it could perhaps be split up instead.
I've moved this to a `OSContainer::available_swap_in_bytes` function. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2511339099
On Mon, 10 Nov 2025 17:09:54 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
src/hotspot/os/linux/os_linux.cpp line 348:
346: return true; 347: } 348: }
This whole function is getting a bit too long in my opinion. Maybe everything inside the `if OSContainer::is_containerized() {}` could be moved into a new function `OSContainer::available_swap_in_bytes`, similar to the already existing `OSContainer::available_memory_in_bytes`. That way, we could abstract away all the `OSContainer` calls.
The only consequence would be that the `log_trace` wouldn't work any more. I couldn't find any test that depends on the exact output, so it could perhaps be split up instead.
I've moved this to a `OSContainer::available_swap_in_bytes` function.
Example trace log (if it fails) is: [0.672s][trace][os,container] OSContainer::available_swap_in_bytes: container_swap_limit=unlimited container_mem_limit=1073741824, host_free_swap: 8589844480 [0.672s][trace][os,container] os::free_swap_space: containerized value unavailable returning host value: 8589844480 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2511340779
On Mon, 27 Oct 2025 11:36:39 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Fix print_container_info output - whitespace clean-ups and other small fixes - Fix log format in container macro and scanf format - Fix duplicate include in osContainer_linux - 8365606: Container code should not be using jlong/julong
src/hotspot/os/linux/cgroupV2Subsystem_linux.cpp line 115:
113: * true if the result reference has been set 114: * false on error 115: */
The beginning part of the comment isn't updated to mention the `result` reference, unlike the other comments.
Should be fixed in https://github.com/openjdk/jdk/pull/27743/commits/46df71e19458b1682d6b8a28ef... ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/27743#discussion_r2511363227
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 21 commits: - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - Better logging for -1 (cpu_shares) - Fix cg v1 cpu_shares to match old behaviour - More comment fixes. - Drop -1 (unlimited) and -2 (failed) constants Will likely need corresponding test changes - ... and 11 more: https://git.openjdk.org/jdk/compare/d5803aa7...08f1c185 ------------- Changes: https://git.openjdk.org/jdk/pull/27743/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=02 Stats: 1307 lines in 16 files changed: 514 ins; 106 del; 687 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision: One more comment fix ------------- Changes: - all: https://git.openjdk.org/jdk/pull/27743/files - new: https://git.openjdk.org/jdk/pull/27743/files/08f1c185..46df71e1 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
On Mon, 10 Nov 2025 17:22:34 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision:
One more comment fix
I've resolved the conflicts now and incorporated reviewers' feedback. Thanks for the reviews! It doesn't solve the larger issue of reference passing for the result value, though :-/ ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3513007587
On Mon, 10 Nov 2025 17:22:34 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request incrementally with one additional commit since the last revision:
One more comment fix
Looks good to me! ------------- Marked as reviewed by cnorrbin (Committer). PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3448050849
On Tue, 11 Nov 2025 13:21:38 GMT, Casper Norrbin <cnorrbin@openjdk.org> wrote:
Looks good to me!
Thanks for the review. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3517235877
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - Better logging for -1 (cpu_shares) - ... and 14 more: https://git.openjdk.org/jdk/compare/29100320...0958b10f ------------- Changes: https://git.openjdk.org/jdk/pull/27743/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=04 Stats: 1308 lines in 16 files changed: 514 ins; 106 del; 688 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
On Tue, 11 Nov 2025 14:32:57 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits:
- Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - Better logging for -1 (cpu_shares) - ... and 14 more: https://git.openjdk.org/jdk/compare/29100320...0958b10f
New version looks good. ------------- Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3464545040
On Tue, 11 Nov 2025 14:32:57 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits:
- Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - Better logging for -1 (cpu_shares) - ... and 14 more: https://git.openjdk.org/jdk/compare/29100320...0958b10f
Thanks for the reviews! If there are no objections to move forward with this I'll integrate Monday. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3532695447
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - ... and 15 more: https://git.openjdk.org/jdk/compare/5d65c23c...9a5f3eb5 ------------- Changes: https://git.openjdk.org/jdk/pull/27743/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=27743&range=05 Stats: 1308 lines in 16 files changed: 514 ins; 106 del; 688 mod Patch: https://git.openjdk.org/jdk/pull/27743.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/27743/head:pull/27743 PR: https://git.openjdk.org/jdk/pull/27743
On Fri, 14 Nov 2025 15:11:02 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - ... and 15 more: https://git.openjdk.org/jdk/compare/5d65c23c...9a5f3eb5
Marked as reviewed by cnorrbin (Committer). ------------- PR Review: https://git.openjdk.org/jdk/pull/27743#pullrequestreview-3465404828
On Fri, 14 Nov 2025 15:11:02 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits:
- Merge branch 'master' into jdk-8365606-jlong-julong-refactor - Add space in trace log - Merge branch 'master' into jdk-8365606-jlong-julong-refactor - One more comment fix - Extract OSContainer::available_swap_in_bytes() - Simplify os::used_memory() - Fix os::active_processor_count() - os::free_memory => use 'value' directly - os::available_memory() => use 'value' directly - Fix pids_max printing in VM.info - ... and 15 more: https://git.openjdk.org/jdk/compare/5d65c23c...9a5f3eb5
OK. Here it goes. ------------- PR Comment: https://git.openjdk.org/jdk/pull/27743#issuecomment-3546491920
On Fri, 10 Oct 2025 13:09:48 GMT, Severin Gehwolf <sgehwolf@openjdk.org> wrote:
Please review this revised version of getting rid of `jlong` and `julong` in internal HotSpot code. The single remaining usage is using `os::elapsed_counter()` which I think is still ok. This refactoring is for the container detection code to (mostly) do away with negative return values.
It gets rid of the trifold-use of return value: 1.) error, 2) unlimited values 3) actual numbers/values/limits. Instead, all container related values are now being read from the interface files as `uint64_t` and afterwards interpreted in the way that make sense for the API implementations. For example, `cpu` values will essentially be treated as `int`s as before, potentially returning a negative value `-1` for unlimited. For memory sizes the type `physical_memory_size_type` has been chosen. When there is no limit for a specific memory size a value `value_unlimited` is being returned.
All error cases have been changed to returning `false` in the API functions (and no value is being set in the passed in reference for the value). The effect of this is that all container related functions now return a `bool` and require a reference to be passed in for the `value` that is being asked for.
All usages of the API have been changed to use the revised API. There is no more usages for `OSCONTAINER_ERROR` (`-2) in HotSpot code.
While working on this, I've noticed that there are still some calls deep in the cgroup subsystem code to query "machine" info (e.g. `os::Linux::active_processor_count()`). I've filed [JDK-8369503](https://bugs.openjdk.org/browse/JDK-8369503) to get this cleaned-up as this patch was already getting large.
Testing (looking good): - [x] GHA - [x] All container tests (including problem listed ones) on Linux x86_64 with cg v1 and cg v2. See [this comment](https://github.com/openjdk/jdk/pull/27743#issuecomment-3390060127) below. - [x] Some ad-hoc manual testing in containers using JFR (`jdk.SwapSpace` event) and `VM.info` diagnostic command.
Thoughts? Opinions?
This pull request has now been integrated. Changeset: 72ebca8a Author: Severin Gehwolf <sgehwolf@openjdk.org> URL: https://git.openjdk.org/jdk/commit/72ebca8a0b19fac8a9483e5a3a98b454176fc342 Stats: 1308 lines in 16 files changed: 514 ins; 106 del; 688 mod 8365606: Container code should not be using jlong/julong Reviewed-by: stuefe, cnorrbin, fitzsim ------------- PR: https://git.openjdk.org/jdk/pull/27743
participants (7)
-
Andrew Haley
-
Casper Norrbin
-
David Holmes
-
Severin Gehwolf
-
Stefan Karlsson
-
Thomas Fitzsimmons
-
Thomas Stuefe