RFR 8213150: Add verification for locking by VMThread
David Holmes
david.holmes at oracle.com
Mon Sep 23 23:00:47 UTC 2019
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
>>>
>
More information about the hotspot-runtime-dev
mailing list