RFR: 8210832: Remove sneaky locking in class Monitor

[David Holmes]

Hi Per,

If I may jump in on one thing you suggest ... destructors. Do we ever 
actually destroy Mutex or Monitor instances? There are inherent races 
that can make it very dangerous to try and actually delete the low-level 
PlatformMonitor and destroy the pthread_mutex or pthread_cond, and even 
release the memory used. The related PlatformEvent and PlatformParker 
are expected to be immortal and I think that is the same for 
PlatformMonitor.

Aside: I don't think distinct PlatformMutex and PlatforMonitor is worth 
the effort unless we also rework the Mutex/Monitor relationship as well.

Cheers,
David

On 29/01/2019 7:22 pm, Per Liden wrote:
> Hi Patricio,
> 
> On 01/28/2019 08:18 PM, Patricio Chilano wrote:
>> Hi Robbin,
>>
>> Thanks for reviewing this! Removing the block_in_safepoint_check 
>> thread local attribute is a great idea, here is v02:
>>
>> Full: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/webrev
> 
> I really like that we're ditching our old locking code in favor of using 
> pthread_mutex, et al. Nice work!
> 
> 
> General comment
> ----------------
> I think Mutex to be a plain mutex and not come with the baggage of 
> having a conditional variable. With this new code, it seems we're in a 
> really good position to make that happen. I.e. something like this:
> 
> class PlatformMutex {
> protected:
>    pthread_mutex_t _mutex;
> 
> public:
>    PlatformMutex();
>    ~PlatformMutex();
> 
>    void lock();
>    void unlock();
>    bool try_lock();
> };
> 
> class PlatformMonitor : public PlatformMutex {
> private:
>    pthread_cond_t _cond;
> 
> public:
>    PlatformMonitor();
>    ~PlatformMonitor();
> 
>    int wait(jlong millis);
>    void notify();
>    void notify_all();
> };
> 
> It might be that we want to do that as a separate step later instead of 
> including it in this patch. But I think we should try to get there.
> 
> 
> src/hotspot/os/posix/os_*.[ch]pp
> ---------------------------------
> * I'd suggest that we place the PlatformMonitor class in a separate file 
> (like src/hotspot/os/posix/monitor_posix.cpp), just like we have done 
> with Semaphore (in src/hotspot/os/posix/semaphore_posix.cpp).
> 
> 
> src/hotspot/os/posix/os_posix.hpp
> src/hotspot/os/solaris/os_solaris.hpp
> src/hotspot/os/windows/os_windows.hpp
> -------------------------------------
> * Please make _mutex/_cond plain variables, instead of arrays of 1. 
> That's just ugly ;)
> 
> 
> src/hotspot/os/posix/os_posix.cpp
> ---------------------------------
> * Destructor missing, to call pthread_(mutex|cond)_destroy().
> 
> 
> src/hotspot/os/solaris/os_solaris.hpp
> -------------------------------------
> * Not sure if there's a good reason to have the constructor be inlined 
> here. I'd suggest moving it to the cpp file.
> 
> * Destructor missing.
> 
> 
> src/hotspot/os/windows/os_windows.cpp
> -------------------------------------
> * Destructor missing (I'm not too familiar with the windows API but I 
> assume there's a destroy function we should call here).
> 
> 
> src/hotspot/share/runtime/interfaceSupport.inline.hpp
> -----------------------------------------------------
> * Move "private:" above monitor_adr;
> 
>   289 class ThreadLockBlockInVM : public ThreadStateTransition {
>   290   Monitor** monitor_adr;
>   291  private:
>   292   void do_preempted(Monitor** in_flight_monitor_adr) {
> 
> * monitor_adr should be _monitor_adr, or maybe even 
> _in_flight_monitor_adr to better match the name of the argument.
> 
> 
> cheers,
> Per
> 
>> Inc: http://cr.openjdk.java.net/~pchilanomate/8210832/v02/inc/webrev/
>>
>> Running mach5 again.
>>
>> Thanks,
>> Patricio
>>
>> On 1/28/19 8:31 AM, Robbin Ehn wrote:
>>> 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
>>>>
>>