src/hotspot/share/runtime/handshake.cpp

rev 52283 : [mq]: 8212933
rev 52284 : 8212933: Thread-SMR: requesting a VM operation whilst holding a ThreadsListHandle can cause deadlocks
Reviewed-by: eosterlund, dcubed, sspitsyn

@@ -39,22 +39,19 @@
 #include "utilities/preserveException.hpp"
 
 class HandshakeOperation: public StackObj {
 public:
   virtual void do_handshake(JavaThread* thread) = 0;
-  virtual void cancel_handshake(JavaThread* thread) = 0;
 };
 
 class HandshakeThreadsOperation: public HandshakeOperation {
   static Semaphore _done;
   ThreadClosure* _thread_cl;
 
 public:
   HandshakeThreadsOperation(ThreadClosure* cl) : _thread_cl(cl) {}
   void do_handshake(JavaThread* thread);
-  void cancel_handshake(JavaThread* thread) { _done.signal(); };
-
   bool thread_has_completed() { return _done.trywait(); }
 
 #ifdef ASSERT
   void check_state() {
     assert(!_done.trywait(), "Must be zero");

@@ -119,19 +116,15 @@
 
   void doit() {
     DEBUG_ONLY(_op->check_state();)
     TraceTime timer("Performing single-target operation (vmoperation doit)", TRACETIME_LOG(Info, handshake));
 
-    {
       ThreadsListHandle tlh;
       if (tlh.includes(_target)) {
         set_handshake(_target);
         _thread_alive = true;
-      }
-    }
-
-    if (!_thread_alive) {
+    } else {
       return;
     }
 
     if (!UseMembar) {
       os::serialize_thread_states();

@@ -145,24 +138,13 @@
       }
 
       // We need to re-think this with SMR ThreadsList.
       // There is an assumption in the code that the Threads_lock should be
       // locked during certain phases.
+      {
       MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
-      ThreadsListHandle tlh;
-      if (tlh.includes(_target)) {
-        // Warning _target's address might be re-used.
-        // handshake_process_by_vmthread will check the semaphore for us again.
-        // Since we can't have more then one handshake in flight a reuse of
-        // _target's address should be okay since the new thread will not have
-        // an operation.
         _target->handshake_process_by_vmthread();
-      } else {
-        // We can't warn here since the thread does cancel_handshake after
-        // it has been removed from the ThreadsList. So we should just keep
-        // looping here until while below returns false. If we have a bug,
-        // then we hang here, which is good for debugging.
       }
     } while (!poll_for_completed_thread());
     DEBUG_ONLY(_op->check_state();)
   }
 

@@ -177,12 +159,13 @@
 
   void doit() {
     DEBUG_ONLY(_op->check_state();)
     TraceTime timer("Performing operation (vmoperation doit)", TRACETIME_LOG(Info, handshake));
 
+    JavaThreadIteratorWithHandle jtiwh;
     int number_of_threads_issued = 0;
-    for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thr = jtiwh.next(); ) {
+    for ( ; JavaThread *thr = jtiwh.next(); ) {
       set_handshake(thr);
       number_of_threads_issued++;
     }
 
     if (number_of_threads_issued < 1) {

@@ -208,12 +191,13 @@
       // by semaphores and we optimistically begin by working on the blocked threads
       {
           // We need to re-think this with SMR ThreadsList.
           // There is an assumption in the code that the Threads_lock should
           // be locked during certain phases.
+          jtiwh.rewind();
           MutexLockerEx ml(Threads_lock, Mutex::_no_safepoint_check_flag);
-          for (JavaThreadIteratorWithHandle jtiwh; JavaThread *thr = jtiwh.next(); ) {
+          for ( ; JavaThread *thr = jtiwh.next(); ) {
             // A new thread on the ThreadsList will not have an operation,
             // hence it is skipped in handshake_process_by_vmthread.
             thr->handshake_process_by_vmthread();
           }
       }

@@ -260,11 +244,15 @@
 void HandshakeThreadsOperation::do_handshake(JavaThread* thread) {
   ResourceMark rm;
   FormatBufferResource message("Operation for thread " PTR_FORMAT ", is_vm_thread: %s",
                                p2i(thread), BOOL_TO_STR(Thread::current()->is_VM_thread()));
   TraceTime timer(message, TRACETIME_LOG(Debug, handshake, task));
+
+  // Only actually execute the operation for non terminated threads.
+  if (!thread->is_terminated()) {
   _thread_cl->do_thread(thread);
+  }
 
   // Use the semaphore to inform the VM thread that we have completed the operation
   _done.signal();
 }
 

@@ -304,16 +292,11 @@
   SafepointMechanism::disarm_local_poll_release(target);
 }
 
 void HandshakeState::process_self_inner(JavaThread* thread) {
   assert(Thread::current() == thread, "should call from thread");
-
-  if (thread->is_terminated()) {
-    // If thread is not on threads list but armed, cancel.
-    thread->cancel_handshake();
-    return;
-  }
+  assert(!thread->is_terminated(), "should not be a terminated thread");
 
   CautiouslyPreserveExceptionMark pem(thread);
   ThreadInVMForHandshake tivm(thread);
   if (!_semaphore.trywait()) {
     _semaphore.wait_with_safepoint_check(thread);

@@ -325,38 +308,31 @@
     op->do_handshake(thread);
   }
   _semaphore.signal();
 }
 
-void HandshakeState::cancel_inner(JavaThread* thread) {
-  assert(Thread::current() == thread, "should call from thread");
-  assert(thread->thread_state() == _thread_in_vm, "must be in vm state");
-  HandshakeOperation* op = _operation;
-  clear_handshake(thread);
-  if (op != NULL) {
-    op->cancel_handshake(thread);
-  }
-}
-
 bool HandshakeState::vmthread_can_process_handshake(JavaThread* target) {
   // SafepointSynchronize::safepoint_safe() does not consider an externally
   // suspended thread to be safe. However, this function must be called with
   // the Threads_lock held so an externally suspended thread cannot be
   // resumed thus it is safe.
   assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
   return SafepointSynchronize::safepoint_safe(target, target->thread_state()) ||
-         target->is_ext_suspended();
+         target->is_ext_suspended() || target->is_terminated();
 }
 
 static bool possibly_vmthread_can_process_handshake(JavaThread* target) {
   // An externally suspended thread cannot be resumed while the
   // Threads_lock is held so it is safe.
   // Note that this method is allowed to produce false positives.
   assert(Threads_lock->owned_by_self(), "Not holding Threads_lock.");
   if (target->is_ext_suspended()) {
     return true;
   }
+  if (target->is_terminated()) {
+    return true;
+  }
   switch (target->thread_state()) {
   case _thread_in_native:
     // native threads are safe if they have no java stack or have walkable stack
     return !target->has_last_Java_frame() || target->frame_anchor()->walkable();
 

@@ -379,10 +355,12 @@
   return false;
 }
 
 void HandshakeState::process_by_vmthread(JavaThread* target) {
   assert(Thread::current()->is_VM_thread(), "should call from vm thread");
+  // Threads_lock must be held here, but that is assert()ed in
+  // possibly_vmthread_can_process_handshake().
 
   if (!has_operation()) {
     // JT has already cleared its handshake
     return;
   }

@@ -400,11 +378,10 @@
   // If we own the semaphore at this point and while owning the semaphore
   // can observe a safe state the thread cannot possibly continue without
   // getting caught by the semaphore.
   if (vmthread_can_process_handshake(target)) {
     guarantee(!_semaphore.trywait(), "we should already own the semaphore");
-
     _operation->do_handshake(target);
     // Disarm after VM thread have executed the operation.
     clear_handshake(target);
     // Release the thread
   }