RFR 8213150: Add verification for locking by VMThread

Tue Sep 24 14:37:23 UTC 2019

Thanks David!
Coleen

On 9/23/19 7:00 PM, David Holmes wrote:
> Thanks for clarifying. No need for updated webrev with the two changes.
>
> David
>
> On 24/09/2019 8:57 am, coleen.phillimore at oracle.com wrote:
>>
>> Hi David, Thanks for reviewing.
>>
>> On 9/23/19 6:21 PM, David Holmes wrote:
>>> Hi Coleen,
>>>
>>> Looking at version 2. Overall this seems a lot clearer regarding how 
>>> the checks work. A few minor comments
>>>
>>> src/hotspot/share/gc/shared/memAllocator.cpp
>>>
>>>     // Allocation of an oop can always invoke a safepoint,
>>>     // hence, the true argument.
>>> !   _thread->check_for_valid_safepoint_state();
>>>
>>> The comment needs fixing as no argument any more.
>>>
>> Removed the second line of the comment.
>>> ---
>>>
>>>  src/hotspot/share/oops/constantPool.cpp
>>>
>>> What is the impact of this change? When would we typically call this 
>>> code at a safepoint? Can we miss an error by not doing the 
>>> verification?
>>
>> We call this code during heap walking (jvmtiTagMap) during a 
>> safepoint. The other callers are in the compiler in the _thread_in_vm 
>> state because they're touching metadata and the 
>> jdk.internal.reflect.ConstantPool code.  One of the reasons, I had 
>> the parameter in the v01 was so that only the caller in jvmtiTagMap 
>> would skip the check.  The current other callers are safe because 
>> they are not at a safepoint.  This function could have also been 
>> refactored for JVMTI use, but it seemed like it would copy too much 
>> code.
>>
>>> ---
>>>
>>> src/hotspot/share/utilities/concurrentHashTable.inline.hpp
>>> src/hotspot/share/utilities/events.hpp
>>>
>>> So these changes are saying that the VMThread can take these locks. 
>>> Was it simply wrong before? (Ditto for the change to 
>>> ThreadsSMRDelete_lock in mutexLocker.cpp).
>>
>> Yes, it was.  We didn't check the VMThread can block 
>> (check_block_state) when calling lock_without_safepoint_check(), only 
>> when calling lock() so we missed these.
>>>
>>> ---
>>>
>>> src/hotspot/share/runtime/mutex.cpp
>>>
>>> The changes between product/non-product/debug caused me quite a bit 
>>> of confusion. IIUC previously we potentially had three versions of 
>>> set_owner_implementation:
>>> - trivial product version
>>> - more complex appearing non-product version that actually 
>>> degenerated to trivial product version
>>> - full debug/assert version
>>>
>>> now we only have two:
>>> - trivial product
>>> - full debug/assert version
>>>
>>> in which case this doesn't need to be DEBUG_ONLY:
>>>
>>>  466     DEBUG_ONLY(_last_owner = old_owner;)
>>
>> I tried to clean up these redundant conditionals, but I missed one. 
>> Good eye!
>>
>> Thank you,
>> Coleen
>>
>>
>>> ---
>>>
>>> Thanks,
>>> David
>>> -----
>>>
>>> On 24/09/2019 4:49 am, coleen.phillimore at oracle.com wrote:
>>>>
>>>>
>>>> On 9/23/19 9:37 AM, Robbin Ehn wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> src/hotspot/share/oops/constantPool.cpp
>>>>>  561     if (access_check && k != NULL) {
>>>>>
>>>>> access_check must be same as 
>>>>> !SafepointSynchronize::is_at_safepoint(), correct?
>>>>> So either change access_check to 
>>>>> !SafepointSynchronize::is_at_safepoint() or assert it.
>>>>
>>>> I can change it to check that it's at a safepoint in the constant 
>>>> pool function, which is better than a bool parameter.
>>>>>
>>>>> src/hotspot/share/runtime/mutex.cpp
>>>>>   49   if (do_safepoint_check && thread->is_active_Java_thread()) {
>>>>>   50     assert(((JavaThread *)thread)->thread_state() == 
>>>>> _thread_in_vm || rank() == Mutex::special,
>>>>>   51            "wrong thread state for using safepoint checking 
>>>>> locks");
>>>>>   52
>>>>>   53     if (rank() != Mutex::special) {
>>>>>   54       thread->check_for_valid_safepoint_state();
>>>>>   55     }
>>>>>   56   }
>>>>>
>>>>> This is not correct, you should be using _mutex->rank() <= 
>>>>> Mutex::special.
>>>>> But there is no point in checking it here, just check this in the 
>>>>> constructor that special and below are never locks. (since it is a 
>>>>> never lock we should not do_safepoint_check thus state check is 
>>>>> not needed)
>>>>
>>>> I'm going to work on the issues with 'special' locks as part of : 
>>>> https://bugs.openjdk.java.net/browse/JDK-8184732 next, but the 
>>>> rewriting (below) removed these two uses of this ranking check.
>>>>
>>>>>
>>>>> do_safepoint_check is checked four times (once via not_allowed), 
>>>>> can you please re-write this method, or better yet remove the bool 
>>>>> input.
>>>>> The bool is hardcoded so can you just create two methods, with one 
>>>>> helper method for the common checks. (yes there will be more code, 
>>>>> but readable :) )
>>>>
>>>> This is a good suggestion.  I've rewritten it to be a lot clearer, 
>>>> and am retesting it now with tier1 (all Oracle platforms) and 
>>>> tier2,3 linux-x64-debug.
>>>>
>>>> incremental webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8213150.02.incr/webrev
>>>> full webrev at 
>>>> http://cr.openjdk.java.net/~coleenp/2019/8213150.02/webrev
>>>>
>>>> Thanks,
>>>> Coleen
>>>>
>>>>>
>>>>> Thanks, Robbin
>>>>>
>>>>> On 9/20/19 11:36 PM, coleen.phillimore at oracle.com wrote:
>>>>>> Summary: extend verification for all locking not just 
>>>>>> VMOperations, and fix CLDG lock to not be taken by VM thread.
>>>>>>
>>>>>> See bug comments for more details about this change.
>>>>>>
>>>>>> Tested with tier1 all Oracle platforms, tier2-8 linux-x64-debug.
>>>>>>
>>>>>> open webrev at 
>>>>>> http://cr.openjdk.java.net/~coleenp/2019/8213150.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8213150
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>
>>