8265753: Remove manual JavaThread transitions to blocked

Reviewed-by: dcubed, rrich, dholmes, pchilanomate

Author: robehn

src/hotspot/share/prims/jvmtiEnv.cpp

@@ -3191,31 +3191,7 @@ JvmtiEnv::RawMonitorEnter(JvmtiRawMonitor * rmonitor) {
     // in thread.cpp.
     JvmtiPendingMonitors::enter(rmonitor);
   } else {
-    Thread* thread = Thread::current();
-    if (thread->is_Java_thread()) {
-      JavaThread* current_thread = thread->as_Java_thread();
-
-      /* Transition to thread_blocked without entering vm state          */
-      /* This is really evil. Normally you can't undo _thread_blocked    */
-      /* transitions like this because it would cause us to miss a       */
-      /* safepoint but since the thread was already in _thread_in_native */
-      /* the thread is not leaving a safepoint safe state and it will    */
-      /* block when it tries to return from native. We can't safepoint   */
-      /* block in here because we could deadlock the vmthread. Blech.    */
-
-      JavaThreadState state = current_thread->thread_state();
-      assert(state == _thread_in_native, "Must be _thread_in_native");
-      // frame should already be walkable since we are in native
-      assert(!current_thread->has_last_Java_frame() ||
-             current_thread->frame_anchor()->walkable(), "Must be walkable");
-      current_thread->set_thread_state(_thread_blocked);
-
-      rmonitor->raw_enter(current_thread);
-      // restore state, still at a safepoint safe state
-      current_thread->set_thread_state(state);
-    } else {
-      rmonitor->raw_enter(thread);
-    }
+    rmonitor->raw_enter(Thread::current());
   }
   return JVMTI_ERROR_NONE;
 } /* end RawMonitorEnter */

src/hotspot/share/prims/jvmtiRawMonitor.cpp

@@ -43,10 +43,12 @@ void JvmtiPendingMonitors::transition_raw_monitors() {
          "Java thread has not been created yet or more than one java thread "
          "is running. Raw monitor transition will not work");
   JavaThread* current_java_thread = JavaThread::current();
-  assert(current_java_thread->thread_state() == _thread_in_vm, "Must be in vm");
-  for (int i = 0; i < count(); i++) {
-    JvmtiRawMonitor* rmonitor = monitors()->at(i);
-    rmonitor->raw_enter(current_java_thread);
+  {
+    ThreadToNativeFromVM ttnfvm(current_java_thread);
+    for (int i = 0; i < count(); i++) {
+      JvmtiRawMonitor* rmonitor = monitors()->at(i);
+      rmonitor->raw_enter(current_java_thread);
+    }
   }
   // pending monitors are converted to real monitor so delete them all.
   dispose();
@@ -60,7 +62,6 @@ JvmtiRawMonitor::JvmtiRawMonitor(const char* name) : _owner(NULL),
                                                      _recursions(0),
                                                      _entry_list(NULL),
                                                      _wait_set(NULL),
-                                                     _waiters(0),
                                                      _magic(JVMTI_RM_MAGIC),
                                                      _name(NULL) {
 #ifdef ASSERT
@@ -217,11 +218,12 @@ inline void JvmtiRawMonitor::dequeue_waiter(QNode& node) {
 // simple_wait is not quite so simple as we have to deal with the interaction
 // with the Thread interrupt state, which resides in the java.lang.Thread object.
 // That state must only be accessed while _thread_in_vm and requires proper thread-state
-// transitions. However, we cannot perform such transitions whilst we hold the RawMonitor,
-// else we can deadlock with the VMThread (which may also use RawMonitors as part of
-// executing various callbacks).
+// transitions.
 // Returns M_OK usually, but M_INTERRUPTED if the thread is a JavaThread and was
 // interrupted.
+// Note:
+//  - simple_wait never reenters the monitor.
+//  - A JavaThread must be in native.
 int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
   guarantee(_owner == self  , "invariant");
   guarantee(_recursions == 0, "invariant");
@@ -235,21 +237,24 @@ int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
   int ret = M_OK;
   if (self->is_Java_thread()) {
     JavaThread* jt = self->as_Java_thread();
-    // Transition to VM so we can check interrupt state
-    ThreadInVMfromNative tivm(jt);
-    if (jt->is_interrupted(true)) {
+    guarantee(jt->thread_state() == _thread_in_native, "invariant");
+    {
+      // This transition must be after we exited the monitor.
+      ThreadInVMfromNative tivmfn(jt);
+      if (jt->is_interrupted(true)) {
         ret = M_INTERRUPTED;
-    } else {
-      ThreadBlockInVM tbivm(jt);
-      if (millis <= 0) {
-        self->_ParkEvent->park();
       } else {
-        self->_ParkEvent->park(millis);
+        ThreadBlockInVM tbivm(jt);
+        if (millis <= 0) {
+          self->_ParkEvent->park();
+        } else {
+          self->_ParkEvent->park(millis);
+        }
+        // Return to VM before post-check of interrupt state
+      }
+      if (jt->is_interrupted(true)) {
+        ret = M_INTERRUPTED;
       }
-      // Return to VM before post-check of interrupt state
-    }
-    if (jt->is_interrupted(true)) {
-      ret = M_INTERRUPTED;
     }
   } else {
     if (millis <= 0) {
@@ -261,10 +266,6 @@ int JvmtiRawMonitor::simple_wait(Thread* self, jlong millis) {
 
   dequeue_waiter(node);
 
-  simple_enter(self);
-  guarantee(_owner == self, "invariant");
-  guarantee(_recursions == 0, "invariant");
-
   return ret;
 }
 
@@ -306,75 +307,37 @@ void JvmtiRawMonitor::simple_notify(Thread* self, bool all) {
   return;
 }
 
-// Any JavaThread will enter here with state _thread_blocked unless we
-// are in single-threaded mode during startup.
-void JvmtiRawMonitor::raw_enter(Thread* self) {
-  void* contended;
-  JavaThread* jt = NULL;
-  // don't enter raw monitor if thread is being externally suspended, it will
-  // surprise the suspender if a "suspended" thread can still enter monitor
-  if (self->is_Java_thread()) {
-    jt = self->as_Java_thread();
-    while (true) {
-      // To pause suspend requests while in blocked we must block handshakes.
-      jt->handshake_state()->lock();
-      // Suspend request flag can only be set in handshakes.
-      // By blocking handshakes, suspend request flag cannot change its value.
-      if (!jt->handshake_state()->is_suspended()) {
-        contended = Atomic::cmpxchg(&_owner, (Thread*)NULL, jt);
-        jt->handshake_state()->unlock();
-        break;
-      }
-      jt->handshake_state()->unlock();
-
-      // We may only be in states other than _thread_blocked when we are
-      // in single-threaded mode during startup.
-      guarantee(jt->thread_state() == _thread_blocked, "invariant");
-
-      jt->set_thread_state_fence(_thread_blocked_trans);
-      SafepointMechanism::process_if_requested(jt);
-      // We should transition to thread_in_vm and then to thread_in_vm_trans,
-      // but those are always treated the same as _thread_blocked_trans.
-      jt->set_thread_state(_thread_blocked);
-    }
-  } else {
-    contended = Atomic::cmpxchg(&_owner, (Thread*)NULL, self);
-  }
+void JvmtiRawMonitor::ExitOnSuspend::operator()(JavaThread* current) {
+  // We must exit the monitor in case of a safepoint.
+  _rm->simple_exit(current);
+  _rm_exited = true;
+}
 
-  if (contended == self) {
+// JavaThreads will enter here with state _thread_in_native.
+void JvmtiRawMonitor::raw_enter(Thread* self) {
+  // TODO Atomic::load on _owner field
+  if (_owner == self) {
     _recursions++;
     return;
   }
 
-  if (contended == NULL) {
-    guarantee(_owner == self, "invariant");
-    guarantee(_recursions == 0, "invariant");
-    return;
-  }
-
   self->set_current_pending_raw_monitor(this);
 
   if (!self->is_Java_thread()) {
     simple_enter(self);
   } else {
-    // In multi-threaded mode, we must enter this method blocked.
-    guarantee(jt->thread_state() == _thread_blocked, "invariant");
+    JavaThread* jt = self->as_Java_thread();
+    guarantee(jt->thread_state() == _thread_in_native, "invariant");
+    ThreadInVMfromNative tivmfn(jt);
     for (;;) {
-      simple_enter(jt);
-      if (!SafepointMechanism::should_process(jt)) {
-        // Not suspended so we're done here.
-        break;
+      ExitOnSuspend eos(this);
+      {
+        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos);
+        simple_enter(jt);
       }
-      if (!jt->is_suspended()) {
-        // Not suspended so we're done here.
+      if (!eos.monitor_exited()) {
         break;
       }
-      simple_exit(jt);
-      jt->set_thread_state_fence(_thread_blocked_trans);
-      SafepointMechanism::process_if_requested(jt);
-      // We should transition to thread_in_vm and then to thread_in_vm_trans,
-      // but those are always treated the same as _thread_blocked_trans.
-      jt->set_thread_state(_thread_blocked);
     }
   }
 
@@ -411,37 +374,34 @@ int JvmtiRawMonitor::raw_wait(jlong millis, Thread* self) {
 
   intptr_t save = _recursions;
   _recursions = 0;
-  _waiters++;
   ret = simple_wait(self, millis);
-  _recursions = save;
-  _waiters--;
-
-  guarantee(self == _owner, "invariant");
 
-  if (self->is_Java_thread()) {
+  // Now we need to re-enter the monitor. For JavaThreads
+  // we need to manage suspend requests.
+  if (self->is_Java_thread()) { // JavaThread re-enter
     JavaThread* jt = self->as_Java_thread();
-    guarantee(jt->thread_state() == _thread_in_native, "invariant");
+    ThreadInVMfromNative tivmfn(jt);
     for (;;) {
-      if (!SafepointMechanism::should_process(jt)) {
-        // Not suspended so we're done here:
-        break;
+      ExitOnSuspend eos(this);
+      {
+        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos);
+        simple_enter(jt);
       }
-      simple_exit(jt);
-      jt->set_thread_state_fence(_thread_in_native_trans);
-      SafepointMechanism::process_if_requested(jt);
-      if (jt->is_interrupted(true)) {
-        ret = M_INTERRUPTED;
+      if (!eos.monitor_exited()) {
+        break;
       }
-      // We should transition to thread_in_vm and then to thread_in_vm_trans,
-      // but those are always treated the same as _thread_in_native_trans.
-      jt->set_thread_state(_thread_in_native);
-      simple_enter(jt);
     }
-    guarantee(jt == _owner, "invariant");
-  } else {
+    if (jt->is_interrupted(true)) {
+      ret = M_INTERRUPTED;
+    }
+  } else { // Non-JavaThread re-enter
     assert(ret != M_INTERRUPTED, "Only JavaThreads can be interrupted");
+    simple_enter(self);
   }
 
+  _recursions = save;
+
+  guarantee(self == _owner, "invariant");
   return ret;
 }
 

src/hotspot/share/prims/jvmtiRawMonitor.hpp

@@ -37,6 +37,21 @@
 // A simplified version of the ObjectMonitor code.
 //
 
+// Important note:
+// Raw monitors can be used in callbacks which happen during safepoint by the VM
+// thread (e.g., heapRootCallback). This means we may not transition/safepoint
+// poll in many cases, else the agent JavaThread can deadlock with the VM thread,
+// as this old comment says:
+// "We can't safepoint block in here because we could deadlock the vmthread. Blech."
+// The rules are:
+// - We must never safepoint poll if raw monitor is owned.
+// - We may safepoint poll before it is owned and after it has been released.
+// If this were the only thing we needed to think about we could just stay in
+// native for all operations. However we need to honor a suspend request, not
+// entering a monitor if suspended, and check for interrupts. Honoring a suspend
+// request and reading the interrupt flag must be done from VM state
+// (a safepoint unsafe state).
+
 class JvmtiRawMonitor : public CHeapObj<mtSynchronizer>  {
 
   // Helper class to allow Threads to be linked into queues.
@@ -59,7 +74,6 @@ class JvmtiRawMonitor : public CHeapObj<mtSynchronizer>  {
                                 // The list is actually composed of nodes,
                                 // acting as proxies for Threads.
   QNode* volatile _wait_set;    // Threads wait()ing on the monitor
-  volatile jint _waiters;       // number of waiting threads
   int _magic;
   char* _name;
   // JVMTI_RM_MAGIC is set in contructor and unset in destructor.
@@ -75,6 +89,16 @@ class JvmtiRawMonitor : public CHeapObj<mtSynchronizer>  {
   int simple_wait(Thread* self, jlong millis);
   void simple_notify(Thread* self, bool all);
 
+  class ExitOnSuspend {
+   protected:
+    JvmtiRawMonitor* _rm;
+    bool _rm_exited;
+   public:
+    ExitOnSuspend(JvmtiRawMonitor* rm) : _rm(rm), _rm_exited(false) {}
+    void operator()(JavaThread* current);
+    bool monitor_exited() { return _rm_exited; }
+  };
+
  public:
 
   // return codes

src/hotspot/share/runtime/handshake.cpp

@@ -600,14 +600,6 @@ HandshakeState::ProcessResult HandshakeState::try_process(HandshakeOperation* ma
   return pr_ret;
 }
 
-void HandshakeState::lock() {
-  _lock.lock_without_safepoint_check();
-}
-
-void HandshakeState::unlock() {
-  _lock.unlock();
-}
-
 void HandshakeState::do_self_suspend() {
   assert(Thread::current() == _handshakee, "should call from _handshakee");
   assert(_lock.owned_by_self(), "Lock must be held");

src/hotspot/share/runtime/handshake.hpp

@@ -73,7 +73,6 @@ class JvmtiRawMonitor;
 // operation is only done by either VMThread/Handshaker on behalf of the
 // JavaThread or by the target JavaThread itself.
 class HandshakeState {
-  friend JvmtiRawMonitor;
   friend ThreadSelfSuspensionHandshake;
   friend SuspendThreadHandshake;
   friend JavaThread;
@@ -103,9 +102,6 @@ class HandshakeState {
   HandshakeOperation* pop_for_self();
   HandshakeOperation* pop();
 
-  void lock();
-  void unlock();
-
  public:
   HandshakeState(JavaThread* thread);