RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx

[Bertrand Delsart]

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.
+   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
>>>
>


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38334 Saint Ismier,                                        FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~