RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx
Max Ockner
max.ockner at oracle.com
Fri Dec 12 16:02:28 UTC 2014
OK, I have made these changes.
Thanks to everyone who helped me through this, especially Dan, David,
and Coleen.
On 12/10/2014 4:11 PM, Daniel D. Daugherty wrote:
> On 12/10/14 9:56 AM, Max Ockner wrote:
>> New webrev: http://cr.openjdk.java.net/~coleenp/8047290absolutely_final/
>
> Thumbs up! No need for a re-review if you address any of the
> formatting comments below.
>
> Repeat from last time (I've seen trailing white space in this
> round; haven't checked for the other jcheck-ish things).
>
> - jcheck is going to complain about some of your lines that
> have trailing white space:
>
> $ grep -n ' *$' `cat files.list`
> $ grep -n '\t' `cat files.list`
>
> Don't know if you have any tabs, but I included that one also.
> Not all platforms like '\t' for the grep parameter.
>
> - jcheck may also complain about the blank lines at the end
> of the new tests
>
> src/share/vm/runtime/mutex.hpp
> line 197: SafepointCheckRequired
> safepoint_check_required = _safepoint_check_always);
> One space too many after the '='.
>
> Also, I see an extra space at the end of the line; jcheck
> will not be happy.
>
> src/share/vm/runtime/mutex.cpp
> No comments.
>
> src/share/vm/runtime/mutexLocker.cpp
> line 174: }
> Should be moved left by two spaces (i.e., one indent to the
> left of the code block above it).
>
> src/share/vm/runtime/thread.cpp
> No comments.
>
> src/share/vm/runtime/vmThread.cpp
> No comments.
>
> src/os/aix/vm/osThread_aix.cpp
> No comments.
>
> src/os/bsd/vm/osThread_bsd.cpp
> No comments.
>
> src/os/linux/vm/osThread_linux.cpp
> No comments.
>
> src/share/vm/classfile/classLoaderData.cpp
> No comments.
>
> src/share/vm/memory/metaspace.cpp
> No comments.
>
> src/share/vm/runtime/vm_operations.cpp
> No comments.
>
> src/share/vm/memory/sharedHeap.cpp
> No comments.
>
> src/share/vm/gc_implementation/concurrentMarkSweep/compactibleFreeListSpace.cpp
>
> No comments.
>
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp
>
> line 6391: _lock(mutex_rank >= 0 ? new Mutex(mutex_rank,
> mutex_name, true ,
> Extra space before the last comma.
>
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp
>
> No comments.
>
> src/share/vm/gc_implementation/g1/concurrentG1RefineThread.cpp
> No comments.
>
> src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
> No comments.
>
> src/share/vm/gc_implementation/g1/heapRegionRemSet.cpp
> No comments.
>
> src/share/vm/gc_implementation/parallelScavenge/gcTaskManager.cpp
> No comments.
>
> src/share/vm/gc_implementation/shared/concurrentGCThread.cpp
> No comments.
>
> src/share/vm/services/diagnosticFramework.cpp
> No comments.
>
> src/share/vm/services/memoryManager.cpp
> No comments.
>
> src/share/vm/utilities/decoder.cpp
> No comments.
>
> src/share/vm/utilities/events.hpp
> No comments.
>
> src/share/vm/utilities/workgroup.cpp
> No comments.
>
> src/share/vm/prims/whitebox.cpp
> line 1045: WB_END
> Please add a blank line after this line.
>
> test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
> No comments.
>
> test/runtime/Safepoint/AssertSafepointCheckConsistency1.java
> line 42: WhiteBox.getWhiteBox().assertMatchingSafepointCalls(true,
> true);
> Java code indent is four spaces.
>
> test/runtime/Safepoint/AssertSafepointCheckConsistency2.java
> line 42:
> WhiteBox.getWhiteBox().assertMatchingSafepointCalls(false, false);
> Java code indent is four spaces.
>
> test/runtime/Safepoint/AssertSafepointCheckConsistency3.java
> line 42:
> WhiteBox.getWhiteBox().assertMatchingSafepointCalls(false, true);
> Java code indent is four spaces.
>
> test/runtime/Safepoint/AssertSafepointCheckConsistency4.java
> line 42: WhiteBox.getWhiteBox().assertMatchingSafepointCalls(true,
> false);
> Java code indent is four spaces.
>
> Dan
>
>
>
>>
>> I have addressed dan's suggestions. I also removed unnecessary
>> "this->" occurrences from the assert statements.
>>
>> Though I realize that I have unnecessarily duplicated code in my
>> tests, I do not want to combine the tests into one right now because
>> (1) They work as they are, (2) They can fail independently, (3) The
>> amount of code needed to run all four tests in one file without
>> crashing out after the first failure is not as small as you might
>> think, and (4) I want to commit this before someone else messes with
>> the locks to avoid more merge conflicts. To be clear, I am not
>> opposed to fixing this separately if people think it is important,
>> but I prefer to put it off until the bulk of the fix is committed.
>>
>> Does this look ready?
>>
>> Thanks for your help,
>> Max O
>>
>>
>>
>> On 12/8/2014 1:39 AM, David Holmes wrote:
>>> Hi Max,
>>>
>>> Just a nit with the tests - it seems to me we should be able to
>>> write a single Java class that can process all four combinations
>>> rather than duplicating the bulk of the code.
>>>
>>> Thanks,
>>> David
>>>
>>> On 5/12/2014 7:50 AM, Max Ockner wrote:
>>>> 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
>>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>
>>
>
More information about the hotspot-dev
mailing list