On Wed, 25 Jan 2023 11:24:55 GMT, Severin Gehwolf <sgehw...@openjdk.org> wrote:
>> Please review this fix to >> [JDK-8292541](https://bugs.openjdk.org/browse/JDK-8292541) which adds the >> same handling for swap values exceeding what's possible in the non-container >> case. I.e. treats it as unlimited and fixes the reporting. This has been >> handled on the hotspot side similarly with >> [JDK-8292083](https://bugs.openjdk.org/browse/JDK-8292083). >> >> Testing: >> - [x] Container tests on linux x86_64 with cg v1 >> - [x] Container tests on linux x86_64 with cg v2 >> - [ ] GHA (seem to have had some infra issues, so re-running) > > Severin Gehwolf has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains one commit: > > 8299858: [Metrics] Swap memory limit reported incorrectly when too large Mostly good, small nits. src/java.base/linux/native/libjava/CgroupMetrics.c line 44: > 42: { > 43: jlong pages = sysconf(_SC_PHYS_PAGES); > 44: jlong page_size = sysconf(_SC_PAGESIZE); Preexisting: Check or at least assert for error? sysconf could also signal "indeterminable" state beside an error. I cannot see how either one could happen with _SC_PHYS_PAGES or _SC_PAGESIZE, so an assert would probably be enough. (I know you do an assert in java, but that has to be switched on explicitly - do we run tests with asserts enabled?) src/java.base/linux/native/libjava/CgroupMetrics.c line 45: > 43: jlong pages = sysconf(_SC_PHYS_PAGES); > 44: jlong page_size = sysconf(_SC_PAGESIZE); > 45: return pages * page_size; Preexisting, and theoretical nitpickery: I got curious when I saw this. What does sysconf return if a 32-bit VM runs on a large machine with >8Tb main memory and 4k pages? In that case, _SC_PHYS_PAGES would overflow the 32-bit long return type of sysconf. src/java.base/linux/native/libjava/CgroupMetrics.c line 54: > 52: struct sysinfo si; > 53: sysinfo(&si); > 54: return (jlong)si.totalswap; If sysinfo fails, we return a random value. ------------- PR: https://git.openjdk.org/jdk/pull/12118
