8346123: [REDO] NMT should not use ThreadCritical

Reviewed-by: dholmes, coleenp, stuefe

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;
+    MemTracker::NmtVirtualMemoryLocker nvml;
     res = ::munmap(addr, bytes);
     if (res == 0) {
       MemTracker::record_virtual_memory_release((address)addr, bytes);

src/hotspot/os/windows/os_windows.cpp

@@ -3639,10 +3639,7 @@ bool os::pd_release_memory(char* addr, size_t bytes) {
     // Handle mapping error. We assert in debug, unconditionally print a warning in release.
     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);
       assert(false, "bad release: [" PTR_FORMAT "-" PTR_FORMAT "): %s", p2i(start), p2i(end), err);
-#endif
       return false;
     }
     // Free this range

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;
+    MemTracker::NmtVirtualMemoryLocker nvml;
     remove_file_mapping(addr);
     MemTracker::record_virtual_memory_release((address)addr, bytes);
   } else {

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;
+    MemTracker::NmtVirtualMemoryLocker nvml;
     MemoryFileTracker::Instance::summary_snapshot(&_virtual_memory_snapshot);
   }
 

src/hotspot/share/nmt/memReporter.cpp

@@ -28,6 +28,7 @@
 #include "nmt/mallocTracker.hpp"
 #include "nmt/memTag.hpp"
 #include "nmt/memReporter.hpp"
+#include "nmt/memTracker.hpp"
 #include "nmt/memoryFileTracker.hpp"
 #include "nmt/threadStackTracker.hpp"
 #include "nmt/virtualMemoryTracker.hpp"
@@ -465,7 +466,7 @@ void MemDetailReporter::report_virtual_memory_region(const ReservedMemoryRegion*
 void MemDetailReporter::report_memory_file_allocations() {
   stringStream st;
   {
-    MemoryFileTracker::Instance::Locker lock;
+    MemTracker::NmtVirtualMemoryLocker nvml;
     MemoryFileTracker::Instance::print_all_reports_on(&st, scale());
   }
   output()->print_raw(st.freeze());

src/hotspot/share/nmt/memTracker.cpp

@@ -52,6 +52,8 @@ NMT_TrackingLevel MemTracker::_tracking_level = NMT_unknown;
 
 MemBaseline MemTracker::_baseline;
 
+bool MemTracker::NmtVirtualMemoryLocker::_safe_to_use;
+
 void MemTracker::initialize() {
   bool rc = true;
   assert(_tracking_level == NMT_unknown, "only call once");

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"
 
@@ -62,6 +61,12 @@ class MemTracker : AllStatic {
     return _tracking_level != NMT_unknown;
   }
 
+  // This may be called on a detached thread during VM init, so we should check that first.
+  static inline void assert_locked() {
+    assert(!NmtVirtualMemoryLocker::is_safe_to_use() || NmtVirtualMemory_lock->owned_by_self(),
+           "should have acquired NmtVirtualMemory_lock");
+  }
+
   static inline NMT_TrackingLevel tracking_level() {
     return _tracking_level;
   }
@@ -125,7 +130,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker nvml;
       VirtualMemoryTracker::add_reserved_region((address)addr, size, stack, mem_tag);
     }
   }
@@ -151,7 +156,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker nvml;
       VirtualMemoryTracker::add_reserved_region((address)addr, size, stack, mem_tag);
       VirtualMemoryTracker::add_committed_region((address)addr, size, stack);
     }
@@ -162,23 +167,23 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker nvml;
       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 nvml;
     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 nvml;
     MemoryFileTracker::Instance::free_file(file);
   }
 
@@ -187,7 +192,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     assert(file != nullptr, "must be");
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker nvml;
     MemoryFileTracker::Instance::allocate_memory(file, offset, size, stack, mem_tag);
   }
 
@@ -196,7 +201,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     assert(file != nullptr, "must be");
-    MemoryFileTracker::Instance::Locker lock;
+    NmtVirtualMemoryLocker nvml;
     MemoryFileTracker::Instance::free_memory(file, offset, size);
   }
 
@@ -210,7 +215,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker nvml;
       VirtualMemoryTracker::split_reserved_region((address)addr, size, split, mem_tag, split_tag);
     }
   }
@@ -219,7 +224,7 @@ class MemTracker : AllStatic {
     assert_post_init();
     if (!enabled()) return;
     if (addr != nullptr) {
-      ThreadCritical tc;
+      NmtVirtualMemoryLocker nvml;
       VirtualMemoryTracker::set_reserved_region_type((address)addr, mem_tag);
     }
   }
@@ -269,6 +274,39 @@ class MemTracker : AllStatic {
   // and return true; false if not found.
   static bool print_containing_region(const void* p, outputStream* out);
 
+  /*
+   * NmtVirtualMemoryLocker is similar to MutexLocker but can be used during VM init before mutexes are ready or
+   * current thread has been assigned. Performs no action during VM init.
+   *
+   * Unlike malloc, NMT requires locking for virtual memory operations. This is because it must synchronize the usage
+   * of global data structures used for modelling the effect of virtual memory operations.
+   * It is important that locking is used such that the actual OS memory operations (mmap) are done atomically with the
+   * corresponding NMT accounting (updating the internal model). Currently, this is not the case in all situations
+   * (see JDK-8341491), but this should be changed in the future.
+   *
+   * An issue with using Mutex is that NMT is used early during VM initialization before mutexes are initialized
+   * and current thread is attached. Mutexes do not work under those conditions, so we must use a flag to avoid
+   * attempting to lock until initialization is finished. Lack of synchronization here should not be a problem since it
+   * is single threaded at that point in time anyway.
+   */
+  class NmtVirtualMemoryLocker: StackObj {
+    // Returns true if it is safe to start using this locker.
+    static bool _safe_to_use;
+    ConditionalMutexLocker _cml;
+
+  public:
+    NmtVirtualMemoryLocker(): _cml(NmtVirtualMemory_lock, _safe_to_use, Mutex::_no_safepoint_check_flag){}
+
+    static inline bool is_safe_to_use()  {
+      return _safe_to_use;
+    }
+
+    // Set in Threads::create_vm once threads and mutexes have been initialized.
+    static inline void set_safe_to_use()  {
+      _safe_to_use = true;
+    }
+  };
+
  private:
   static void report(bool summary_only, outputStream* output, size_t scale);
 
@@ -277,8 +315,6 @@ class MemTracker : AllStatic {
   static NMT_TrackingLevel   _tracking_level;
   // Stored baseline
   static MemBaseline      _baseline;
-  // Query lock
-  static Mutex*           _query_lock;
 };
 
 #endif // SHARE_NMT_MEMTRACKER_HPP

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;
 }
 
@@ -189,11 +186,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();
-}

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"
@@ -93,14 +92,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);
 

src/hotspot/share/nmt/nmtUsage.cpp

@@ -25,6 +25,7 @@
 #include "precompiled.hpp"
 #include "nmt/mallocTracker.hpp"
 #include "nmt/memoryFileTracker.hpp"
+#include "nmt/memTracker.hpp"
 #include "nmt/nmtCommon.hpp"
 #include "nmt/nmtUsage.hpp"
 #include "nmt/threadStackTracker.hpp"
@@ -94,7 +95,7 @@ void NMTUsage::update_vm_usage() {
 
   { // MemoryFileTracker addition
     using MFT = MemoryFileTracker::Instance;
-    MFT::Locker lock;
+    MemTracker::NmtVirtualMemoryLocker nvml;
     MFT::iterate_summary([&](MemTag tag, const VirtualMemory* vm) {
       int i = NMTUtil::tag_to_index(tag);
       _vm_by_type[i].committed += vm->committed();

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;
+  MemTracker::NmtVirtualMemoryLocker nvml;
   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;
+  MemTracker::NmtVirtualMemoryLocker nvml;
   VirtualMemoryTracker::remove_released_region((address)base, size);
   _thread_count--;
 }

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;
@@ -338,6 +337,8 @@ bool VirtualMemoryTracker::add_reserved_region(address base_addr, size_t size,
   assert(base_addr != nullptr, "Invalid address");
   assert(size > 0, "Invalid size");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
+
   ReservedMemoryRegion  rgn(base_addr, size, stack, mem_tag);
   ReservedMemoryRegion* reserved_rgn = _reserved_regions->find(rgn);
 
@@ -416,6 +417,7 @@ bool VirtualMemoryTracker::add_reserved_region(address base_addr, size_t size,
 void VirtualMemoryTracker::set_reserved_region_type(address addr, MemTag mem_tag) {
   assert(addr != nullptr, "Invalid address");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
 
   ReservedMemoryRegion   rgn(addr, 1);
   ReservedMemoryRegion*  reserved_rgn = _reserved_regions->find(rgn);
@@ -434,6 +436,7 @@ bool VirtualMemoryTracker::add_committed_region(address addr, size_t size,
   assert(addr != nullptr, "Invalid address");
   assert(size > 0, "Invalid size");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
 
   ReservedMemoryRegion  rgn(addr, size);
   ReservedMemoryRegion* reserved_rgn = _reserved_regions->find(rgn);
@@ -454,6 +457,7 @@ bool VirtualMemoryTracker::remove_uncommitted_region(address addr, size_t size)
   assert(addr != nullptr, "Invalid address");
   assert(size > 0, "Invalid size");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
 
   ReservedMemoryRegion  rgn(addr, size);
   ReservedMemoryRegion* reserved_rgn = _reserved_regions->find(rgn);
@@ -469,6 +473,7 @@ bool VirtualMemoryTracker::remove_uncommitted_region(address addr, size_t size)
 bool VirtualMemoryTracker::remove_released_region(ReservedMemoryRegion* rgn) {
   assert(rgn != nullptr, "Sanity check");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
 
   // uncommit regions within the released region
   ReservedMemoryRegion backup(*rgn);
@@ -490,6 +495,7 @@ bool VirtualMemoryTracker::remove_released_region(address addr, size_t size) {
   assert(addr != nullptr, "Invalid address");
   assert(size > 0, "Invalid size");
   assert(_reserved_regions != nullptr, "Sanity check");
+  MemTracker::assert_locked();
 
   ReservedMemoryRegion  rgn(addr, size);
   ReservedMemoryRegion* reserved_rgn = _reserved_regions->find(rgn);
@@ -621,6 +627,9 @@ class SnapshotThreadStackWalker : public VirtualMemoryWalker {
   SnapshotThreadStackWalker() {}
 
   bool do_allocation_site(const ReservedMemoryRegion* rgn) {
+    if (MemTracker::NmtVirtualMemoryLocker::is_safe_to_use()) {
+      assert_lock_strong(NmtVirtualMemory_lock);
+    }
     if (rgn->mem_tag() == mtThreadStack) {
       address stack_bottom = rgn->thread_stack_uncommitted_bottom();
       address committed_start;
@@ -661,7 +670,7 @@ void VirtualMemoryTracker::snapshot_thread_stacks() {
 
 bool VirtualMemoryTracker::walk_virtual_memory(VirtualMemoryWalker* walker) {
   assert(_reserved_regions != nullptr, "Sanity check");
-  ThreadCritical tc;
+  MemTracker::NmtVirtualMemoryLocker nvml;
   // Check that the _reserved_regions haven't been deleted.
   if (_reserved_regions != nullptr) {
     LinkedListNode<ReservedMemoryRegion>* head = _reserved_regions->head();