8346123: [REDO] NMT should not use ThreadCritical

Reviewed-by: dholmes, coleenp, stuefe

Author: roberttoyonaga

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();