# HG changeset patch # User kbarrett # Date 1554153098 14400 # Node ID 2b48cedce327700188b4bc714e8d71cafb0315d1 # Parent dd5c643260278faa72a5f5b4daf80faef052efe3 8220671: Initialization race for non-JavaThread PtrQueues Summary: Include on_thread_(attach|detach) under NJTList_lock. Reviewed-by: pliden, rkennke diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/gc/g1/g1BarrierSet.cpp --- a/src/hotspot/share/gc/g1/g1BarrierSet.cpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/gc/g1/g1BarrierSet.cpp Mon Apr 01 17:11:38 2019 -0400 @@ -148,24 +148,7 @@ // If we are creating the thread during a marking cycle, we should // set the active field of the SATB queue to true. That involves - // copying the global is_active value to this thread's queue, which - // is done without any direct synchronization here. - // - // The activation and deactivation of the SATB queues occurs at the - // beginning / end of a marking cycle, and is done during - // safepoints. This function is called just before a thread is - // added to its corresponding threads list (for Java or non-Java - // threads, respectively). - // - // For Java threads, that's done while holding the Threads_lock, - // which ensures we're not at a safepoint, so reading the global - // is_active state is synchronized against update. - assert(!thread->is_Java_thread() || !SafepointSynchronize::is_at_safepoint(), - "Should not be at a safepoint"); - // For non-Java threads, thread creation (and list addition) may, - // and indeed usually does, occur during a safepoint. But such - // creation isn't concurrent with updating the global SATB active - // state. + // copying the global is_active value to this thread's queue. bool is_satb_active = _satb_mark_queue_set.is_active(); G1ThreadLocalData::satb_mark_queue(thread).set_active(is_satb_active); } diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/gc/shared/barrierSet.hpp --- a/src/hotspot/share/gc/shared/barrierSet.hpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/gc/shared/barrierSet.hpp Mon Apr 01 17:11:38 2019 -0400 @@ -130,8 +130,18 @@ virtual void on_slowpath_allocation_exit(JavaThread* thread, oop new_obj) {} virtual void on_thread_create(Thread* thread) {} virtual void on_thread_destroy(Thread* thread) {} + + // These perform BarrierSet-related initialization/cleanup before the thread + // is added to or removed from the corresponding set of threads. The + // argument thread is the current thread. These are called either holding + // the Threads_lock (for a JavaThread) and so not at a safepoint, or holding + // the NonJavaThreadsList_lock (for a NonJavaThread) locked by the + // caller. That locking ensures the operation is "atomic" with the list + // modification wrto operations that hold the NJTList_lock and either also + // hold the Threads_lock or are at a safepoint. virtual void on_thread_attach(Thread* thread) {} virtual void on_thread_detach(Thread* thread) {} + virtual void make_parsable(JavaThread* thread) {} #ifdef CHECK_UNHANDLED_OOPS diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/gc/shared/satbMarkQueue.cpp --- a/src/hotspot/share/gc/shared/satbMarkQueue.cpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/gc/shared/satbMarkQueue.cpp Mon Apr 01 17:11:38 2019 -0400 @@ -170,7 +170,11 @@ #ifdef ASSERT verify_active_states(expected_active); #endif // ASSERT - _all_active = active; + // Update the global state, synchronized with threads list management. + { + MutexLockerEx ml(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); + _all_active = active; + } class SetThreadActiveClosure : public ThreadClosure { SATBMarkQueueSet* _qset; diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/runtime/mutexLocker.cpp --- a/src/hotspot/share/runtime/mutexLocker.cpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/runtime/mutexLocker.cpp Mon Apr 01 17:11:38 2019 -0400 @@ -76,6 +76,7 @@ Monitor* SerializePage_lock = NULL; Monitor* Threads_lock = NULL; Mutex* NonJavaThreadsList_lock = NULL; +Mutex* NonJavaThreadsListSync_lock = NULL; Monitor* CGC_lock = NULL; Monitor* STS_lock = NULL; Monitor* FullGCCount_lock = NULL; @@ -279,6 +280,7 @@ def(Threads_lock , PaddedMonitor, barrier, true, Monitor::_safepoint_check_sometimes); def(NonJavaThreadsList_lock , PaddedMutex, leaf, true, Monitor::_safepoint_check_never); + def(NonJavaThreadsListSync_lock , PaddedMutex, leaf, true, Monitor::_safepoint_check_never); def(VMOperationQueue_lock , PaddedMonitor, nonleaf, true, Monitor::_safepoint_check_sometimes); // VM_thread allowed to block on these def(VMOperationRequest_lock , PaddedMonitor, nonleaf, true, Monitor::_safepoint_check_sometimes); diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/runtime/mutexLocker.hpp --- a/src/hotspot/share/runtime/mutexLocker.hpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/runtime/mutexLocker.hpp Mon Apr 01 17:11:38 2019 -0400 @@ -72,6 +72,7 @@ extern Monitor* Threads_lock; // a lock on the Threads table of active Java threads // (also used by Safepoints too to block threads creation/destruction) extern Mutex* NonJavaThreadsList_lock; // a lock on the NonJavaThreads list +extern Mutex* NonJavaThreadsListSync_lock; // a lock for NonJavaThreads list synchronization extern Monitor* CGC_lock; // used for coordination between // fore- & background GC threads. extern Monitor* STS_lock; // used for joining/leaving SuspendibleThreadSet. diff -r dd5c64326027 -r 2b48cedce327 src/hotspot/share/runtime/thread.cpp --- a/src/hotspot/share/runtime/thread.cpp Mon Apr 01 11:02:40 2019 -0700 +++ b/src/hotspot/share/runtime/thread.cpp Mon Apr 01 17:11:38 2019 -0400 @@ -1291,29 +1291,35 @@ NonJavaThread::~NonJavaThread() { } void NonJavaThread::add_to_the_list() { - MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); - _next = _the_list._head; + MutexLockerEx ml(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); + // Initialize BarrierSet-related data before adding to list. + BarrierSet::barrier_set()->on_thread_attach(this); + OrderAccess::release_store(&_next, _the_list._head); OrderAccess::release_store(&_the_list._head, this); } void NonJavaThread::remove_from_the_list() { - MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); - NonJavaThread* volatile* p = &_the_list._head; - for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) { - if (t == this) { - *p = this->_next; - // Wait for any in-progress iterators. Concurrent synchronize is - // not allowed, so do it while holding the list lock. - _the_list._protect.synchronize(); - break; + { + MutexLockerEx ml(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag); + // Cleanup BarrierSet-related data before removing from list. + BarrierSet::barrier_set()->on_thread_detach(this); + NonJavaThread* volatile* p = &_the_list._head; + for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) { + if (t == this) { + *p = _next; + break; + } } } + // Wait for any in-progress iterators. Concurrent synchronize is not + // allowed, so do it while holding a dedicated lock. Outside and distinct + // from NJTList_lock in case an iteration attempts to lock it. + MutexLockerEx ml(NonJavaThreadsListSync_lock, Mutex::_no_safepoint_check_flag); + _the_list._protect.synchronize(); + _next = NULL; // Safe to drop the link now. } void NonJavaThread::pre_run() { - // Initialize BarrierSet-related data before adding to list. - assert(BarrierSet::barrier_set() != NULL, "invariant"); - BarrierSet::barrier_set()->on_thread_attach(this); add_to_the_list(); // This is slightly odd in that NamedThread is a subclass, but @@ -1324,8 +1330,6 @@ void NonJavaThread::post_run() { JFR_ONLY(Jfr::on_thread_exit(this);) - // Clean up BarrierSet data before removing from list. - BarrierSet::barrier_set()->on_thread_detach(this); remove_from_the_list(); // Ensure thread-local-storage is cleared before termination. Thread::clear_thread_current();