From 0ff67000dc22e8c58e8a9ed1379010f4a2ad7666 Mon Sep 17 00:00:00 2001 From: Robbin Ehn Date: Sat, 18 Jan 2025 08:41:00 +0000 Subject: [PATCH 1/4] 8347987: Bad ifdef in 8330851 Reviewed-by: stefank, mdoerr, syan, amitkumar --- src/hotspot/share/opto/runtime.cpp | 6 +++--- src/hotspot/share/opto/runtime.hpp | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/hotspot/share/opto/runtime.cpp b/src/hotspot/share/opto/runtime.cpp index 012a06f42f4..be9989bb868 100644 --- a/src/hotspot/share/opto/runtime.cpp +++ b/src/hotspot/share/opto/runtime.cpp @@ -250,10 +250,10 @@ const TypeFunc* OptoRuntime::_updateBytesCRC32C_Type = nullptr; const TypeFunc* OptoRuntime::_updateBytesAdler32_Type = nullptr; const TypeFunc* OptoRuntime::_osr_end_Type = nullptr; const TypeFunc* OptoRuntime::_register_finalizer_Type = nullptr; -#ifdef INCLUDE_JFR +#if INCLUDE_JFR const TypeFunc* OptoRuntime::_class_id_load_barrier_Type = nullptr; #endif // INCLUDE_JFR -#ifdef INCLUDE_JVMTI +#if INCLUDE_JVMTI const TypeFunc* OptoRuntime::_notify_jvmti_vthread_Type = nullptr; #endif // INCLUDE_JVMTI const TypeFunc* OptoRuntime::_dtrace_method_entry_exit_Type = nullptr; @@ -2002,7 +2002,7 @@ void OptoRuntime::initialize_types() { JFR_ONLY( _class_id_load_barrier_Type = make_class_id_load_barrier_Type(); ) -#ifdef INCLUDE_JVMTI +#if INCLUDE_JVMTI _notify_jvmti_vthread_Type = make_notify_jvmti_vthread_Type(); #endif // INCLUDE_JVMTI _dtrace_method_entry_exit_Type = make_dtrace_method_entry_exit_Type(); diff --git a/src/hotspot/share/opto/runtime.hpp b/src/hotspot/share/opto/runtime.hpp index cfc79c8fdcc..fceece73f66 100644 --- a/src/hotspot/share/opto/runtime.hpp +++ b/src/hotspot/share/opto/runtime.hpp @@ -190,10 +190,10 @@ class OptoRuntime : public AllStatic { static const TypeFunc* _updateBytesAdler32_Type; static const TypeFunc* _osr_end_Type; static const TypeFunc* _register_finalizer_Type; -#ifdef INCLUDE_JFR +#if INCLUDE_JFR static const TypeFunc* _class_id_load_barrier_Type; #endif // INCLUDE_JFR -#ifdef INCLUDE_JVMTI +#if INCLUDE_JVMTI static const TypeFunc* _notify_jvmti_vthread_Type; #endif // INCLUDE_JVMTI static const TypeFunc* _dtrace_method_entry_exit_Type; @@ -645,7 +645,7 @@ class OptoRuntime : public AllStatic { return _register_finalizer_Type; } -#ifdef INCLUDE_JFR +#if INCLUDE_JFR static inline const TypeFunc* class_id_load_barrier_Type() { assert(_class_id_load_barrier_Type != nullptr, "should be initialized"); return _class_id_load_barrier_Type; From ca8ba5c890206546c79ce781878a3f8978e637f9 Mon Sep 17 00:00:00 2001 From: Robbin Ehn Date: Sat, 18 Jan 2025 09:16:22 +0000 Subject: [PATCH 2/4] 8347366: RISC-V: Add extension asserts for CMO instructions Reviewed-by: fyang, mli --- src/hotspot/cpu/riscv/assembler_riscv.hpp | 81 ++++++++++++++--------- 1 file changed, 51 insertions(+), 30 deletions(-) diff --git a/src/hotspot/cpu/riscv/assembler_riscv.hpp b/src/hotspot/cpu/riscv/assembler_riscv.hpp index 88317031562..bc01137163b 100644 --- a/src/hotspot/cpu/riscv/assembler_riscv.hpp +++ b/src/hotspot/cpu/riscv/assembler_riscv.hpp @@ -3070,42 +3070,63 @@ enum Nf { #undef INSN // Cache Management Operations -#define INSN(NAME, funct) \ - void NAME(Register Rs1) { \ - unsigned insn = 0; \ - patch((address)&insn, 6, 0, 0b0001111); \ - patch((address)&insn, 14, 12, 0b010); \ - patch_reg((address)&insn, 15, Rs1); \ - patch((address)&insn, 31, 20, funct); \ - emit(insn); \ +// These instruction may be turned off for user space. + private: + enum CBO_FUNCT : unsigned int { + CBO_INVAL = 0b0000000000000, + CBO_CLEAN = 0b0000000000001, + CBO_FLUSH = 0b0000000000010, + CBO_ZERO = 0b0000000000100 + }; + + template + void cbo_base(Register Rs1) { + assert((UseZicbom && FUNCT != CBO_ZERO) || UseZicboz, "sanity"); + unsigned insn = 0; + patch((address)&insn, 6, 0, 0b0001111); + patch((address)&insn, 14, 12, 0b010); + patch_reg((address)&insn, 15, Rs1); + patch((address)&insn, 31, 20, FUNCT); + emit(insn); } - INSN(cbo_inval, 0b0000000000000); - INSN(cbo_clean, 0b0000000000001); - INSN(cbo_flush, 0b0000000000010); - INSN(cbo_zero, 0b0000000000100); + // This instruction have some security implication. + // At this time it's not likely to be enabled for user mode. + void cbo_inval(Register Rs1) { cbo_base(Rs1); } + public: + // Zicbom + void cbo_clean(Register Rs1) { cbo_base(Rs1); } + void cbo_flush(Register Rs1) { cbo_base(Rs1); } + // Zicboz + void cbo_zero(Register Rs1) { cbo_base(Rs1); } -#undef INSN + private: + enum PREFETCH_FUNCT : unsigned int { + PREFETCH_I = 0b0000000000000, + PREFETCH_R = 0b0000000000001, + PREFETCH_W = 0b0000000000011 + }; -#define INSN(NAME, funct) \ - void NAME(Register Rs1, int32_t offset) { \ - guarantee((offset & 0x1f) == 0, "offset lowest 5 bits must be zero"); \ - int32_t upperOffset = offset >> 5; \ - unsigned insn = 0; \ - patch((address)&insn, 6, 0, 0b0010011); \ - patch((address)&insn, 14, 12, 0b110); \ - patch_reg((address)&insn, 15, Rs1); \ - patch((address)&insn, 24, 20, funct); \ - upperOffset &= 0x7f; \ - patch((address)&insn, 31, 25, upperOffset); \ - emit(insn); \ + template + void prefetch_base(Register Rs1, int32_t offset) { + assert_cond(UseZicbop); + guarantee((offset & 0x1f) == 0, "offset lowest 5 bits must be zero"); + int32_t upperOffset = offset >> 5; + unsigned insn = 0; + patch((address)&insn, 6, 0, 0b0010011); + patch((address)&insn, 14, 12, 0b110); + patch_reg((address)&insn, 15, Rs1); + patch((address)&insn, 24, 20, FUNCT); + upperOffset &= 0x7f; + patch((address)&insn, 31, 25, upperOffset); + emit(insn); } - INSN(prefetch_i, 0b0000000000000); - INSN(prefetch_r, 0b0000000000001); - INSN(prefetch_w, 0b0000000000011); - -#undef INSN + public: + // Zicbop + void prefetch_i(Register Rs1, int32_t offset) { prefetch_base(Rs1, offset); } + void prefetch_r(Register Rs1, int32_t offset) { prefetch_base(Rs1, offset); } + void prefetch_w(Register Rs1, int32_t offset) { prefetch_base(Rs1, offset); } // -------------- Zicond Instruction Definitions -------------- // Zicond conditional operations extension From 1f0efc00913e57690b57b7425bcc7dd6373e698f Mon Sep 17 00:00:00 2001 From: Robbin Ehn Date: Sat, 18 Jan 2025 09:19:21 +0000 Subject: [PATCH 3/4] 8347343: RISC-V: Unchecked zicntr csr reads Reviewed-by: fyang, mli --- .../cpu/riscv/macroAssembler_riscv.cpp | 19 +++----------- .../cpu/riscv/macroAssembler_riscv.hpp | 25 ++++++++++--------- src/hotspot/cpu/riscv/vm_version_riscv.hpp | 1 + 3 files changed, 18 insertions(+), 27 deletions(-) diff --git a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp index 40b7573aaca..d93772899d2 100644 --- a/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp +++ b/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp @@ -1333,22 +1333,11 @@ void MacroAssembler::cmov_gtu(Register cmp1, Register cmp2, Register dst, Regist #undef INSN - -#define INSN(NAME, CSR) \ - void MacroAssembler::NAME(Register Rd) { \ - csrr(Rd, CSR); \ - } - - INSN(rdinstret, CSR_INSTRET); - INSN(rdcycle, CSR_CYCLE); - INSN(rdtime, CSR_TIME); - INSN(frcsr, CSR_FCSR); - INSN(frrm, CSR_FRM); - INSN(frflags, CSR_FFLAGS); - -#undef INSN - void MacroAssembler::csrr(Register Rd, unsigned csr) { + // These three are specified in zicntr and are unused. + // Before adding use-cases add the appropriate hwprobe and flag. + assert(csr != CSR_INSTRET && csr != CSR_CYCLE && csr != CSR_TIME, + "Not intended for use without enabling zicntr."); csrrs(Rd, csr, x0); } diff --git a/src/hotspot/cpu/riscv/macroAssembler_riscv.hpp b/src/hotspot/cpu/riscv/macroAssembler_riscv.hpp index 06aa5537cc1..014297bcb32 100644 --- a/src/hotspot/cpu/riscv/macroAssembler_riscv.hpp +++ b/src/hotspot/cpu/riscv/macroAssembler_riscv.hpp @@ -626,9 +626,6 @@ class MacroAssembler: public Assembler { } // Control and status pseudo instructions - void rdinstret(Register Rd); // read instruction-retired counter - void rdcycle(Register Rd); // read cycle counter - void rdtime(Register Rd); // read time void csrr(Register Rd, unsigned csr); // read csr void csrw(unsigned csr, Register Rs); // write csr void csrs(unsigned csr, Register Rs); // set bits in csr @@ -636,19 +633,23 @@ class MacroAssembler: public Assembler { void csrwi(unsigned csr, unsigned imm); void csrsi(unsigned csr, unsigned imm); void csrci(unsigned csr, unsigned imm); - void frcsr(Register Rd); // read float-point csr - void fscsr(Register Rd, Register Rs); // swap float-point csr - void fscsr(Register Rs); // write float-point csr - void frrm(Register Rd); // read float-point rounding mode - void fsrm(Register Rd, Register Rs); // swap float-point rounding mode - void fsrm(Register Rs); // write float-point rounding mode + void frcsr(Register Rd) { csrr(Rd, CSR_FCSR); }; // read float-point csr + void fscsr(Register Rd, Register Rs); // swap float-point csr + void fscsr(Register Rs); // write float-point csr + void frrm(Register Rd) { csrr(Rd, CSR_FRM); }; // read float-point rounding mode + void fsrm(Register Rd, Register Rs); // swap float-point rounding mode + void fsrm(Register Rs); // write float-point rounding mode void fsrmi(Register Rd, unsigned imm); void fsrmi(unsigned imm); - void frflags(Register Rd); // read float-point exception flags - void fsflags(Register Rd, Register Rs); // swap float-point exception flags - void fsflags(Register Rs); // write float-point exception flags + void frflags(Register Rd) { csrr(Rd, CSR_FFLAGS); }; // read float-point exception flags + void fsflags(Register Rd, Register Rs); // swap float-point exception flags + void fsflags(Register Rs); // write float-point exception flags void fsflagsi(Register Rd, unsigned imm); void fsflagsi(unsigned imm); + // Requires Zicntr + void rdinstret(Register Rd) { csrr(Rd, CSR_INSTRET); }; // read instruction-retired counter + void rdcycle(Register Rd) { csrr(Rd, CSR_CYCLE); }; // read cycle counter + void rdtime(Register Rd) { csrr(Rd, CSR_TIME); }; // read time // Restore cpu control state after JNI call void restore_cpu_control_state_after_jni(Register tmp); diff --git a/src/hotspot/cpu/riscv/vm_version_riscv.hpp b/src/hotspot/cpu/riscv/vm_version_riscv.hpp index 59b41892fef..3ed706b7751 100644 --- a/src/hotspot/cpu/riscv/vm_version_riscv.hpp +++ b/src/hotspot/cpu/riscv/vm_version_riscv.hpp @@ -158,6 +158,7 @@ class VM_Version : public Abstract_VM_Version { decl(ext_Zcb , "Zcb" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZcb)) \ decl(ext_Zfh , "Zfh" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZfh)) \ decl(ext_Zicsr , "Zicsr" , RV_NO_FLAG_BIT, true , NO_UPDATE_DEFAULT) \ + decl(ext_Zicntr , "Zicntr" , RV_NO_FLAG_BIT, true , NO_UPDATE_DEFAULT) \ decl(ext_Zifencei , "Zifencei" , RV_NO_FLAG_BIT, true , NO_UPDATE_DEFAULT) \ decl(ext_Zic64b , "Zic64b" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZic64b)) \ decl(ext_Ztso , "Ztso" , RV_NO_FLAG_BIT, true , UPDATE_DEFAULT(UseZtso)) \ From 3804082cba56e6d26c500880cc5cbe6d4332d8f8 Mon Sep 17 00:00:00 2001 From: Robert Toyonaga Date: Sat, 18 Jan 2025 17:21:28 +0000 Subject: [PATCH 4/4] 8346123: [REDO] NMT should not use ThreadCritical Reviewed-by: dholmes, coleenp, stuefe --- 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 | 3 +- src/hotspot/share/nmt/memTracker.cpp | 2 + src/hotspot/share/nmt/memTracker.hpp | 60 +++++++++++++++---- src/hotspot/share/nmt/memoryFileTracker.cpp | 11 ---- src/hotspot/share/nmt/memoryFileTracker.hpp | 7 --- src/hotspot/share/nmt/nmtUsage.cpp | 3 +- src/hotspot/share/nmt/threadStackTracker.cpp | 5 +- .../share/nmt/virtualMemoryTracker.cpp | 13 +++- src/hotspot/share/prims/jni.cpp | 2 +- src/hotspot/share/runtime/mutexLocker.cpp | 4 +- src/hotspot/share/runtime/mutexLocker.hpp | 1 + src/hotspot/share/runtime/os.cpp | 8 +-- src/hotspot/share/runtime/threads.cpp | 2 + src/hotspot/share/utilities/vmError.cpp | 6 ++ .../runtime/test_virtualMemoryTracker.cpp | 9 +++ 19 files changed, 96 insertions(+), 49 deletions(-) diff --git a/src/hotspot/os/posix/perfMemory_posix.cpp b/src/hotspot/os/posix/perfMemory_posix.cpp index fdeb0b990d6..62c34ebe947 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; + MemTracker::NmtVirtualMemoryLocker nvml; 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 36feb7e2a7e..78ac42ebfe3 100644 --- a/src/hotspot/os/windows/os_windows.cpp +++ b/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 diff --git a/src/hotspot/os/windows/perfMemory_windows.cpp b/src/hotspot/os/windows/perfMemory_windows.cpp index 84c4d840a99..00a3d545cc3 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; + MemTracker::NmtVirtualMemoryLocker nvml; 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..5134c02a42d 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; + MemTracker::NmtVirtualMemoryLocker nvml; MemoryFileTracker::Instance::summary_snapshot(&_virtual_memory_snapshot); } diff --git a/src/hotspot/share/nmt/memReporter.cpp b/src/hotspot/share/nmt/memReporter.cpp index e6617223c16..63504b98ba9 100644 --- a/src/hotspot/share/nmt/memReporter.cpp +++ b/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()); diff --git a/src/hotspot/share/nmt/memTracker.cpp b/src/hotspot/share/nmt/memTracker.cpp index fb9c9a50db1..206ed14315c 100644 --- a/src/hotspot/share/nmt/memTracker.cpp +++ b/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"); diff --git a/src/hotspot/share/nmt/memTracker.hpp b/src/hotspot/share/nmt/memTracker.hpp index 6ba1db2e7ff..119720448d9 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" @@ -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,7 +167,7 @@ class MemTracker : AllStatic { assert_post_init(); if (!enabled()) return; if (addr != nullptr) { - ThreadCritical tc; + NmtVirtualMemoryLocker nvml; VirtualMemoryTracker::add_committed_region((address)addr, size, stack); } } @@ -170,7 +175,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 nvml; return MemoryFileTracker::Instance::make_file(descriptive_name); } @@ -178,7 +183,7 @@ class MemTracker : AllStatic { 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 diff --git a/src/hotspot/share/nmt/memoryFileTracker.cpp b/src/hotspot/share/nmt/memoryFileTracker.cpp index 83e7a1b917f..6e4280122ed 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; } @@ -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(); -} diff --git a/src/hotspot/share/nmt/memoryFileTracker.hpp b/src/hotspot/share/nmt/memoryFileTracker.hpp index 94f9cb2006c..035e9b466e2 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" @@ -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); diff --git a/src/hotspot/share/nmt/nmtUsage.cpp b/src/hotspot/share/nmt/nmtUsage.cpp index aa1d681b8a5..92978f017c5 100644 --- a/src/hotspot/share/nmt/nmtUsage.cpp +++ b/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(); diff --git a/src/hotspot/share/nmt/threadStackTracker.cpp b/src/hotspot/share/nmt/threadStackTracker.cpp index 6f112fa8fc5..51754ab0094 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; + 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--; } diff --git a/src/hotspot/share/nmt/virtualMemoryTracker.cpp b/src/hotspot/share/nmt/virtualMemoryTracker.cpp index 05db5341174..dd9172280a1 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; @@ -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* head = _reserved_regions->head(); diff --git a/src/hotspot/share/prims/jni.cpp b/src/hotspot/share/prims/jni.cpp index a37979ffe7f..ce797c68117 100644 --- a/src/hotspot/share/prims/jni.cpp +++ b/src/hotspot/share/prims/jni.cpp @@ -3790,8 +3790,8 @@ static jint attach_current_thread(JavaVM *vm, void **penv, void *_args, bool dae // be set in order for the Safepoint code to deal with it correctly. thread->set_thread_state(_thread_in_vm); thread->record_stack_base_and_size(); - thread->register_thread_stack_with_NMT(); thread->initialize_thread_current(); + thread->register_thread_stack_with_NMT(); MACOS_AARCH64_ONLY(thread->init_wx()); if (!os::create_attached_thread(thread)) { diff --git a/src/hotspot/share/runtime/mutexLocker.cpp b/src/hotspot/share/runtime/mutexLocker.cpp index 3d5b49f56ce..ab809908581 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 @@ -284,10 +285,10 @@ 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(DCmdFactory_lock , PaddedMutex , nosafepoint); MUTEX_DEFN(NMTQuery_lock , PaddedMutex , safepoint); MUTEX_DEFN(NMTCompilationCostHistory_lock , PaddedMutex , nosafepoint); + MUTEX_DEFN(NmtVirtualMemory_lock , PaddedMutex , service-4); // Must be lower than G1Mapper_lock used from G1RegionsSmallerThanCommitSizeMapper::commit_regions #if INCLUDE_CDS #if INCLUDE_JVMTI MUTEX_DEFN(CDSClassFileStream_lock , PaddedMutex , safepoint); @@ -345,6 +346,7 @@ void mutex_init() { MUTEX_DEFL(JVMCI_lock , PaddedMonitor, JVMCIRuntime_lock); #endif MUTEX_DEFL(JvmtiThreadState_lock , PaddedMutex , JvmtiVTMSTransition_lock); // Used by JvmtiThreadState/JvmtiEventController + MUTEX_DEFL(SharedDecoder_lock , PaddedMutex , NmtVirtualMemory_lock); // Must be lower than NmtVirtualMemory_lock due to MemTracker::print_containing_region // Allocate RecursiveMutex MultiArray_lock = new RecursiveMutex(); diff --git a/src/hotspot/share/runtime/mutexLocker.hpp b/src/hotspot/share/runtime/mutexLocker.hpp index 0e0c0435443..b5098a6d726 100644 --- a/src/hotspot/share/runtime/mutexLocker.hpp +++ b/src/hotspot/share/runtime/mutexLocker.hpp @@ -117,6 +117,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 1f807416d3d..6ea2eedaf6d 100644 --- a/src/hotspot/share/runtime/os.cpp +++ b/src/hotspot/share/runtime/os.cpp @@ -2203,7 +2203,7 @@ bool os::uncommit_memory(char* addr, size_t bytes, bool executable) { assert_nonempty_range(addr, bytes); bool res; if (MemTracker::enabled()) { - ThreadCritical tc; + MemTracker::NmtVirtualMemoryLocker nvml; res = pd_uncommit_memory(addr, bytes, executable); if (res) { MemTracker::record_virtual_memory_uncommit((address)addr, bytes); @@ -2225,7 +2225,7 @@ bool os::release_memory(char* addr, size_t bytes) { assert_nonempty_range(addr, bytes); bool res; if (MemTracker::enabled()) { - ThreadCritical tc; + MemTracker::NmtVirtualMemoryLocker nvml; res = pd_release_memory(addr, bytes); if (res) { MemTracker::record_virtual_memory_release((address)addr, bytes); @@ -2310,7 +2310,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; + MemTracker::NmtVirtualMemoryLocker nvml; result = pd_unmap_memory(addr, bytes); if (result) { MemTracker::record_virtual_memory_release((address)addr, bytes); @@ -2349,7 +2349,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; + MemTracker::NmtVirtualMemoryLocker nvml; res = pd_release_memory_special(addr, bytes); if (res) { MemTracker::record_virtual_memory_release((address)addr, bytes); diff --git a/src/hotspot/share/runtime/threads.cpp b/src/hotspot/share/runtime/threads.cpp index 1cab9bc5d53..691c9943afd 100644 --- a/src/hotspot/share/runtime/threads.cpp +++ b/src/hotspot/share/runtime/threads.cpp @@ -542,6 +542,8 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) { JavaThread* main_thread = new JavaThread(); main_thread->set_thread_state(_thread_in_vm); main_thread->initialize_thread_current(); + // Once mutexes and main_thread are ready, we can use NmtVirtualMemoryLocker. + MemTracker::NmtVirtualMemoryLocker::set_safe_to_use(); // must do this before set_active_handles main_thread->record_stack_base_and_size(); main_thread->register_thread_stack_with_NMT(); diff --git a/src/hotspot/share/utilities/vmError.cpp b/src/hotspot/share/utilities/vmError.cpp index 2c927a9be3c..97a81606589 100644 --- a/src/hotspot/share/utilities/vmError.cpp +++ b/src/hotspot/share/utilities/vmError.cpp @@ -649,6 +649,12 @@ 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("#"); diff --git a/test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp b/test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp index cb3e6ee24c0..91905480b51 100644 --- a/test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp +++ b/test/hotspot/gtest/runtime/test_virtualMemoryTracker.cpp @@ -93,6 +93,8 @@ class VirtualMemoryTrackerTest { size_t size = 0x01000000; ReservedSpace rs = MemoryReserver::reserve(size, mtTest); + MemTracker::NmtVirtualMemoryLocker nvml; + address addr = (address)rs.base(); address frame1 = (address)0x1234; @@ -167,6 +169,8 @@ class VirtualMemoryTrackerTest { size_t size = 0x01000000; ReservedSpace rs = MemoryReserver::reserve(size, mtTest); + MemTracker::NmtVirtualMemoryLocker nvml; + address addr = (address)rs.base(); address frame1 = (address)0x1234; @@ -253,7 +257,10 @@ class VirtualMemoryTrackerTest { static void test_add_committed_region_overlapping() { size_t size = 0x01000000; + ReservedSpace rs = MemoryReserver::reserve(size, mtTest); + MemTracker::NmtVirtualMemoryLocker nvml; + address addr = (address)rs.base(); address frame1 = (address)0x1234; @@ -425,6 +432,8 @@ class VirtualMemoryTrackerTest { size_t size = 0x01000000; ReservedSpace rs = MemoryReserver::reserve(size, mtTest); + MemTracker::NmtVirtualMemoryLocker nvml; + address addr = (address)rs.base(); address frame1 = (address)0x1234;