RFR 8213150: Add verification for locking by VMThread

[David Holmes]

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
>>>
>