RFR: 8210832: Remove sneaky locking in class Monitor

[Robbin Ehn]

Hi Patricio,

Mostly looks good!

block_at_safepoint is always called with block_in_safepoint_check = true. (correct?)
Changing that to a local state instead of global simplifies the code.

So I'm suggesting something like below.

Thanks, Robbin

diff -r e65cc445234c src/hotspot/share/runtime/interfaceSupport.inline.hpp
--- a/src/hotspot/share/runtime/interfaceSupport.inline.hpp	Mon Jan 28 13:10:15 
2019 +0100
+++ b/src/hotspot/share/runtime/interfaceSupport.inline.hpp	Mon Jan 28 14:10:59 
2019 +0100
@@ -308,2 +308,1 @@
-    thread->block_in_safepoint_check = false;
-    SafepointMechanism::block_at_safepoint(thread);
+    SafepointMechanism::callback_if_safepoint(thread);
@@ -323,2 +322,1 @@
-      SafepointMechanism::block_at_safepoint(_thread);
-      _thread->block_in_safepoint_check = true;
+      SafepointMechanism::callback_if_safepoint(_thread);
@@ -335,2 +332,0 @@
-    } else {
-      _thread->block_in_safepoint_check = true;
@@ -337,0 +334,1 @@
+
diff -r e65cc445234c src/hotspot/share/runtime/safepoint.cpp
--- a/src/hotspot/share/runtime/safepoint.cpp	Mon Jan 28 13:10:15 2019 +0100
+++ b/src/hotspot/share/runtime/safepoint.cpp	Mon Jan 28 14:10:59 2019 +0100
@@ -795,1 +795,1 @@
-void SafepointSynchronize::block(JavaThread *thread) {
+void SafepointSynchronize::block(JavaThread *thread, bool 
block_in_safepoint_check) {
@@ -850,1 +850,1 @@
-      if (thread->block_in_safepoint_check) {
+      if (block_in_safepoint_check) {
@@ -880,1 +880,1 @@
-          thread->block_in_safepoint_check) {
+          block_in_safepoint_check) {
diff -r e65cc445234c src/hotspot/share/runtime/safepoint.hpp
--- a/src/hotspot/share/runtime/safepoint.hpp	Mon Jan 28 13:10:15 2019 +0100
+++ b/src/hotspot/share/runtime/safepoint.hpp	Mon Jan 28 14:10:59 2019 +0100
@@ -146,1 +146,1 @@
-  static void   block(JavaThread *thread);
+  static void   block(JavaThread *thread, bool block_in_safepoint_check = true);
diff -r e65cc445234c src/hotspot/share/runtime/safepointMechanism.hpp
--- a/src/hotspot/share/runtime/safepointMechanism.hpp	Mon Jan 28 13:10:15 2019 
+0100
+++ b/src/hotspot/share/runtime/safepointMechanism.hpp	Mon Jan 28 14:10:59 2019 
+0100
@@ -82,1 +82,1 @@
-  static inline void block_at_safepoint(JavaThread* thread);
+  static inline void callback_if_safepoint(JavaThread* thread);
diff -r e65cc445234c src/hotspot/share/runtime/safepointMechanism.inline.hpp
--- a/src/hotspot/share/runtime/safepointMechanism.inline.hpp	Mon Jan 28 
13:10:15 2019 +0100
+++ b/src/hotspot/share/runtime/safepointMechanism.inline.hpp	Mon Jan 28 
14:10:59 2019 +0100
@@ -82,1 +82,1 @@
-void SafepointMechanism::block_at_safepoint(JavaThread* thread) {
+void SafepointMechanism::callback_if_safepoint(JavaThread* thread) {
@@ -84,1 +84,1 @@
-    SafepointSynchronize::block(thread);
+    SafepointSynchronize::block(thread, false);
diff -r e65cc445234c src/hotspot/share/runtime/thread.cpp
--- a/src/hotspot/share/runtime/thread.cpp	Mon Jan 28 13:10:15 2019 +0100
+++ b/src/hotspot/share/runtime/thread.cpp	Mon Jan 28 14:10:59 2019 +0100
@@ -298,2 +297,0 @@
-  block_in_safepoint_check = true;
-
diff -r e65cc445234c src/hotspot/share/runtime/thread.hpp
--- a/src/hotspot/share/runtime/thread.hpp	Mon Jan 28 13:10:15 2019 +0100
+++ b/src/hotspot/share/runtime/thread.hpp	Mon Jan 28 14:10:59 2019 +0100
@@ -788,2 +787,0 @@
-  bool block_in_safepoint_check;              // to decide whether to block in 
SS::block or not
-


On 1/28/19 9:42 AM, Patricio Chilano wrote:
> Hi all,
> 
> Please review the following patch:
> 
> Bug URL: https://bugs.openjdk.java.net/browse/JDK-8210832
> Webrev: http://cr.openjdk.java.net/~pchilanomate/8210832/v01/webrev/
> 
> The current implementation of native monitors uses a technique that we name 
> "sneaky locking" to prevent possible deadlocks of the JVM during safepoints. The 
> implementation of this technique though introduces a race when a monitor is 
> shared between the VMThread and non-JavaThreads. This patch aims to solve that 
> problem and at the same time simplify the code.
> 
> The proposal is based on the introduction of the new class PlatformMonitor, 
> which serves as a wrapper for the actual synchronization primitives in each 
> platform (mutexes and condition variables). Most of the API calls can thus be 
> implemented as simple wrappers around PlatformMonitor, adding more assertions 
> and very little extra metadata.
> To be able to remove the lock sneaking code and at the same time avoid 
> deadlocking scenarios, we combine two techniques:
> 
> -When a JavaThread that has just acquired the lock, detects there is a safepoint 
> request in the ThreadLockBlockInVM destructor, it releases the lock before 
> blocking at the safepoint. After resuming from it, the JavaThread will have to 
> acquire the lock again.
> 
> - In the ThreadLockBlockInVM constructor for the Monitor::wait() method, in 
> order to avoid blocking we allow for a possible safepoint request to make 
> progress but without letting the JavaThread block for it (since we would be 
> stopped by the destructor anyways). We also do that for the Monitor::lock() case 
> although no deadlock is being prevented there.
> 
> The ThreadLockBlockInVM jacket is a new ThreadStateTransition class used instead 
> of the ThreadBlockInVM one. This allowed more flexibility to handle the two 
> techniques mentioned above. Also, ThreadBlockInVM calls 
> SafepointMechanism::block_if_requested() which creates some problems when trying 
> to allow safepoints to continue without stopping, since that method not only 
> checks for safepoints but also processes handshakes.
> 
> In terms of performance, benchmarks show very similar results to what we have now.
> 
> So far mach5 hs-tier1-6 on Linux, OS X, Windows and Solaris have been tested.
> 
> Thanks,
> Patricio
>