8304824: NMT should not use ThreadCritical

roberttoyonaga · roberttoyonaga · commit 0abfa3ba8f72 · 2024-10-29T07:50:43.000Z
Reviewed-by: stuefe, dholmes, jsjolen
diff --git a/src/hotspot/os/posix/perfMemory_posix.cpp b/src/hotspot/os/posix/perfMemory_posix.cpp
@@ -1086,7 +1086,7 @@ static char* mmap_create_shared(size_t size) {
 static void unmap_shared(char* addr, size_t bytes) {
   int res;
   if (MemTracker::enabled()) {
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     res = ::munmap(addr, bytes);
     if (res == 0) {
       MemTracker::record_virtual_memory_release((address)addr, bytes);
diff --git a/src/hotspot/os/windows/os_windows.cpp b/src/hotspot/os/windows/os_windows.cpp
@@ -3825,7 +3825,8 @@ bool os::pd_release_memory(char* addr, size_t bytes) {
     if (err != nullptr) {
       log_warning(os)("bad release: [" PTR_FORMAT "-" PTR_FORMAT "): %s", p2i(start), p2i(end), err);
 #ifdef ASSERT
-      os::print_memory_mappings((char*)start, bytes, tty);
+      fileStream fs(stdout);
+      os::print_memory_mappings((char*)start, bytes, &fs);
       assert(false, "bad release: [" PTR_FORMAT "-" PTR_FORMAT "): %s", p2i(start), p2i(end), err);
 #endif
       return false;
diff --git a/src/hotspot/os/windows/perfMemory_windows.cpp b/src/hotspot/os/windows/perfMemory_windows.cpp
@@ -1803,7 +1803,7 @@ void PerfMemory::detach(char* addr, size_t bytes) {
 
   if (MemTracker::enabled()) {
     // it does not go through os api, the operation has to record from here
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     remove_file_mapping(addr);
     MemTracker::record_virtual_memory_release((address)addr, bytes);
   } else {
diff --git a/src/hotspot/share/nmt/memBaseline.cpp b/src/hotspot/share/nmt/memBaseline.cpp
@@ -141,7 +141,7 @@ void MemBaseline::baseline_summary() {
   MallocMemorySummary::snapshot(&_malloc_memory_snapshot);
   VirtualMemorySummary::snapshot(&_virtual_memory_snapshot);
   {
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     MemoryFileTracker::Instance::summary_snapshot(&_virtual_memory_snapshot);
   }
 
diff --git a/src/hotspot/share/nmt/memReporter.cpp b/src/hotspot/share/nmt/memReporter.cpp
@@ -465,7 +465,7 @@ void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion*
 void MemDetailReporter::report_memory_file_allocations() {
   stringStream st;
   {
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     MemoryFileTracker::Instance::print_all_reports_on(&st, scale());
   }
   output()->print_raw(st.freeze());
diff --git a/src/hotspot/share/nmt/memTracker.hpp b/src/hotspot/share/nmt/memTracker.hpp
@@ -31,7 +31,6 @@
 #include "nmt/threadStackTracker.hpp"
 #include "nmt/virtualMemoryTracker.hpp"
 #include "runtime/mutexLocker.hpp"
-#include "runtime/threadCritical.hpp"
 #include "utilities/debug.hpp"
 #include "utilities/nativeCallStack.hpp"
 
@@ -125,7 +124,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker ml;
       VirtualMemoryTracker::add_reserved_region((address)addr, size, stack, mem_tag);
     }
   }
@@ -151,7 +150,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker ml;
       VirtualMemoryTracker::add_reserved_region((address)addr, size, stack, mem_tag);
       VirtualMemoryTracker::add_committed_region((address)addr, size, stack);
     }
@@ -162,23 +161,23 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker ml;
       VirtualMemoryTracker::add_committed_region((address)addr, size, stack);
     }
   }
 
   static inline MemoryFileTracker::MemoryFile* register_file(const char* descriptive_name) {
     assert_post_init();
     if (!enabled()) return nullptr;
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     return MemoryFileTracker::Instance::make_file(descriptive_name);
   }
 
   static inline void remove_file(MemoryFileTracker::MemoryFile* file) {
     assert_post_init();
     if (!enabled()) return;
     assert(file != nullptr, "must be");
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     MemoryFileTracker::Instance::free_file(file);
   }
 
@@ -187,7 +186,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     assert(file != nullptr, "must be");
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     MemoryFileTracker::Instance::allocate_memory(file, offset, size, stack, mem_tag);
   }
 
@@ -196,7 +195,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     assert(file != nullptr, "must be");
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker ml;
     MemoryFileTracker::Instance::free_memory(file, offset, size);
   }
 
@@ -210,7 +209,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker ml;
       VirtualMemoryTracker::split_reserved_region((address)addr, size, split, mem_tag, split_tag);
     }
   }
@@ -219,7 +218,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker ml;
       VirtualMemoryTracker::set_reserved_region_type((address)addr, mem_tag);
     }
   }
diff --git a/src/hotspot/share/nmt/memoryFileTracker.cpp b/src/hotspot/share/nmt/memoryFileTracker.cpp
@@ -29,13 +29,11 @@
 #include "nmt/nmtCommon.hpp"
 #include "nmt/nmtNativeCallStackStorage.hpp"
 #include "nmt/vmatree.hpp"
-#include "runtime/mutex.hpp"
 #include "utilities/growableArray.hpp"
 #include "utilities/nativeCallStack.hpp"
 #include "utilities/ostream.hpp"
 
 MemoryFileTracker* MemoryFileTracker::Instance::_tracker = nullptr;
-PlatformMutex* MemoryFileTracker::Instance::_mutex = nullptr;
 
 MemoryFileTracker::MemoryFileTracker(bool is_detailed_mode)
   : _stack_storage(is_detailed_mode), _files() {}
@@ -132,7 +130,6 @@ bool MemoryFileTracker::Instance::initialize(NMT_TrackingLevel tracking_level) {
   _tracker = static_cast<MemoryFileTracker*>(os::malloc(sizeof(MemoryFileTracker), mtNMT));
   if (_tracker == nullptr) return false;
   new (_tracker) MemoryFileTracker(tracking_level == NMT_TrackingLevel::NMT_detail);
-  _mutex = new PlatformMutex();
   return true;
 }
 
@@ -193,11 +190,3 @@ void MemoryFileTracker::summary_snapshot(VirtualMemorySnapshot* snapshot) const
 void MemoryFileTracker::Instance::summary_snapshot(VirtualMemorySnapshot* snapshot) {
   _tracker->summary_snapshot(snapshot);
 }
-
-MemoryFileTracker::Instance::Locker::Locker() {
-  MemoryFileTracker::Instance::_mutex->lock();
-}
-
-MemoryFileTracker::Instance::Locker::~Locker() {
-  MemoryFileTracker::Instance::_mutex->unlock();
-}
diff --git a/src/hotspot/share/nmt/memoryFileTracker.hpp b/src/hotspot/share/nmt/memoryFileTracker.hpp
@@ -30,7 +30,6 @@
 #include "nmt/nmtNativeCallStackStorage.hpp"
 #include "nmt/virtualMemoryTracker.hpp"
 #include "nmt/vmatree.hpp"
-#include "runtime/mutex.hpp"
 #include "runtime/os.inline.hpp"
 #include "utilities/growableArray.hpp"
 #include "utilities/nativeCallStack.hpp"
@@ -81,14 +80,8 @@ class MemoryFileTracker {
 
   class Instance : public AllStatic {
     static MemoryFileTracker* _tracker;
-    static PlatformMutex* _mutex;
 
   public:
-    class Locker : public StackObj {
-    public:
-      Locker();
-      ~Locker();
-    };
 
     static bool initialize(NMT_TrackingLevel tracking_level);
 
diff --git a/src/hotspot/share/nmt/nmtCommon.hpp b/src/hotspot/share/nmt/nmtCommon.hpp
@@ -29,6 +29,7 @@
 
 #include "memory/allStatic.hpp"
 #include "nmt/memTag.hpp"
+#include "runtime/mutexLocker.hpp"
 #include "utilities/align.hpp"
 #include "utilities/globalDefinitions.hpp"
 
@@ -137,5 +138,13 @@ class NMTUtil : AllStatic {
   static S _strings[mt_number_of_tags];
 };
 
+// Same as MutexLocker but can be used during VM init.
+// Performs no action if given a null mutex or with detached threads.
+class NmtVirtualMemoryLocker: public ConditionalMutexLocker {
+public:
+    NmtVirtualMemoryLocker() :
+            ConditionalMutexLocker(NmtVirtualMemory_lock, Thread::current_or_null_safe() != nullptr, Mutex::_no_safepoint_check_flag) {
+    }
+};
 
 #endif // SHARE_NMT_NMTCOMMON_HPP
diff --git a/src/hotspot/share/nmt/threadStackTracker.cpp b/src/hotspot/share/nmt/threadStackTracker.cpp
@@ -29,7 +29,6 @@
 #include "nmt/threadStackTracker.hpp"
 #include "nmt/virtualMemoryTracker.hpp"
 #include "runtime/os.hpp"
-#include "runtime/threadCritical.hpp"
 #include "utilities/align.hpp"
 #include "utilities/debug.hpp"
 #include "utilities/globalDefinitions.hpp"
@@ -53,7 +52,7 @@ void ThreadStackTracker::new_thread_stack(void* base, size_t size, const NativeC
   assert(base != nullptr, "Should have been filtered");
   align_thread_stack_boundaries_inward(base, size);
 
-  ThreadCritical tc;
+  NmtVirtualMemoryLocker ml;
   VirtualMemoryTracker::add_reserved_region((address)base, size, stack, mtThreadStack);
   _thread_count++;
 }
@@ -63,7 +62,7 @@ void ThreadStackTracker::delete_thread_stack(void* base, size_t size) {
   assert(base != nullptr, "Should have been filtered");
   align_thread_stack_boundaries_inward(base, size);
 
-  ThreadCritical tc;
+  NmtVirtualMemoryLocker ml;
   VirtualMemoryTracker::remove_released_region((address)base, size);
   _thread_count--;
 }
diff --git a/src/hotspot/share/nmt/virtualMemoryTracker.cpp b/src/hotspot/share/nmt/virtualMemoryTracker.cpp
@@ -30,7 +30,6 @@
 #include "nmt/threadStackTracker.hpp"
 #include "nmt/virtualMemoryTracker.hpp"
 #include "runtime/os.hpp"
-#include "runtime/threadCritical.hpp"
 #include "utilities/ostream.hpp"
 
 VirtualMemorySnapshot VirtualMemorySummary::_snapshot;
@@ -621,6 +620,7 @@ class SnapshotThreadStackWalker : public VirtualMemoryWalker {
   SnapshotThreadStackWalker() {}
 
   bool do_allocation_site(const ReservedMemoryRegion* rgn) {
+    assert_lock_strong(NmtVirtualMemory_lock);
     if (rgn->mem_tag() == mtThreadStack) {
       address stack_bottom = rgn->thread_stack_uncommitted_bottom();
       address committed_start;
@@ -661,7 +661,7 @@ void VirtualMemoryTracker::snapshot_thread_stacks() {
 
 bool VirtualMemoryTracker::walk_virtual_memory(VirtualMemoryWalker* walker) {
   assert(_reserved_regions != nullptr, "Sanity check");
-  ThreadCritical tc;
+  NmtVirtualMemoryLocker ml;
   // Check that the _reserved_regions haven't been deleted.
   if (_reserved_regions != nullptr) {
     LinkedListNode<ReservedMemoryRegion>* head = _reserved_regions->head();
diff --git a/src/hotspot/share/runtime/mutexLocker.cpp b/src/hotspot/share/runtime/mutexLocker.cpp
@@ -135,6 +135,7 @@ Mutex*   SharedDecoder_lock           = nullptr;
 Mutex*   DCmdFactory_lock             = nullptr;
 Mutex*   NMTQuery_lock                = nullptr;
 Mutex*   NMTCompilationCostHistory_lock = nullptr;
+Mutex*   NmtVirtualMemory_lock          = nullptr;
 
 #if INCLUDE_CDS
 #if INCLUDE_JVMTI
@@ -293,10 +294,11 @@ void mutex_init() {
   MUTEX_DEFN(CodeHeapStateAnalytics_lock     , PaddedMutex  , safepoint);
   MUTEX_DEFN(ThreadsSMRDelete_lock           , PaddedMonitor, service-2); // Holds ConcurrentHashTableResize_lock
   MUTEX_DEFN(ThreadIdTableCreate_lock        , PaddedMutex  , safepoint);
-  MUTEX_DEFN(SharedDecoder_lock              , PaddedMutex  , tty-1);
+  MUTEX_DEFN(SharedDecoder_lock              , PaddedMutex  , service-5); // Must be lower than NmtVirtualMemory_lock due to MemTracker::print_containing_region
   MUTEX_DEFN(DCmdFactory_lock                , PaddedMutex  , nosafepoint);
   MUTEX_DEFN(NMTQuery_lock                   , PaddedMutex  , safepoint);
   MUTEX_DEFN(NMTCompilationCostHistory_lock  , PaddedMutex  , nosafepoint);
+  MUTEX_DEFN(NmtVirtualMemory_lock           , PaddedMutex  , service-4);
 #if INCLUDE_CDS
 #if INCLUDE_JVMTI
   MUTEX_DEFN(CDSClassFileStream_lock         , PaddedMutex  , safepoint);
diff --git a/src/hotspot/share/runtime/mutexLocker.hpp b/src/hotspot/share/runtime/mutexLocker.hpp
@@ -28,6 +28,7 @@
 #include "memory/allocation.hpp"
 #include "runtime/flags/flagSetting.hpp"
 #include "runtime/mutex.hpp"
+#include "runtime/thread.hpp"
 
 // Mutexes used in the VM.
 
@@ -115,6 +116,7 @@ extern Mutex*   SharedDecoder_lock;              // serializes access to the dec
 extern Mutex*   DCmdFactory_lock;                // serialize access to DCmdFactory information
 extern Mutex*   NMTQuery_lock;                   // serialize NMT Dcmd queries
 extern Mutex*   NMTCompilationCostHistory_lock;  // guards NMT compilation cost history
+extern Mutex*   NmtVirtualMemory_lock;           // guards NMT virtual memory updates
 #if INCLUDE_CDS
 #if INCLUDE_JVMTI
 extern Mutex*   CDSClassFileStream_lock;         // FileMapInfo::open_stream_for_jvmti
diff --git a/src/hotspot/share/runtime/os.cpp b/src/hotspot/share/runtime/os.cpp
@@ -2166,7 +2166,7 @@ bool os::uncommit_memory(char* addr, size_t bytes, bool executable) {
   assert_nonempty_range(addr, bytes);
   bool res;
   if (MemTracker::enabled()) {
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     res = pd_uncommit_memory(addr, bytes, executable);
     if (res) {
       MemTracker::record_virtual_memory_uncommit((address)addr, bytes);
@@ -2188,7 +2188,7 @@ bool os::release_memory(char* addr, size_t bytes) {
   assert_nonempty_range(addr, bytes);
   bool res;
   if (MemTracker::enabled()) {
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     res = pd_release_memory(addr, bytes);
     if (res) {
       MemTracker::record_virtual_memory_release((address)addr, bytes);
@@ -2273,7 +2273,7 @@ char* os::map_memory(int fd, const char* file_name, size_t file_offset,
 bool os::unmap_memory(char *addr, size_t bytes) {
   bool result;
   if (MemTracker::enabled()) {
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     result = pd_unmap_memory(addr, bytes);
     if (result) {
       MemTracker::record_virtual_memory_release((address)addr, bytes);
@@ -2312,7 +2312,7 @@ char* os::reserve_memory_special(size_t size, size_t alignment, size_t page_size
 bool os::release_memory_special(char* addr, size_t bytes) {
   bool res;
   if (MemTracker::enabled()) {
-    ThreadCritical tc;
+    NmtVirtualMemoryLocker ml;
     res = pd_release_memory_special(addr, bytes);
     if (res) {
       MemTracker::record_virtual_memory_release((address)addr, bytes);
diff --git a/src/hotspot/share/utilities/vmError.cpp b/src/hotspot/share/utilities/vmError.cpp
@@ -717,6 +717,10 @@ void VMError::report(outputStream* st, bool _verbose) {
   address lastpc = nullptr;
 
   BEGIN
+  if (MemTracker::enabled() && NmtVirtualMemory_lock != nullptr && NmtVirtualMemory_lock->owned_by_self()) {
+    // Manually unlock to avoid reentrancy due to mallocs in detailed mode.
+    NmtVirtualMemory_lock->unlock();
+  }
 
   STEP("printing fatal error message")
     st->print_cr("#");

Original file line number	Diff line number	Diff line change
`@@ -141,7 +141,7 @@ void MemBaseline::baseline_summary() {`
`141`	`141`	`MallocMemorySummary::snapshot(&_malloc_memory_snapshot);`
`142`	`142`	`VirtualMemorySummary::snapshot(&_virtual_memory_snapshot);`
`143`	`143`	`{`
`144`		`- MemoryFileTracker::Instance::Locker lock;`
	`144`	`+ NmtVirtualMemoryLocker ml;`
`145`	`145`	`MemoryFileTracker::Instance::summary_snapshot(&_virtual_memory_snapshot);`
`146`	`146`	`}`
`147`	`147`
Original file line number	Diff line number	Diff line change
`@@ -465,7 +465,7 @@ void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion*`
`465`	`465`	`void MemDetailReporter::report_memory_file_allocations() {`
`466`	`466`	`stringStream st;`
`467`	`467`	`{`
`468`		`- MemoryFileTracker::Instance::Locker lock;`
	`468`	`+ NmtVirtualMemoryLocker ml;`
`469`	`469`	`MemoryFileTracker::Instance::print_all_reports_on(&st, scale());`
`470`	`470`	`}`
`471`	`471`	`output()->print_raw(st.freeze());`