8184732: Deadlock detection improvements for 'special' locks
authorcoleenp
Mon, 30 Sep 2019 13:10:11 -0400
changeset 58409 a595e67d6683
parent 58407 b14643d898d3
child 58410 a074e637aeee
8184732: Deadlock detection improvements for 'special' locks Summary: Assert that special ranked locks cannot safepoint and allow_vm_block and remove locks from the exceptional lock list in no_safepoint_verifier. Reviewed-by: dholmes, eosterlund
src/hotspot/share/oops/instanceKlass.cpp
src/hotspot/share/prims/whitebox.cpp
src/hotspot/share/runtime/mutex.cpp
src/hotspot/share/runtime/mutexLocker.cpp
src/hotspot/share/runtime/vmThread.cpp
test/hotspot/jtreg/runtime/Safepoint/NoSafepointVerifier.java
test/lib/sun/hotspot/WhiteBox.java
--- a/src/hotspot/share/oops/instanceKlass.cpp	Mon Sep 30 18:02:24 2019 +0200
+++ b/src/hotspot/share/oops/instanceKlass.cpp	Mon Sep 30 13:10:11 2019 -0400
@@ -1101,7 +1101,7 @@
 
 
 void InstanceKlass::set_implementor(Klass* k) {
-  assert_lock_strong(Compile_lock);
+  assert_locked_or_safepoint(Compile_lock);
   assert(is_interface(), "not interface");
   Klass* volatile* addr = adr_implementor();
   assert(addr != NULL, "null addr");
@@ -2333,8 +2333,8 @@
   // being added to class hierarchy (see SystemDictionary:::add_to_hierarchy()).
   _init_state = allocated;
 
-  {
-    MutexLocker ml(Compile_lock);
+  { // Otherwise this needs to take out the Compile_lock.
+    assert(SafepointSynchronize::is_at_safepoint(), "only called at safepoint");
     init_implementor();
   }
 
--- a/src/hotspot/share/prims/whitebox.cpp	Mon Sep 30 18:02:24 2019 +0200
+++ b/src/hotspot/share/prims/whitebox.cpp	Mon Sep 30 13:10:11 2019 -0400
@@ -1745,6 +1745,20 @@
                  sfpt_check_attempted);
 WB_END
 
+WB_ENTRY(void, WB_AssertSpecialLock(JNIEnv* env, jobject o, jboolean allowVMBlock, jboolean safepointCheck))
+  // Create a special lock violating condition in value
+  Mutex::SafepointCheckRequired sfpt_check_required = safepointCheck ?
+                                           Mutex::_safepoint_check_always :
+                                           Mutex::_safepoint_check_never;
+  Mutex::SafepointCheckFlag safepoint_check = safepointCheck ?
+                                           Monitor::_safepoint_check_flag :
+                                           Monitor::_no_safepoint_check_flag;
+
+  MutexLocker ml(new Mutex(Mutex::special, "SpecialTest_lock", allowVMBlock, sfpt_check_required), safepoint_check);
+  // If the lock above succeeds, try to safepoint to test the NSV implied with this special lock.
+  ThreadBlockInVM tbivm(JavaThread::current());
+WB_END
+
 WB_ENTRY(jboolean, WB_IsMonitorInflated(JNIEnv* env, jobject wb, jobject obj))
   oop obj_oop = JNIHandles::resolve(obj);
   return (jboolean) obj_oop->mark().has_monitor();
@@ -2322,6 +2336,7 @@
   {CC"AddModuleExportsToAll", CC"(Ljava/lang/Object;Ljava/lang/String;)V",
                                                       (void*)&WB_AddModuleExportsToAll },
   {CC"assertMatchingSafepointCalls", CC"(ZZ)V",       (void*)&WB_AssertMatchingSafepointCalls },
+  {CC"assertSpecialLock",  CC"(ZZ)V",                 (void*)&WB_AssertSpecialLock },
   {CC"isMonitorInflated0", CC"(Ljava/lang/Object;)Z", (void*)&WB_IsMonitorInflated  },
   {CC"forceSafepoint",     CC"()V",                   (void*)&WB_ForceSafepoint     },
   {CC"getConstantPool0",   CC"(Ljava/lang/Class;)J",  (void*)&WB_GetConstantPool    },
--- a/src/hotspot/share/runtime/mutex.cpp	Mon Sep 30 18:02:24 2019 +0200
+++ b/src/hotspot/share/runtime/mutex.cpp	Mon Sep 30 13:10:11 2019 -0400
@@ -282,6 +282,11 @@
 
   assert(_safepoint_check_required != _safepoint_check_sometimes || is_sometimes_ok(name),
          "Lock has _safepoint_check_sometimes %s", name);
+
+  assert(_rank > special || _allow_vm_block,
+         "Special locks or below should allow the vm to block");
+  assert(_rank > special || _safepoint_check_required == _safepoint_check_never,
+         "Special locks or below should never safepoint");
 #endif
 }
 
@@ -388,17 +393,13 @@
 
 // NSV implied with locking allow_vm_block or !safepoint_check locks.
 void Mutex::no_safepoint_verifier(Thread* thread, bool enable) {
-  // Threads_lock is special, since the safepoint synchronization will not start before this is
-  // acquired. Hence, a JavaThread cannot be holding it at a safepoint. So is VMOperationRequest_lock,
-  // since it is used to transfer control between JavaThreads and the VMThread
-  // Do not *exclude* any locks unless you are absolutely sure it is correct. Ask someone else first!
-  if ((_allow_vm_block &&
-       this != Threads_lock &&
-       this != Compile_lock &&           // Temporary: should not be necessary when we get separate compilation
-       this != tty_lock &&               // The tty_lock is released for the safepoint.
-       this != VMOperationRequest_lock &&
-       this != VMOperationQueue_lock) ||
-       rank() == Mutex::special) {
+  // The tty_lock is special because it is released for the safepoint by
+  // the safepoint mechanism.
+  if (this == tty_lock) {
+    return;
+  }
+
+  if (_allow_vm_block) {
     if (enable) {
       thread->_no_safepoint_count++;
     } else {
--- a/src/hotspot/share/runtime/mutexLocker.cpp	Mon Sep 30 18:02:24 2019 +0200
+++ b/src/hotspot/share/runtime/mutexLocker.cpp	Mon Sep 30 13:10:11 2019 -0400
@@ -288,7 +288,7 @@
   def(JvmtiThreadState_lock        , PaddedMutex  , nonleaf+2,   false, _safepoint_check_always); // Used by JvmtiThreadState/JvmtiEventController
   def(Management_lock              , PaddedMutex  , nonleaf+2,   false, _safepoint_check_always); // used for JVM management
 
-  def(Compile_lock                 , PaddedMutex  , nonleaf+3,   true,  _safepoint_check_always);
+  def(Compile_lock                 , PaddedMutex  , nonleaf+3,   false, _safepoint_check_always);
   def(MethodData_lock              , PaddedMutex  , nonleaf+3,   false, _safepoint_check_always);
   def(TouchedMethodLog_lock        , PaddedMutex  , nonleaf+3,   false, _safepoint_check_always);
 
--- a/src/hotspot/share/runtime/vmThread.cpp	Mon Sep 30 18:02:24 2019 +0200
+++ b/src/hotspot/share/runtime/vmThread.cpp	Mon Sep 30 13:10:11 2019 -0400
@@ -354,9 +354,9 @@
 void VMThread::wait_for_vm_thread_exit() {
   assert(Thread::current()->is_Java_thread(), "Should be a JavaThread");
   assert(((JavaThread*)Thread::current())->is_terminated(), "Should be terminated");
-  { MutexLocker mu(VMOperationQueue_lock, Mutex::_no_safepoint_check_flag);
+  { MonitorLocker mu(VMOperationQueue_lock, Mutex::_no_safepoint_check_flag);
     _should_terminate = true;
-    VMOperationQueue_lock->notify();
+    mu.notify();
   }
 
   // Note: VM thread leaves at Safepoint. We are not stopped by Safepoint
@@ -620,8 +620,8 @@
 
     //
     //  Notify (potential) waiting Java thread(s)
-    { MutexLocker mu(VMOperationRequest_lock, Mutex::_no_safepoint_check_flag);
-      VMOperationRequest_lock->notify_all();
+    { MonitorLocker mu(VMOperationRequest_lock, Mutex::_no_safepoint_check_flag);
+      mu.notify_all();
     }
 
     // We want to make sure that we get to a safepoint regularly
@@ -695,12 +695,11 @@
     // VMOperationQueue_lock, so we can block without a safepoint check. This allows vm operation requests
     // to be queued up during a safepoint synchronization.
     {
-      VMOperationQueue_lock->lock_without_safepoint_check();
+      MonitorLocker ml(VMOperationQueue_lock, Mutex::_no_safepoint_check_flag);
       log_debug(vmthread)("Adding VM operation: %s", op->name());
       _vm_queue->add(op);
       op->set_timestamp(os::javaTimeMillis());
-      VMOperationQueue_lock->notify();
-      VMOperationQueue_lock->unlock();
+      ml.notify();
     }
 
     if (!concurrent) {
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/test/hotspot/jtreg/runtime/Safepoint/NoSafepointVerifier.java	Mon Sep 30 13:10:11 2019 -0400
@@ -0,0 +1,78 @@
+/*
+ * Copyright (c) 2018, 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug 8184732
+ * @summary Ensure that special locks never safepoint check and are vm_block.
+ * @library /test/lib
+ * @modules java.base/jdk.internal.misc
+ *          java.management
+ * @build sun.hotspot.WhiteBox
+ * @run driver ClassFileInstaller sun.hotspot.WhiteBox
+ *                              sun.hotspot.WhiteBox$WhiteBoxPermission
+ * @run main NoSafepointVerifier
+ */
+
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+import jdk.test.lib.Platform;
+
+import sun.hotspot.WhiteBox;
+
+public class NoSafepointVerifier {
+
+    static void runTest(String test) throws Exception {
+        if (Platform.isDebugBuild()){
+            ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+                  "-Xbootclasspath/a:.",
+                  "-XX:+UnlockDiagnosticVMOptions",
+                  "-XX:+WhiteBoxAPI",
+                  "-XX:-CreateCoredumpOnCrash",
+                  "NoSafepointVerifier",
+                  test);
+            OutputAnalyzer output = new OutputAnalyzer(pb.start());
+            output.shouldContain(test);
+        }
+    }
+
+    static String test1 = "Special locks or below should never safepoint";
+    static String test2 = "Special locks or below should allow the vm to block";
+    static String test3 = "Possible safepoint reached by thread that does not allow it";
+
+    public static void main(String args[]) throws Exception {
+        if (args.length > 0) {
+            if (args[0].equals(test1)) {
+                WhiteBox.getWhiteBox().assertSpecialLock(/*vm_block*/true, /*safepoint_check_always*/true);
+            } else if (args[0].equals(test2)) {
+                WhiteBox.getWhiteBox().assertSpecialLock(/*vm_block*/false, /*safepoint_check_always*/false);
+            } else if (args[0].equals(test3)) {
+                WhiteBox.getWhiteBox().assertSpecialLock(/*vm_block*/true, /*safepoint_check_always*/false);
+            }
+        } else {
+            runTest(test1);
+            runTest(test2);
+            runTest(test3);
+        }
+    }
+}
--- a/test/lib/sun/hotspot/WhiteBox.java	Mon Sep 30 18:02:24 2019 +0200
+++ b/test/lib/sun/hotspot/WhiteBox.java	Mon Sep 30 13:10:11 2019 -0400
@@ -511,6 +511,7 @@
 
   // Safepoint Checking
   public native void assertMatchingSafepointCalls(boolean mutexSafepointValue, boolean attemptedNoSafepointValue);
+  public native void assertSpecialLock(boolean allowVMBlock, boolean safepointCheck);
 
   // Sharing & archiving
   public native String  getDefaultArchivePath();