RFR 8213150: Add verification for locking by VMThread

[coleen.phillimore at oracle.com]

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