RFR: 8272797: Mutex with rank safepoint_check_never imply allow_vm_block [v7]

[David Holmes]

On 31/08/2021 12:50 am, Coleen Phillimore wrote:
> On Mon, 30 Aug 2021 02:34:17 GMT, David Holmes <dholmes at openjdk.org> wrote:
> 
>>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>>
>>>    Reverse the order of safepoint_check_required and allow_vm_block arguments so that the first can imply the value of the second.  This is good because if we decide to rename the second to something like _allow_safepoint_use instead there's less to change. Also remove default arguments as they are evil.
>>
>> src/hotspot/share/memory/heapInspection.hpp line 248:
>>
>>> 246:       _missed_count(0),
>>> 247:       _success(true),
>>> 248:       _mutex(Mutex::leaf, "Parallel heap iteration data merge lock", Mutex::_safepoint_check_always) {}
>>
>> Is this lock even used by JavaThreads. ??
> 
> It doesn't look like this is.  If we change the _allow_vm_block check in Mutex::check_block_state to be:
> 
>       if (!_enforces_nsv && (!thread->isJavaThread() && is_at_safepoint())) {
>           // some good explanatory comment
>           fatal("A NonJava thread could block on lock that may be held by a JavaThread during safepoint: %s", name());
>      }
> 
> I think this lock will get caught by this assert.

The point of my comment was more that it seems very odd to mark a lock 
as safepoint-check-always when it is never used by any JavaThreads. 
Given it isn't used by JavaThreads then the actual value of the 
safepoint-check field is immaterial in practice, but saying "always" 
seems "wrong".

Cheers,
David
-----

>> test/hotspot/gtest/metaspace/test_is_metaspace_obj.cpp line 52:
>>
>>> 50:
>>> 51:   void do_test(Metaspace::MetadataType mdType) {
>>> 52:     _lock = new Mutex(Monitor::leaf, "gtest-IsMetaspaceObjTest-lock", Monitor::_safepoint_check_never);
>>
>> Again unclear what impact false to true might have here - assuming VM threads even lock this? Ditto for other metaspace tests.
> 
> It has zero impact.  These locks just want no safepoint checking.
> 
> -------------
> 
> PR: https://git.openjdk.java.net/jdk/pull/5218
>