From 73c87a69c107723a4fd01811d3d662bbfb72de26 Mon Sep 17 00:00:00 2001 From: zgu Date: Wed, 10 Apr 2013 08:55:50 -0400 Subject: [PATCH] 8010151: nsk/regression/b6653214 fails "assert(snapshot != NULL) failed: Worker should not be started" Summary: Fixed a racing condition when shutting down NMT while worker thread is being started, also fixed a few mis-declared volatile pointers. Reviewed-by: dholmes, dlong --- src/share/vm/runtime/thread.hpp | 4 ++-- src/share/vm/services/memTrackWorker.cpp | 11 +++++----- src/share/vm/services/memTrackWorker.hpp | 4 +++- src/share/vm/services/memTracker.cpp | 26 ++++++++++++------------ src/share/vm/services/memTracker.hpp | 8 ++++---- 5 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/share/vm/runtime/thread.hpp b/src/share/vm/runtime/thread.hpp index 259a4765c..d64ca311c 100644 --- a/src/share/vm/runtime/thread.hpp +++ b/src/share/vm/runtime/thread.hpp @@ -1056,11 +1056,11 @@ class JavaThread: public Thread { #if INCLUDE_NMT // native memory tracking inline MemRecorder* get_recorder() const { return (MemRecorder*)_recorder; } - inline void set_recorder(MemRecorder* rc) { _recorder = (volatile MemRecorder*)rc; } + inline void set_recorder(MemRecorder* rc) { _recorder = rc; } private: // per-thread memory recorder - volatile MemRecorder* _recorder; + MemRecorder* volatile _recorder; #endif // INCLUDE_NMT // Suspend/resume support for JavaThread diff --git a/src/share/vm/services/memTrackWorker.cpp b/src/share/vm/services/memTrackWorker.cpp index 8c38d1a37..3e9bcd2f6 100644 --- a/src/share/vm/services/memTrackWorker.cpp +++ b/src/share/vm/services/memTrackWorker.cpp @@ -39,7 +39,7 @@ void GenerationData::reset() { } } -MemTrackWorker::MemTrackWorker() { +MemTrackWorker::MemTrackWorker(MemSnapshot* snapshot): _snapshot(snapshot) { // create thread uses cgc thread type for now. We should revisit // the option, or create new thread type. _has_error = !os::create_thread(this, os::cgc_thread); @@ -88,8 +88,7 @@ void MemTrackWorker::run() { assert(MemTracker::is_on(), "native memory tracking is off"); this->initialize_thread_local_storage(); this->record_stack_base_and_size(); - MemSnapshot* snapshot = MemTracker::get_snapshot(); - assert(snapshot != NULL, "Worker should not be started"); + assert(_snapshot != NULL, "Worker should not be started"); MemRecorder* rec; unsigned long processing_generation = 0; bool worker_idle = false; @@ -109,7 +108,7 @@ void MemTrackWorker::run() { } // merge the recorder into staging area - if (!snapshot->merge(rec)) { + if (!_snapshot->merge(rec)) { MemTracker::shutdown(MemTracker::NMT_out_of_memory); } else { NOT_PRODUCT(_merge_count ++;) @@ -132,7 +131,7 @@ void MemTrackWorker::run() { _head = (_head + 1) % MAX_GENERATIONS; } // promote this generation data to snapshot - if (!snapshot->promote(number_of_classes)) { + if (!_snapshot->promote(number_of_classes)) { // failed to promote, means out of memory MemTracker::shutdown(MemTracker::NMT_out_of_memory); } @@ -140,7 +139,7 @@ void MemTrackWorker::run() { // worker thread is idle worker_idle = true; MemTracker::report_worker_idle(); - snapshot->wait(1000); + _snapshot->wait(1000); ThreadCritical tc; // check if more data arrived if (!_gen[_head].has_more_recorder()) { diff --git a/src/share/vm/services/memTrackWorker.hpp b/src/share/vm/services/memTrackWorker.hpp index be80e294d..964aad31d 100644 --- a/src/share/vm/services/memTrackWorker.hpp +++ b/src/share/vm/services/memTrackWorker.hpp @@ -85,8 +85,10 @@ class MemTrackWorker : public NamedThread { bool _has_error; + MemSnapshot* _snapshot; + public: - MemTrackWorker(); + MemTrackWorker(MemSnapshot* snapshot); ~MemTrackWorker(); _NOINLINE_ void* operator new(size_t size); _NOINLINE_ void* operator new(size_t size, const std::nothrow_t& nothrow_constant); diff --git a/src/share/vm/services/memTracker.cpp b/src/share/vm/services/memTracker.cpp index 13b6de80a..3d4393073 100644 --- a/src/share/vm/services/memTracker.cpp +++ b/src/share/vm/services/memTracker.cpp @@ -53,12 +53,12 @@ void SyncThreadRecorderClosure::do_thread(Thread* thread) { } -MemRecorder* MemTracker::_global_recorder = NULL; +MemRecorder* volatile MemTracker::_global_recorder = NULL; MemSnapshot* MemTracker::_snapshot = NULL; MemBaseline MemTracker::_baseline; Mutex* MemTracker::_query_lock = NULL; -volatile MemRecorder* MemTracker::_merge_pending_queue = NULL; -volatile MemRecorder* MemTracker::_pooled_recorders = NULL; +MemRecorder* volatile MemTracker::_merge_pending_queue = NULL; +MemRecorder* volatile MemTracker::_pooled_recorders = NULL; MemTrackWorker* MemTracker::_worker_thread = NULL; int MemTracker::_sync_point_skip_count = 0; MemTracker::NMTLevel MemTracker::_tracking_level = MemTracker::NMT_off; @@ -128,7 +128,7 @@ void MemTracker::start() { _snapshot = new (std::nothrow)MemSnapshot(); if (_snapshot != NULL) { - if (!_snapshot->out_of_memory() && start_worker()) { + if (!_snapshot->out_of_memory() && start_worker(_snapshot)) { _state = NMT_started; NMT_track_callsite = (_tracking_level == NMT_detail && can_walk_stack()); return; @@ -209,7 +209,7 @@ void MemTracker::final_shutdown() { // delete all pooled recorders void MemTracker::delete_all_pooled_recorders() { // free all pooled recorders - volatile MemRecorder* cur_head = _pooled_recorders; + MemRecorder* volatile cur_head = _pooled_recorders; if (cur_head != NULL) { MemRecorder* null_ptr = NULL; while (cur_head != NULL && (void*)cur_head != Atomic::cmpxchg_ptr((void*)null_ptr, @@ -543,14 +543,14 @@ void MemTracker::sync() { /* * Start worker thread. */ -bool MemTracker::start_worker() { - assert(_worker_thread == NULL, "Just Check"); - _worker_thread = new (std::nothrow) MemTrackWorker(); - if (_worker_thread == NULL || _worker_thread->has_error()) { - if (_worker_thread != NULL) { - delete _worker_thread; - _worker_thread = NULL; - } +bool MemTracker::start_worker(MemSnapshot* snapshot) { + assert(_worker_thread == NULL && _snapshot != NULL, "Just Check"); + _worker_thread = new (std::nothrow) MemTrackWorker(snapshot); + if (_worker_thread == NULL) { + return false; + } else if (_worker_thread->has_error()) { + delete _worker_thread; + _worker_thread = NULL; return false; } _worker_thread->start(); diff --git a/src/share/vm/services/memTracker.hpp b/src/share/vm/services/memTracker.hpp index ebcc41500..a7d067552 100644 --- a/src/share/vm/services/memTracker.hpp +++ b/src/share/vm/services/memTracker.hpp @@ -421,7 +421,7 @@ class MemTracker : AllStatic { private: // start native memory tracking worker thread - static bool start_worker(); + static bool start_worker(MemSnapshot* snapshot); // called by worker thread to complete shutdown process static void final_shutdown(); @@ -475,18 +475,18 @@ class MemTracker : AllStatic { // a thread can start to allocate memory before it is attached // to VM 'Thread', those memory activities are recorded here. // ThreadCritical is required to guard this global recorder. - static MemRecorder* _global_recorder; + static MemRecorder* volatile _global_recorder; // main thread id debug_only(static intx _main_thread_tid;) // pending recorders to be merged - static volatile MemRecorder* _merge_pending_queue; + static MemRecorder* volatile _merge_pending_queue; NOT_PRODUCT(static volatile jint _pending_recorder_count;) // pooled memory recorders - static volatile MemRecorder* _pooled_recorders; + static MemRecorder* volatile _pooled_recorders; // memory recorder pool management, uses following // counter to determine if a released memory recorder -- GitLab