8220671: Initialization race for non-JavaThread PtrQueues

Include on_thread_(attach|detach) under NJTList_lock.

Reviewed-by: pliden, rkennke

src/hotspot/share/gc/g1/g1BarrierSet.cpp

@@ -148,24 +148,7 @@ void G1BarrierSet::on_thread_attach(Thread* thread) {
 
   // 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
+  // copying the global is_active value to this thread's queue.
-  // 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.
   bool is_satb_active = _satb_mark_queue_set.is_active();
   G1ThreadLocalData::satb_mark_queue(thread).set_active(is_satb_active);
 }

src/hotspot/share/gc/shared/barrierSet.hpp

@@ -130,8 +130,18 @@ public:
   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

src/hotspot/share/gc/shared/satbMarkQueue.cpp

@@ -170,7 +170,11 @@ void SATBMarkQueueSet::set_active_all_threads(bool active, bool expected_active)
 #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;

src/hotspot/share/runtime/mutexLocker.cpp

@@ -76,6 +76,7 @@ Monitor* VMOperationRequest_lock      = NULL;
 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 @@ void mutex_init() {
 
   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);

src/hotspot/share/runtime/mutexLocker.hpp

@@ -72,6 +72,7 @@ extern Monitor* VMOperationRequest_lock;         // a lock on Threads waiting fo
 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.

src/hotspot/share/runtime/thread.cpp

@@ -1291,29 +1291,35 @@ NonJavaThread::NonJavaThread() : Thread(), _next(NULL) {
 NonJavaThread::~NonJavaThread() { }
 
 void NonJavaThread::add_to_the_list() {
-  MutexLockerEx lock(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
+  MutexLockerEx ml(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
-  _next = _the_list._head;
+  // 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;
+    MutexLockerEx ml(NonJavaThreadsList_lock, Mutex::_no_safepoint_check_flag);
-  for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) {
+    // Cleanup BarrierSet-related data before removing from list.
-    if (t == this) {
+    BarrierSet::barrier_set()->on_thread_detach(this);
-      *p = this->_next;
+    NonJavaThread* volatile* p = &_the_list._head;
-      // Wait for any in-progress iterators.  Concurrent synchronize is
+    for (NonJavaThread* t = *p; t != NULL; p = &t->_next, t = *p) {
-      // not allowed, so do it while holding the list lock.
+      if (t == this) {
-      _the_list._protect.synchronize();
+        *p = _next;
-      break;
+        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::pre_run() {
 
 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();