RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx

Thu Dec 4 21:50:55 UTC 2014

Hello once again...
I have a new (and suggestively titled) webrev: 
http://cr.openjdk.java.net/~coleenp/8047290final/

Ran aurora tests.

Here is a list of "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
"CMS_markBitMap_lock" 
share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.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

Thanks,
Max Ockner

On 11/25/2014 3:03 AM, David Holmes wrote:
> Hi Max,
>
> On 21/11/2014 7:56 AM, Max Ockner wrote:
>> Hello again,
>>
>> I have made changes based on all comments. There is now a pair of assert
>> statements in the Monitor/Mutex wait() methods. When I reran tests, I
>
> Is there an updated webrev?
>
>> caught another lock that I had to change to "sometimes", but the assert
>> that caught this lock was not in wait. There are currently no locks that
>> use try to pass an incorrect safepoint check argument to wait().
>> Instead, gcbasher did not catch this lock last time, when the only
>> asserts were in lock and lock_without_safepoint. This lock is
>> "CMS_markBitMap_lock" in
>> share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp. 
>>
>> I'm guessing that it was not caught by the tests because this section of
>> code is not reached very often. My initial inspection failed to catch
>> this lock because it is passed around between various structures and
>> renamed 4 times. I have not yet found a good way to check for this
>> situation, even with a debugger.
>>
>> Are there any tests which actually manage to hit every line of code?
>
> No. There is too much code that is dependent on low-level details of 
> the GC used, the compilation strategy, plus the set of runtime flags 
> used (and whether product or fastdebug). That's why we have lots of 
> tests run in lots of different ways, to try to get coverage.
>
>> How should I handle this situation where I can't rely on the tests that
>> I have run to tell me if I have missed something?
>> At what point can I assume that everything is OK?
>
> Difficult to answer in general - there are a number of recommended 
> test suites used by the runtime team, but your changes will also 
> impact GC and compiler code and so may not get exercised by the 
> runtime test suites (unless run with various compiler and GC options). 
> Perhaps an ad-hoc test run similar to nightlies? Or you push after the 
> weekly snapshot so as to maximise nightly testing, and if there are 
> issues exposed then you have time to address them or revert the fix.
>
> Cheers,
> David
>
>> Thanks,
>> Max Ockner
>>
>> On 11/10/2014 11:57 PM, David Holmes wrote:
>>> 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
>>>>>>
>>>>
>>