RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx
Claes Redestad
claes.redestad at oracle.com
Mon Feb 2 16:54:16 UTC 2015
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