diff --git a/src/hotspot/share/runtime/objectMonitor.cpp b/src/hotspot/share/runtime/objectMonitor.cpp index ee0f754b865..1e5b601b140 100644 --- a/src/hotspot/share/runtime/objectMonitor.cpp +++ b/src/hotspot/share/runtime/objectMonitor.cpp @@ -53,6 +53,7 @@ #include "runtime/sharedRuntime.hpp" #include "services/threadService.hpp" #include "utilities/dtrace.hpp" +#include "utilities/globalCounter.inline.hpp" #include "utilities/macros.hpp" #include "utilities/preserveException.hpp" #if INCLUDE_JFR @@ -828,9 +829,9 @@ void ObjectMonitor::EnterI(JavaThread* current) { // That is by design - we trade "lossy" counters which are exposed to // races during updates for a lower probe effect. - // This PerfData object can be used in parallel with a safepoint. - // See the work around in PerfDataManager::destroy(). - OM_PERFDATA_OP(FutileWakeups, inc()); + // We are in safepoint safe state, so shutdown can remove the counter + // under our feet. Make sure we make this access safely. + OM_PERFDATA_SAFE_OP(FutileWakeups, inc()); ++nWakeups; // Assuming this is not a spurious wakeup we'll normally find _succ == current. @@ -971,8 +972,6 @@ void ObjectMonitor::ReenterI(JavaThread* current, ObjectWaiter* currentNode) { // *must* retry _owner before parking. OrderAccess::fence(); - // This PerfData object can be used in parallel with a safepoint. - // See the work around in PerfDataManager::destroy(). OM_PERFDATA_OP(FutileWakeups, inc()); } diff --git a/src/hotspot/share/runtime/objectMonitor.hpp b/src/hotspot/share/runtime/objectMonitor.hpp index d6c0e31f7a1..4997294ecca 100644 --- a/src/hotspot/share/runtime/objectMonitor.hpp +++ b/src/hotspot/share/runtime/objectMonitor.hpp @@ -197,13 +197,31 @@ class ObjectMonitor : public CHeapObj { // Only perform a PerfData operation if the PerfData object has been // allocated and if the PerfDataManager has not freed the PerfData - // objects which can happen at normal VM shutdown. - // + // objects which can happen at normal VM shutdown. This operation is + // only safe when thread is not in safepoint-safe code, i.e. PerfDataManager + // could not reach the safepoint and free the counter while we are using it. + // If this is not guaranteed, use OM_PERFDATA_SAFE_OP instead. #define OM_PERFDATA_OP(f, op_str) \ do { \ - if (ObjectMonitor::_sync_ ## f != nullptr && \ - PerfDataManager::has_PerfData()) { \ - ObjectMonitor::_sync_ ## f->op_str; \ + if (ObjectMonitor::_sync_ ## f != nullptr) { \ + if (PerfDataManager::has_PerfData()) { \ + ObjectMonitor::_sync_ ## f->op_str; \ + } \ + } \ + } while (0) + + // Only perform a PerfData operation if the PerfData object has been + // allocated and if the PerfDataManager has not freed the PerfData + // objects which can happen at normal VM shutdown. Additionally, we + // enter the critical section to resolve the race against PerfDataManager + // entering the safepoint and deleting the counter during shutdown. + #define OM_PERFDATA_SAFE_OP(f, op_str) \ + do { \ + if (ObjectMonitor::_sync_ ## f != nullptr) { \ + GlobalCounter::CriticalSection cs(Thread::current()); \ + if (PerfDataManager::has_PerfData()) { \ + ObjectMonitor::_sync_ ## f->op_str; \ + } \ } \ } while (0) diff --git a/src/hotspot/share/runtime/perfData.cpp b/src/hotspot/share/runtime/perfData.cpp index f748826a622..6d00e348297 100644 --- a/src/hotspot/share/runtime/perfData.cpp +++ b/src/hotspot/share/runtime/perfData.cpp @@ -35,6 +35,7 @@ #include "runtime/os.hpp" #include "runtime/perfData.inline.hpp" #include "utilities/exceptions.hpp" +#include "utilities/globalCounter.inline.hpp" #include "utilities/globalDefinitions.hpp" PerfDataList* PerfDataManager::_all = nullptr; @@ -251,15 +252,13 @@ void PerfDataManager::destroy() { // destroy already called, or initialization never happened return; - // Clear the flag before we free the PerfData counters. Thus begins - // the race between this thread and another thread that has just - // queried PerfDataManager::has_PerfData() and gotten back 'true'. - // The hope is that the other thread will finish its PerfData - // manipulation before we free the memory. The two alternatives are - // 1) leak the PerfData memory or 2) do some form of synchronized - // access or check before every PerfData operation. - _has_PerfData = false; - os::naked_short_sleep(1); // 1ms sleep to let other thread(s) run + // About to delete the counters than might still be accessed by other threads. + // The shutdown is performed in two stages: a) clear the flag to notify future + // counter users that we are at shutdown; b) sync up with current users, waiting + // for them to finish with counters. + // + Atomic::store(&_has_PerfData, false); + GlobalCounter::write_synchronize(); log_debug(perf, datacreation)("Total = %d, Sampled = %d, Constants = %d", _all->length(), _sampled == nullptr ? 0 : _sampled->length(), @@ -286,7 +285,7 @@ void PerfDataManager::add_item(PerfData* p, bool sampled) { // Default sizes determined using -Xlog:perf+datacreation=debug if (_all == nullptr) { _all = new PerfDataList(191); - _has_PerfData = true; + Atomic::release_store(&_has_PerfData, true); } assert(!_all->contains(p->name()), "duplicate name added"); diff --git a/src/hotspot/share/runtime/perfData.hpp b/src/hotspot/share/runtime/perfData.hpp index d4ac8a735ec..462e02361f8 100644 --- a/src/hotspot/share/runtime/perfData.hpp +++ b/src/hotspot/share/runtime/perfData.hpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2001, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2001, 2025, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -26,6 +26,7 @@ #define SHARE_RUNTIME_PERFDATA_HPP #include "memory/allocation.hpp" +#include "runtime/atomic.hpp" #include "runtime/perfDataTypes.hpp" #include "runtime/perfMemory.hpp" #include "runtime/timer.hpp" @@ -241,6 +242,18 @@ enum CounterNS { * UsePerfData, a product flag set to true by default. This flag may * be removed from the product in the future. * + * There are possible shutdown races between counter uses and counter + * destruction code. Normal shutdown happens with taking VM_Exit safepoint + * operation, so in the vast majority of uses this is not an issue. On the + * paths where a concurrent access can still happen when VM is at safepoint, + * use the following pattern to coordinate with shutdown: + * + * { + * GlobalCounter::CriticalSection cs(Thread::current()); + * if (PerfDataManager::has_PerfData()) { + * + * } + * } */ class PerfData : public CHeapObj { @@ -785,7 +798,7 @@ class PerfDataManager : AllStatic { } static void destroy(); - static bool has_PerfData() { return _has_PerfData; } + static bool has_PerfData() { return Atomic::load_acquire(&_has_PerfData); } }; // Useful macros to create the performance counters diff --git a/src/hotspot/share/runtime/synchronizer.cpp b/src/hotspot/share/runtime/synchronizer.cpp index cc73082ed3c..3cd3c56bcec 100644 --- a/src/hotspot/share/runtime/synchronizer.cpp +++ b/src/hotspot/share/runtime/synchronizer.cpp @@ -60,6 +60,7 @@ #include "utilities/align.hpp" #include "utilities/dtrace.hpp" #include "utilities/events.hpp" +#include "utilities/globalCounter.inline.hpp" #include "utilities/linkedlist.hpp" #include "utilities/preserveException.hpp"