# HG changeset patch
# User tonyp
# Date 1285965785 14400
# Node ID 6e0aac35bfa97bb4cf59968a26d8a66f98a16981
# Parent  32a1f7bf0c21ac33c6d748f432e0d2bb8a5e601a
6980838: G1: guarantee(false) failed: thread has an unexpected active value in its SATB queue
Summary: Under certain circumstances a safepoint could happen between a JavaThread object being created and that object being added to the Java threads list. This could cause the active field of that thread's SATB queue to get out-of-sync with respect to the other Java threads. The solution is to activate the SATB queue, when necessary, before adding the thread to the Java threads list, not when the JavaThread object is created. The changeset also includes a small fix to rename the surrogate locker thread from "Surrogate Locker Thread (CMS)" to "Surrogate Locker Thread (Concurrent GC)" since it's also used in G1.
Reviewed-by: iveresov, ysr, johnc, jcoomes

diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/gc_implementation/g1/dirtyCardQueue.hpp
--- a/src/share/vm/gc_implementation/g1/dirtyCardQueue.hpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/dirtyCardQueue.hpp	Fri Oct 01 16:43:05 2010 -0400
@@ -37,11 +37,10 @@
 class DirtyCardQueue: public PtrQueue {
 public:
   DirtyCardQueue(PtrQueueSet* qset_, bool perm = false) :
-    PtrQueue(qset_, perm)
-  {
-    // Dirty card queues are always active.
-    _active = true;
-  }
+    // Dirty card queues are always active, so we create them with their
+    // active field set to true.
+    PtrQueue(qset_, perm, true /* active */) { }
+
   // Apply the closure to all elements, and reset the index to make the
   // buffer empty.  If a closure application returns "false", return
   // "false" immediately, halting the iteration.  If "consume" is true,
diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/gc_implementation/g1/ptrQueue.hpp
--- a/src/share/vm/gc_implementation/g1/ptrQueue.hpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/ptrQueue.hpp	Fri Oct 01 16:43:05 2010 -0400
@@ -89,6 +89,10 @@
     return _buf == NULL ? 0 : _sz - _index;
   }
 
+  bool is_empty() {
+    return _buf == NULL || _sz == _index;
+  }
+
   // Set the "active" property of the queue to "b".  An enqueue to an
   // inactive thread is a no-op.  Setting a queue to inactive resets its
   // log to the empty state.
diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/gc_implementation/g1/satbQueue.hpp
--- a/src/share/vm/gc_implementation/g1/satbQueue.hpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/gc_implementation/g1/satbQueue.hpp	Fri Oct 01 16:43:05 2010 -0400
@@ -29,7 +29,12 @@
 class ObjPtrQueue: public PtrQueue {
 public:
   ObjPtrQueue(PtrQueueSet* qset_, bool perm = false) :
-    PtrQueue(qset_, perm, qset_->is_active()) { }
+    // SATB queues are only active during marking cycles. We create
+    // them with their active field set to false. If a thread is
+    // created during a cycle and its SATB queue needs to be activated
+    // before the thread starts running, we'll need to set its active
+    // field to true. This is done in JavaThread::initialize_queues().
+    PtrQueue(qset_, perm, false /* active */) { }
   // Apply the closure to all elements, and reset the index to make the
   // buffer empty.
   void apply_closure(ObjectClosure* cl);
diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/gc_implementation/shared/concurrentGCThread.cpp
--- a/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp	Fri Oct 01 16:43:05 2010 -0400
@@ -185,7 +185,7 @@
   instanceKlassHandle klass (THREAD, k);
   instanceHandle thread_oop = klass->allocate_instance_handle(CHECK_NULL);
 
-  const char thread_name[] = "Surrogate Locker Thread (CMS)";
+  const char thread_name[] = "Surrogate Locker Thread (Concurrent GC)";
   Handle string = java_lang_String::create_from_str(thread_name, CHECK_NULL);
 
   // Initialize thread_oop to put it into the system threadGroup
diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/runtime/thread.cpp
--- a/src/share/vm/runtime/thread.cpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/runtime/thread.cpp	Fri Oct 01 16:43:05 2010 -0400
@@ -1644,7 +1644,29 @@
   satb_mark_queue().flush();
   dirty_card_queue().flush();
 }
-#endif
+
+void JavaThread::initialize_queues() {
+  assert(!SafepointSynchronize::is_at_safepoint(),
+         "we should not be at a safepoint");
+
+  ObjPtrQueue& satb_queue = satb_mark_queue();
+  SATBMarkQueueSet& satb_queue_set = satb_mark_queue_set();
+  // The SATB queue should have been constructed with its active
+  // field set to false.
+  assert(!satb_queue.is_active(), "SATB queue should not be active");
+  assert(satb_queue.is_empty(), "SATB queue should be empty");
+  // If we are creating the thread during a marking cycle, we should
+  // set the active field of the SATB queue to true.
+  if (satb_queue_set.is_active()) {
+    satb_queue.set_active(true);
+  }
+
+  DirtyCardQueue& dirty_queue = dirty_card_queue();
+  // The dirty card queue should have been constructed with its
+  // active field set to true.
+  assert(dirty_queue.is_active(), "dirty card queue should be active");
+}
+#endif // !SERIALGC
 
 void JavaThread::cleanup_failed_attach_current_thread() {
   if (get_thread_profiler() != NULL) {
@@ -3626,6 +3648,10 @@
 void Threads::add(JavaThread* p, bool force_daemon) {
   // The threads lock must be owned at this point
   assert_locked_or_safepoint(Threads_lock);
+
+  // See the comment for this method in thread.hpp for its purpose and
+  // why it is called here.
+  p->initialize_queues();
   p->set_next(_thread_list);
   _thread_list = p;
   _number_of_threads++;
diff -r 32a1f7bf0c21 -r 6e0aac35bfa9 src/share/vm/runtime/thread.hpp
--- a/src/share/vm/runtime/thread.hpp	Fri Oct 01 21:48:40 2010 -0700
+++ b/src/share/vm/runtime/thread.hpp	Fri Oct 01 16:43:05 2010 -0400
@@ -1487,6 +1487,29 @@
   }
 #endif // !SERIALGC
 
+  // This method initializes the SATB and dirty card queues before a
+  // JavaThread is added to the Java thread list. Right now, we don't
+  // have to do anything to the dirty card queue (it should have been
+  // activated when the thread was created), but we have to activate
+  // the SATB queue if the thread is created while a marking cycle is
+  // in progress. The activation / de-activation of the SATB queues at
+  // the beginning / end of a marking cycle is done during safepoints
+  // so we have to make sure this method is called outside one to be
+  // able to safely read the active field of the SATB queue set. Right
+  // now, it is called just before the thread is added to the Java
+  // thread list in the Threads::add() method. That method is holding
+  // the Threads_lock which ensures we are outside a safepoint. We
+  // cannot do the obvious and set the active field of the SATB queue
+  // when the thread is created given that, in some cases, safepoints
+  // might happen between the JavaThread constructor being called and the
+  // thread being added to the Java thread list (an example of this is
+  // when the structure for the DestroyJavaVM thread is created).
+#ifndef SERIALGC
+  void initialize_queues();
+#else // !SERIALGC
+  void initialize_queues() { }
+#endif // !SERIALGC
+
   // Machine dependent stuff
   #include "incls/_thread_pd.hpp.incl"