RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx

[David Holmes]

Hi Max,

On 8/11/2014 2:15 AM, 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/

Generally this is all okay - a few style and other nits below.

However you missed adding an assert in Monitor::wait to check if the 
no_safepoint_check flag was being used correctly for the current monitor.

Specific comments:

src/share/vm/runtime/mutex.hpp

This comment is no longer accurate with the moved check location:

+ // MutexLockerEx checks these flags when acquiring a lock
+ // to ensure consistent checking for each lock.

The same goes for other references to MutexLockerEx in the enum description.

Also copyright year needs updating.

---

src/share/vm/runtime/mutex.cpp

  898   //Ensure
  961   //Ensure

Space needed after //

---

src/share/vm/runtime/mutexLocker.cpp

+  var = new type(Mutex::pri, #var, vm_block,safepoint_check_allowed); \

space needed after comma in k,s

---

src/share/vm/runtime/mutexLocker.hpp

Whitespace only changes - looks like leftovers from removed edits.



Thanks,
David


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