RFR:8047290:Ensure consistent safepoint checking in MutexLockerEx
Daniel D. Daugherty
daniel.daugherty at oracle.com
Fri Dec 5 21:27:34 UTC 2014
General
- Most of these comments are style related.
- The accidental deletion of 'UnsafeJlong_lock' needs to
be fixed.
- This is a big piece of work for the first bug that you
started on! You've done really good work here.
- The correct choice between these values:
Monitor::_safepoint_check_never
Monitor::_safepoint_check_sometimes
Monitor::_safepoint_check_always
is very hard to sanity check. As David H. likes to say
for changes like this: The proof is in the testing!
In this case, because of the myriad of options and code
paths, we may not catch all the "oops" values right away.
This early in the JDK9 release is the perfect time to
push a change like this!
- 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
On 12/4/14 2:50 PM, Max Ockner wrote:
> Hello once again...
> I have a new (and suggestively titled) webrev:
> http://cr.openjdk.java.net/~coleenp/8047290final/
src/share/vm/runtime/mutex.hpp
lines 157-162: the entire comment block needs one more
space of indent.
line 163 enum SafepointCheckRequired {
line 171: };
The right brace should line up under the 'e' in 'enum'.
line 173: NOT_PRODUCT(SafepointCheckRequired
_safepoint_check_required;)
I think this one needs one more space of indent.
line 196: Monitor(int rank, const char *name, bool
allow_vm_block=false,
line 197: SafepointCheckRequired
safepoint_check_required= _safepoint_check_always);
A single space around the '=' would be good.
line 283: Mutex (int rank, const char *name, bool
allow_vm_block=false,
Deleting the space before the '(' would be good; I
A single space around the '=' would be good.
src/share/vm/runtime/mutex.cpp
line 1094: //Make sure safepoint checking is used properly.
Please add a space before 'Make'.
src/share/vm/runtime/mutexLocker.cpp
line 170: #define def(var, type, pri, vm_block,
safepoint_check_allowed ) { \
line 171: var = new type(Mutex::pri, #var, vm_block,
safepoint_check_allowed); \
line 172: assert(_num_mutex < MAX_NUM_MUTEX, "increase
MAX_NUM_MUTEX"); \
line 173: _mutex_array[_num_mutex] = var;
Please reformat like so:
line 170: #define def(var, type, pri, vm_block,
safepoint_check_allowed) { \
line 171: var = new type(Mutex::pri, #var, vm_block,
safepoint_check_allowed); \
line 172: assert(_num_mutex < MAX_NUM_MUTEX, "increase
MAX_NUM_MUTEX"); \
line 173: _mutex_array[_num_mutex] =
var; \
line 176: void mutex_init() {
For all the def() macro calls, you need to decide whether to
have spaces before the ');' or not to make them line up on the
longest variant. I see these variations:
..., Monitor::_safepoint_check_never );
..., Monitor::_safepoint_check_sometimes);
..., Monitor::_safepoint_check_always);
I would prefer no space before the ');' and do the line ups on
the comments, but it is your call. Example:
..., Monitor::_safepoint_check_never); // comment1
..., Monitor::_safepoint_check_sometimes); // comment2
..., Monitor::_safepoint_check_always); // comment3
line 240: // CMS_modUnionTable_lock leaf
line 241: // CMS_bitMap_lock leaf 1
line 242: // CMS_freeList_lock leaf 2
Lines 241,242 should line up with the line 240.
line 293: #ifndef SUPPORTS_NATIVE_CX8
line 294: def(UnsafeJlong_lock , Mutex, special,
false);
line 295: #endif
Looks like a merge error. This lock was recently added.
src/share/vm/runtime/thread.cpp
No comments.
src/share/vm/runtime/vmThread.cpp
line 217: please break the really line at the new parameter
and align with the 'M' in '(Mutex:...'
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
No comments.
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 1044 attemptedNoSafepointValue == JNI_TRUE );
Please delete the space before ');'.
test/testlibrary/whitebox/sun/hotspot/WhiteBox.java
line 230: //Safepoint Checking
Please add a space before the 'S' in 'Safepoint'.
Comment test comments:
New file should have a single copyright year.
Should there be an @bug entry?
Should there be an @build entry for the test library?
test/runtime/Safepoint/AssertSafepointCheckConsistency1.java
See common test comments.
Should this test check for "assert"?
test/runtime/Safepoint/AssertSafepointCheckConsistency2.java
See common test comments.
line 53: OutputAnalyzer output = new
OutputAnalyzer(pb.start());
One space too many on this indent.
Should this test check for "assert"?
test/runtime/Safepoint/AssertSafepointCheckConsistency3.java
See common test comments.
Should this test check for no "assert"?
test/runtime/Safepoint/AssertSafepointCheckConsistency4.java
See common test comments.
Should this test check for no "assert"?
> Ran aurora tests.
OK, but which Aurora tests? With Aurora you can run a single
test in one subsuite or you can run everything we have...
My recommendation would be to run the configs that are
specific to each team's nightly run. For example, the
"Runtime-SVC Nightly tests" is ours...
Dan
>
> 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