RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx

[David Holmes]

On 8/11/2014 2:45 AM, Bertrand Delsart wrote:
> Hi Max,
>
> Like David, I think we should go further but this is one step in the
> right direction. Thanks for doing it.
>
> Only noticed one small issue. The "or allow" part of the comment look
> strange in lock_without_safepoint_check:
>
>    void Monitor::lock_without_safepoint_check(Thread * Self) {
> +   //Ensure that the Monitor does not require or allow safepoint checks.

This is okay to me. "require" maps to _safepoint_check_always, while 
"allow" maps to _safepoint_check_sometimes.

Cheers,
David

> +   assert(this->_safepoint_check_required !=
> Monitor::_safepoint_check_always,
> +          err_msg("This lock should always have a safepoint check: %s",
> +                  this->name()));
>
> Regards,
>
> Bertrand (not a Reviewer).
>
> On 07/11/2014 17:15, Max Ockner wrote:
>> 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
>>>>
>>
>
>