From 0abfa3ba8f72538f62be838c1ebac8cfbdd14cdf Mon Sep 17 00:00:00 2001 From: Robert Toyonaga Date: Tue, 29 Oct 2024 07:50:43 +0000 Subject: [PATCH] 8304824: NMT should not use ThreadCritical Reviewed-by: stuefe, dholmes, jsjolen --- src/hotspot/os/posix/perfMemory_posix.cpp | 2 +- src/hotspot/os/windows/os_windows.cpp | 3 ++- src/hotspot/os/windows/perfMemory_windows.cpp | 2 +- src/hotspot/share/nmt/memBaseline.cpp | 2 +- src/hotspot/share/nmt/memReporter.cpp | 2 +- src/hotspot/share/nmt/memTracker.hpp | 19 +++++++++---------- src/hotspot/share/nmt/memoryFileTracker.cpp | 11 ----------- src/hotspot/share/nmt/memoryFileTracker.hpp | 7 ------- src/hotspot/share/nmt/nmtCommon.hpp | 9 +++++++++ src/hotspot/share/nmt/threadStackTracker.cpp | 5 ++--- .../share/nmt/virtualMemoryTracker.cpp | 4 ++-- src/hotspot/share/runtime/mutexLocker.cpp | 4 +++- src/hotspot/share/runtime/mutexLocker.hpp | 2 ++ src/hotspot/share/runtime/os.cpp | 8 ++++---- src/hotspot/share/utilities/vmError.cpp | 4 ++++ 15 files changed, 41 insertions(+), 43 deletions(-) diff --git a/src/hotspot/os/posix/perfMemory_posix.cpp b/src/hotspot/os/posix/perfMemory_posix.cpp index 4eb46169878..17bf63092c2 100644 --- 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 index 4dafef0c098..71efb57e0f2 100644 --- 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 index 06b057315cb..959be982fab 100644 --- 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 index 6f82b2de9f1..270601e9c05 100644 --- 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 index 6ce6206ebcc..15767da276c 100644 --- 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 index 6ba1db2e7ff..92640430e1c 100644 --- 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,7 +161,7 @@ class MemTracker : AllStatic { assert_post_init(); if (!enabled()) return; if (addr != nullptr) { - ThreadCritical tc; + NmtVirtualMemoryLocker ml; VirtualMemoryTracker::add_committed_region((address)addr, size, stack); } } @@ -170,7 +169,7 @@ class MemTracker : AllStatic { 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); } @@ -178,7 +177,7 @@ class MemTracker : AllStatic { 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 index ede483ed337..ab4f8b6d1f3 100644 --- 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(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 index 42902701a16..911f10baf98 100644 --- 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 @@ public: 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 index 3f72960f21f..4422d340634 100644 --- 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 index 6f112fa8fc5..42af67d6464 100644 --- 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 index d298381f103..8b0f2f4d7a4 100644 --- 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 @@ public: 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* head = _reserved_regions->head(); diff --git a/src/hotspot/share/runtime/mutexLocker.cpp b/src/hotspot/share/runtime/mutexLocker.cpp index a0a6e5626e4..76d0674e8c6 100644 --- 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 index 932ca5fce82..07bea6b614d 100644 --- 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 index 5dc5a1af8cc..2395510f27f 100644 --- 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 index 7bbb978b806..eec612b1dd8 100644 --- 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("#");