8306841: Generational ZGC: NMT reports Java heap size larger than max heap size

Reviewed-by: eosterlund, stuefe

Author: stefank

src/hotspot/share/gc/z/zPhysicalMemory.cpp

@@ -276,6 +276,13 @@ void ZPhysicalMemoryManager::try_enable_uncommit(size_t min_capacity, size_t max
 }
 
 void ZPhysicalMemoryManager::nmt_commit(zoffset offset, size_t size) const {
+  // NMT expects a 1-to-1 mapping between virtual and physical memory.
+  // ZGC can temporarily have multiple virtual addresses pointing to
+  // the same physical memory.
+  //
+  // When this function is called we don't know where in the virtual memory
+  // this physical memory will be mapped. So we fake that the virtual memory
+  // address is the heap base + the given offset.
   const zaddress addr = ZOffset::address(offset);
   MemTracker::record_virtual_memory_commit((void*)untype(addr), size, CALLER_PC);
 }
@@ -320,6 +327,11 @@ bool ZPhysicalMemoryManager::commit(ZPhysicalMemory& pmem) {
 
     // Commit segment
     const size_t committed = _backing.commit(segment.start(), segment.size());
+
+    // Register with NMT
+    nmt_commit(segment.start(), committed);
+
+    // Register committed segment
     if (!pmem.commit_segment(i, committed)) {
       // Failed or partially failed
       return false;
@@ -341,6 +353,11 @@ bool ZPhysicalMemoryManager::uncommit(ZPhysicalMemory& pmem) {
 
     // Uncommit segment
     const size_t uncommitted = _backing.uncommit(segment.start(), segment.size());
+
+    // Unregister with NMT
+    nmt_uncommit(segment.start(), uncommitted);
+
+    // Deregister uncommitted segment
     if (!pmem.uncommit_segment(i, uncommitted)) {
       // Failed or partially failed
       return false;
@@ -351,12 +368,16 @@ bool ZPhysicalMemoryManager::uncommit(ZPhysicalMemory& pmem) {
   return true;
 }
 
-void ZPhysicalMemoryManager::pretouch_view(zaddress addr, size_t size) const {
+void ZPhysicalMemoryManager::pretouch(zoffset offset, size_t size) const {
+  const uintptr_t addr = untype(ZOffset::address(offset));
   const size_t page_size = ZLargePages::is_explicit() ? ZGranuleSize : os::vm_page_size();
-  os::pretouch_memory((void*)untype(addr), (void*)(untype(addr) + size), page_size);
+  os::pretouch_memory((void*)addr, (void*)(addr + size), page_size);
 }
 
-void ZPhysicalMemoryManager::map_view(zaddress_unsafe addr, const ZPhysicalMemory& pmem) const {
+// Map virtual memory to physcial memory
+void ZPhysicalMemoryManager::map(zoffset offset, const ZPhysicalMemory& pmem) const {
+  const zaddress_unsafe addr = ZOffset::address_unsafe(offset);
+
   size_t size = 0;
 
   // Map segments
@@ -375,27 +396,9 @@ void ZPhysicalMemoryManager::map_view(zaddress_unsafe addr, const ZPhysicalMemor
   }
 }
 
-void ZPhysicalMemoryManager::unmap_view(zaddress_unsafe addr, size_t size) const {
-  _backing.unmap(addr, size);
-}
-
-void ZPhysicalMemoryManager::pretouch(zoffset offset, size_t size) const {
-  // Pre-touch all views
-  pretouch_view(ZOffset::address(offset), size);
-}
-
-void ZPhysicalMemoryManager::map(zoffset offset, const ZPhysicalMemory& pmem) const {
-  const size_t size = pmem.size();
-
-  // Map all views
-  map_view(ZOffset::address_unsafe(offset), pmem);
-
-  nmt_commit(offset, size);
-}
-
+// Unmap virtual memory from physical memory
 void ZPhysicalMemoryManager::unmap(zoffset offset, size_t size) const {
-  nmt_uncommit(offset, size);
+  const zaddress_unsafe addr = ZOffset::address_unsafe(offset);
 
-  // Unmap all views
-  unmap_view(ZOffset::address_unsafe(offset), size);
+  _backing.unmap(addr, size);
 }

test/hotspot/jtreg/runtime/NMT/NMTJavaHeapTest.java

@@ -0,0 +1,83 @@
+/*
+ * Copyright (c) 2023, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8306841
+ * @summary Sanity check Java Heap size values
+ * @modules java.base/jdk.internal.misc
+ * @library /test/lib
+ * @run driver NMTJavaHeapTest
+ */
+
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.Asserts;
+import jdk.test.lib.Utils;
+import jdk.test.lib.process.OutputAnalyzer;
+
+public class NMTJavaHeapTest {
+    public static void main(String args[]) throws Exception {
+        ProcessBuilder pb = ProcessTools.createTestJvm(
+              "-XX:+UnlockDiagnosticVMOptions",
+              "-XX:+PrintNMTStatistics",
+              "-XX:NativeMemoryTracking=summary",
+              "-version");
+
+        OutputAnalyzer output = new OutputAnalyzer(pb.start());
+
+        // Java Heap (reserved=786432KB, committed=49152KB)
+        String pattern = ".*Java Heap \\(reserved=.*, committed=(.*)\\).*";
+        String committed = output.firstMatch(pattern, 1);
+        Asserts.assertNotNull(committed, "Couldn't find pattern '" + pattern
+                + "': in output '" + output.getOutput() + "'");
+
+        long committedBytes = committedStringToBytes(committed);
+
+        // Must be more than zero
+        Asserts.assertGT(committedBytes, 0L);
+
+        // Compare against the max heap size
+        long maxBytes = Runtime.getRuntime().maxMemory();
+        Asserts.assertLTE(committedBytes, maxBytes);
+    }
+
+    private static long K = 1024;
+    private static long M = K * 1024;
+    private static long G = M * 1024;
+
+    private static long committedStringToBytes(String committed) {
+        long multiplier = 1;
+        if (committed.endsWith("GB")) {
+            multiplier = G;
+            committed = committed.replace("GB", "");
+        } else if (committed.endsWith("MB")) {
+            multiplier = M;
+            committed = committed.replace("MB", "");
+        } else if (committed.endsWith("KB")) {
+            multiplier = K;
+            committed = committed.replace("KB", "");
+        }
+
+        return Long.parseLong(committed) * multiplier;
+    }
+}