RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx
Claes Redestad
claes.redestad at oracle.com
Tue Feb 3 12:21:32 UTC 2015
Filed https://bugs.openjdk.java.net/browse/JDK-8072406, thanks!
/Claes
On 02/02/2015 06:07 PM, Coleen Phillimore wrote:
>
> That appears unintentional. Yes, that is a bug. Do you want to
> file one, or we could?
> Thanks!
> Coleen
>
> On 2/2/15, 11:54 AM, Claes Redestad wrote:
>> Hi,
>>
>> I know this has been pushed, but I wonder if the removal of
>> _num_mutex++ from
>> the def macro in mutexLocker.cpp was really intentional?
>>
>> It seems to me this means _mutex_array won't initialize properly in
>> the current
>> code, breaking print_owned_locks_on_error (always prints None). Bug?
>>
>> /Claes
>>
>> On 2014-12-12 17:02, Max Ockner wrote:
>>> 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