8271293: Monitor class should use ThreadBlockInVMPreprocess

Reviewed-by: dholmes, dcubed

src/hotspot/share/prims/jvmtiRawMonitor.cpp

@@ -332,7 +332,7 @@ void JvmtiRawMonitor::raw_enter(Thread* self) {
     for (;;) {
       ExitOnSuspend eos(this);
       {
-        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos);
+        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos, true /* allow_suspend */);
         simple_enter(jt);
       }
       if (!eos.monitor_exited()) {
@@ -384,7 +384,7 @@ int JvmtiRawMonitor::raw_wait(jlong millis, Thread* self) {
     for (;;) {
       ExitOnSuspend eos(this);
       {
-        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos);
+        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivmp(jt, eos, true /* allow_suspend */);
         simple_enter(jt);
       }
       if (!eos.monitor_exited()) {

src/hotspot/share/runtime/interfaceSupport.inline.hpp

@@ -239,13 +239,13 @@ class ThreadToNativeFromVM : public ThreadStateTransition {
 // SafepointMechanism::process_if_requested when returning to the VM. This allows us
 // to perform an "undo" action if we might block processing a safepoint/handshake operation
 // (such as thread suspension).
-template <typename PRE_PROC>
+template <typename PRE_PROC = void(JavaThread*)>
 class ThreadBlockInVMPreprocess : public ThreadStateTransition {
  private:
   PRE_PROC& _pr;
   bool _allow_suspend;
  public:
-  ThreadBlockInVMPreprocess(JavaThread* thread, PRE_PROC& pr, bool allow_suspend = true)
+  ThreadBlockInVMPreprocess(JavaThread* thread, PRE_PROC& pr = emptyOp, bool allow_suspend = false)
     : ThreadStateTransition(thread), _pr(pr), _allow_suspend(allow_suspend) {
     assert(thread->thread_state() == _thread_in_vm, "coming from wrong thread state");
     thread->check_possible_safepoint();
@@ -266,33 +266,12 @@ class ThreadBlockInVMPreprocess : public ThreadStateTransition {
 
     _thread->set_thread_state(_thread_in_vm);
   }
+
+  static void emptyOp(JavaThread* current) {}
 };
 
-class InFlightMutexRelease {
- private:
-  Mutex** _in_flight_mutex_addr;
- public:
-  InFlightMutexRelease(Mutex** in_flight_mutex_addr) : _in_flight_mutex_addr(in_flight_mutex_addr) {}
-  void operator()(JavaThread* current) {
-    if (_in_flight_mutex_addr != NULL && *_in_flight_mutex_addr != NULL) {
-      (*_in_flight_mutex_addr)->release_for_safepoint();
-      *_in_flight_mutex_addr = NULL;
-    }
-  }
-};
+typedef ThreadBlockInVMPreprocess<> ThreadBlockInVM;
 
-// Parameter in_flight_mutex_addr is only used by class Mutex to avoid certain deadlock
-// scenarios while making transitions that might block for a safepoint or handshake.
-// It's the address of a pointer to the mutex we are trying to acquire. This will be used to
-// access and release said mutex when transitioning back from blocked to vm (destructor) in
-// case we need to stop for a safepoint or handshake.
-class ThreadBlockInVM {
-  InFlightMutexRelease _ifmr;
-  ThreadBlockInVMPreprocess<InFlightMutexRelease> _tbivmpp;
- public:
-  ThreadBlockInVM(JavaThread* thread, Mutex** in_flight_mutex_addr = NULL)
-    : _ifmr(in_flight_mutex_addr), _tbivmpp(thread, _ifmr, /* allow_suspend= */ false) {}
-};
 
 // Debug class instantiated in JRT_ENTRY macro.
 // Can be used to verify properties on enter/exit of the VM.

src/hotspot/share/runtime/mutex.cpp

@@ -33,6 +33,20 @@
 #include "utilities/events.hpp"
 #include "utilities/macros.hpp"
 
+class InFlightMutexRelease {
+ private:
+  Mutex* _in_flight_mutex;
+ public:
+  InFlightMutexRelease(Mutex* in_flight_mutex) : _in_flight_mutex(in_flight_mutex) {
+    assert(in_flight_mutex != NULL, "must be");
+  }
+  void operator()(JavaThread* current) {
+    _in_flight_mutex->release_for_safepoint();
+    _in_flight_mutex = NULL;
+  }
+  bool not_released() { return _in_flight_mutex != NULL; }
+};
+
 #ifdef ASSERT
 void Mutex::check_block_state(Thread* thread) {
   if (!_allow_vm_block && thread->is_VM_thread()) {
@@ -72,7 +86,6 @@ void Mutex::check_no_safepoint_state(Thread* thread) {
 #endif // ASSERT
 
 void Mutex::lock_contended(Thread* self) {
-  Mutex *in_flight_mutex = NULL;
   DEBUG_ONLY(int retry_cnt = 0;)
   bool is_active_Java_thread = self->is_active_Java_thread();
   do {
@@ -84,13 +97,14 @@ void Mutex::lock_contended(Thread* self) {
 
     // Is it a JavaThread participating in the safepoint protocol.
     if (is_active_Java_thread) {
+      InFlightMutexRelease ifmr(this);
       assert(rank() > Mutex::special, "Potential deadlock with special or lesser rank mutex");
-      { ThreadBlockInVM tbivmdc(JavaThread::cast(self), &in_flight_mutex);
-        in_flight_mutex = this;  // save for ~ThreadBlockInVM
+      {
+        ThreadBlockInVMPreprocess<InFlightMutexRelease> tbivmdc(JavaThread::cast(self), ifmr);
         _lock.lock();
       }
-      if (in_flight_mutex != NULL) {
-        // Not unlocked by ~ThreadBlockInVM
+      if (ifmr.not_released()) {
+        // Not unlocked by ~ThreadBlockInVMPreprocess
         break;
       }
     } else {
@@ -234,18 +248,17 @@ bool Monitor::wait(int64_t timeout) {
   check_safepoint_state(self);
 
   int wait_status;
-  Mutex* in_flight_mutex = NULL;
+  InFlightMutexRelease ifmr(this);
 
   {
-    ThreadBlockInVM tbivmdc(self, &in_flight_mutex);
+    ThreadBlockInVMPreprocess<InFlightMutexRelease> tbivmdc(self, ifmr);
     OSThreadWaitState osts(self->osthread(), false /* not Object.wait() */);
 
     wait_status = _lock.wait(timeout);
-    in_flight_mutex = this;  // save for ~ThreadBlockInVM
   }
 
-  if (in_flight_mutex != NULL) {
-    // Not unlocked by ~ThreadBlockInVM
+  if (ifmr.not_released()) {
+    // Not unlocked by ~ThreadBlockInVMPreprocess
     assert_owner(NULL);
     // Conceptually reestablish ownership of the lock.
     set_owner(self);

src/hotspot/share/runtime/objectMonitor.cpp

@@ -430,7 +430,7 @@ bool ObjectMonitor::enter(JavaThread* current) {
     for (;;) {
       ExitOnSuspend eos(this);
       {
-        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivs(current, eos);
+        ThreadBlockInVMPreprocess<ExitOnSuspend> tbivs(current, eos, true /* allow_suspend */);
         EnterI(current);
         current->set_current_pending_monitor(NULL);
         // We can go to a safepoint at the end of this block. If we
@@ -975,7 +975,7 @@ void ObjectMonitor::ReenterI(JavaThread* current, ObjectWaiter* currentNode) {
 
       {
         ClearSuccOnSuspend csos(this);
-        ThreadBlockInVMPreprocess<ClearSuccOnSuspend> tbivs(current, csos);
+        ThreadBlockInVMPreprocess<ClearSuccOnSuspend> tbivs(current, csos, true /* allow_suspend */);
         current->_ParkEvent->park();
       }
     }
@@ -1536,7 +1536,7 @@ void ObjectMonitor::wait(jlong millis, bool interruptible, TRAPS) {
 
     {
       ClearSuccOnSuspend csos(this);
-      ThreadBlockInVMPreprocess<ClearSuccOnSuspend> tbivs(current, csos);
+      ThreadBlockInVMPreprocess<ClearSuccOnSuspend> tbivs(current, csos, true /* allow_suspend */);
       if (interrupted || HAS_PENDING_EXCEPTION) {
         // Intentionally empty
       } else if (node._notified == 0) {