RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx

[Max Ockner]

Hello all,
I have made these additonal changes:
-Moved the assert() statements into the lock and lock_without_safepoint 
methods.
-Changed Monitor::SafepointAllowed to Monitor::SafepointCheckRequired
-Changed the Monitor::SafepointCheckRequired values for several locks 
which were locked outside of a MutexLockerEx (some were locked with 
MutexLocker, some were locked were locked without any MutexLocker* )

New webrev location: http://cr.openjdk.java.net/~coleenp/8047290/

Additional testing:
jtreg ./jdk/test/java/lang/invoke
jtreg jfr tests

Here is a list of ALL of the "sometimes" locks:

"WorkGroup monitor" share/vm/utilities/workgroup.cpp
"SLTMonitor" share/vm/gc_implementation/shared/concurrentGCThread.cpp
"CompactibleFreeListSpace._lock" 
share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp
"freelist par lock" 
share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp
"SR_lock" share/vm/runtime/thread.cpp

The remaining "sometimes" locks can be found in 
share/vm/runtime/mutexLocker.cpp:

ParGCRareEvent_lock
Safepoint_lock
Threads_lock
VMOperationQueue_lock
VMOperationRequest_lock
Terminator_lock
Heap_lock
Compile_lock
PeriodicTask_lock
JfrStacktrace_lock

I have not checked the validity of the "sometimes" locks, and I believe 
that this should be a different project.

Thanks for your help!
Max Ockner
On 10/15/2014 8:54 PM, David Holmes wrote:
> Hi Max,
>
> This is looking good.
>
> A few high-level initial comments:
>
> I think SafepointAllowed should be SafepointCheckNeeded
>
> Why are the checks in MutexLocker when the state is maintained in the 
> mutex itself and the mutex/monitor has lock_without_safepoint, and 
> wait(<safepoint checking flag>) ? I would have expected to see the 
> check in the mutex/monitor methods.
>
> Checking consistent usage of the _no_safepoint_check_flag is good. But 
> another part of this is that a monitor/mutex that never checks for 
> safepoints should never be held when a thread blocks at a safepoint - 
> is there some way to easily check that? I was surprised how many locks 
> are actually not checking for safepoints.
>
> Did you find any cases where the mutex/monitor was being used 
> inconsistently and incorrectly?
>
> Did you analyse the "sometimes" cases to see if they were safe? 
> (Aside: just for fun check out what happens if you lock the 
> Threads_lock with a safepoint check and a safepoint has been requested 
> :) ).
>
> Cheers,
> David
>
> On 16/10/2014 4:04 AM, Max Ockner wrote:
>> Hi all,
>>
>> I am a new member of the Hotspot runtime team in Burlington, MA.
>> Please review my first fix related to safepoint checking.
>>
>> Summary: MutexLockerEx can either acquire a lock with or without a
>> safepoint check.
>> In some cases, a particular lock must either safepoint check always or
>> never to avoid deadlocking.
>> Some other locks have semantics which allow them to avoid deadlocks
>> despite having a safepoint check only some of the time.
>> All locks that are OK having inconsistent safepoint checks have been
>> marked. All locks that should never safepoint check and all locks that
>> should always safepoint check have also been marked.
>> When a MutexLockerEx acquires a lock with or without a safepoint check,
>> the lock's safepointAllowed marker is checked to ensure consistent
>> safepoint checking.
>>
>> Webrev: http://oklahoma.us.oracle.com/~mockner/webrev/8047290/
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8047290
>>
>> Tested with:
>>      jprt "-testset hotspot"
>>      jtreg hotspot
>>      vm.quick.testlist
>>
>> Whitebox tests:
>> test/runtime/Safepoint/AssertSafepointCheckConsistency1.java: Test
>> expects Assert ("This lock should always have a safepoint check")
>> test/runtime/Safepoint/AssertSafepointCheckConsistency2.java: Test
>> expects Assert ("This lock should never have a safepoint check")
>> test/runtime/Safepoint/AssertSafepointCheckConsistency3.java: code
>> should not assert. (Lock is properly acquired with no safepoint check)
>> test/runtime/Safepoint/AssertSafepointCheckConsistency4.java: code
>> should not assert. (Lock is properly acquired with safepoint check)
>>
>> Thanks,
>> Max
>>