8232733: Remove need to grab Threads_lock while processing handshakes

Threads_lock is not acquired anymore while processing handshakes

Reviewed-by: coleenp, rehn, dcubed, dholmes

src/hotspot/share/runtime/handshake.cpp

@@ -147,14 +147,7 @@ class VM_HandshakeOneThread: public VM_Handshake {
       if (handshake_has_timed_out(timeout_start_time)) {
         handle_timeout();
       }
-
-      // 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.
-      {
-        MutexLocker ml(Threads_lock);
-        by_vm_thread = _target->handshake_try_process_by_vmThread();
-      }
+      by_vm_thread = _target->handshake_try_process_by_vmThread();
     } while (!poll_for_completed_thread());
     DEBUG_ONLY(_op->check_state();)
     log_handshake_info(start_time_ns, _op->name(), 1, by_vm_thread ? 1 : 0);
@@ -202,21 +195,14 @@ class VM_HandshakeAllThreads: public VM_Handshake {
       // Have VM thread perform the handshake operation for blocked threads.
       // Observing a blocked state may of course be transient but the processing is guarded
       // 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();
-          MutexLocker ml(Threads_lock);
-          for (JavaThread *thr = jtiwh.next(); thr != NULL; thr = jtiwh.next()) {
-            // A new thread on the ThreadsList will not have an operation,
-            // hence it is skipped in handshake_process_by_vmthread.
-            if (thr->handshake_try_process_by_vmThread()) {
-              handshake_executed_by_vm_thread++;
-            }
-          }
+      jtiwh.rewind();
+      for (JavaThread *thr = jtiwh.next(); thr != NULL; thr = jtiwh.next()) {
+        // A new thread on the ThreadsList will not have an operation,
+        // hence it is skipped in handshake_process_by_vmthread.
+        if (thr->handshake_try_process_by_vmThread()) {
+          handshake_executed_by_vm_thread++;
+        }
       }
-
       while (poll_for_completed_thread()) {
         // Includes canceled operations by exiting threads.
         number_of_threads_completed++;
@@ -279,6 +265,8 @@ void HandshakeThreadsOperation::do_handshake(JavaThread* thread) {
 
   // Use the semaphore to inform the VM thread that we have completed the operation
   _done.signal();
+
+  // It is no longer safe to refer to 'this' as the VMThread may have destroyed this operation
 }
 
 void Handshake::execute(HandshakeClosure* thread_cl) {
@@ -305,7 +293,9 @@ bool Handshake::execute(HandshakeClosure* thread_cl, JavaThread* target) {
   }
 }
 
-HandshakeState::HandshakeState() : _operation(NULL), _semaphore(1), _thread_in_process_handshake(false) {}
+HandshakeState::HandshakeState() : _operation(NULL), _semaphore(1), _thread_in_process_handshake(false) {
+  DEBUG_ONLY(_vmthread_processing_handshake = false;)
+}
 
 void HandshakeState::set_operation(JavaThread* target, HandshakeOperation* op) {
   _operation = op;
@@ -347,10 +337,7 @@ bool HandshakeState::vmthread_can_process_handshake(JavaThread* target) {
 }
 
 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;
   }
@@ -383,8 +370,6 @@ bool HandshakeState::claim_handshake_for_vmthread() {
 
 bool HandshakeState::try_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
@@ -408,7 +393,9 @@ bool HandshakeState::try_process_by_vmThread(JavaThread* target) {
   if (vmthread_can_process_handshake(target)) {
     guarantee(!_semaphore.trywait(), "we should already own the semaphore");
     log_trace(handshake)("Processing handshake by VMThtread");
+    DEBUG_ONLY(_vmthread_processing_handshake = true;)
     _operation->do_handshake(target);
+    DEBUG_ONLY(_vmthread_processing_handshake = false;)
     // Disarm after VM thread have executed the operation.
     clear_handshake(target);
     executed = true;

src/hotspot/share/runtime/handshake.hpp

@@ -89,6 +89,12 @@ public:
   }
 
   bool try_process_by_vmThread(JavaThread* target);
+
+#ifdef ASSERT
+  bool _vmthread_processing_handshake;
+  bool is_vmthread_processing_handshake() const { return _vmthread_processing_handshake; }
+#endif
+
 };
 
 #endif // SHARE_RUNTIME_HANDSHAKE_HPP

src/hotspot/share/runtime/mutexLocker.cpp

@@ -187,6 +187,11 @@ void assert_lock_strong(const Mutex* lock) {
   if (lock->owned_by_self()) return;
   fatal("must own lock %s", lock->name());
 }
+
+void assert_locked_or_safepoint_or_handshake(const Mutex* lock, const JavaThread* thread) {
+  if (Thread::current()->is_VM_thread() && thread->is_vmthread_processing_handshake()) return;
+  assert_locked_or_safepoint(lock);
+}
 #endif
 
 #define def(var, type, pri, vm_block, safepoint_check_allowed ) {      \

src/hotspot/share/runtime/mutexLocker.hpp

@@ -174,15 +174,17 @@ void print_owned_locks_on_error(outputStream* st);
 
 char *lock_name(Mutex *mutex);
 
-// for debugging: check that we're already owning this lock (or are at a safepoint)
+// for debugging: check that we're already owning this lock (or are at a safepoint / handshake)
 #ifdef ASSERT
 void assert_locked_or_safepoint(const Mutex* lock);
 void assert_locked_or_safepoint_weak(const Mutex* lock);
 void assert_lock_strong(const Mutex* lock);
+void assert_locked_or_safepoint_or_handshake(const Mutex* lock, const JavaThread* thread);
 #else
 #define assert_locked_or_safepoint(lock)
 #define assert_locked_or_safepoint_weak(lock)
 #define assert_lock_strong(lock)
+#define assert_locked_or_safepoint_or_handshake(lock, thread)
 #endif
 
 class MutexLocker: public StackObj {

src/hotspot/share/runtime/safepoint.cpp

@@ -736,10 +736,7 @@ static bool safepoint_safe_with(JavaThread *thread, JavaThreadState state) {
 }
 
 bool SafepointSynchronize::handshake_safe(JavaThread *thread) {
-  // 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() && Thread::current()->is_VM_thread(),
-         "Must hold Threads_lock and be VMThread");
+  assert(Thread::current()->is_VM_thread(), "Must be VMThread");
   if (thread->is_ext_suspended() || thread->is_terminated()) {
     return true;
   }

src/hotspot/share/runtime/thread.cpp

@@ -3156,7 +3156,7 @@ const char* JavaThread::get_thread_name() const {
     if (!(cur->is_Java_thread() && cur == this)) {
       // Current JavaThreads are allowed to get their own name without
       // the Threads_lock.
-      assert_locked_or_safepoint(Threads_lock);
+      assert_locked_or_safepoint_or_handshake(Threads_lock, this);
     }
   }
 #endif // ASSERT

src/hotspot/share/runtime/thread.hpp

@@ -1333,6 +1333,12 @@ class JavaThread: public Thread {
     return _handshake.try_process_by_vmThread(this);
   }
 
+#ifdef ASSERT
+  bool is_vmthread_processing_handshake() const {
+    return _handshake.is_vmthread_processing_handshake();
+  }
+#endif
+
   // Suspend/resume support for JavaThread
  private:
   inline void set_ext_suspended();