Bug ID: JDK-8272797 Mutex with rank safepoint_check_never imply allow_vm

Type: Sub-task
Component: hotspot
Sub-Component: runtime
Affected Version: 18

Priority: P4
Status: Resolved
Resolution: Fixed

Submitted: 2021-08-20
Updated: 2021-09-02
Resolved: 2021-08-30

JDK 18
18 b13Fixed

If a Mutex has safepoint_check_never, that should imply that the vm thread or gc threads can also block on it, ie. _allow_vm_block.  This also makes these locks imply a NoSafepointVerifier once taken.  Having locks that don't block for safepoint but can safepoint while holding the lock should be a configuration that we should disallow for simplicity.
ie. if you can't safepoint while taking the lock, why allow safepoints while holding the lock?

The allow_vm_block matrix looks like this:

 _allow_vm_block=true,  _safepoint_check_always   - after safepoint check and lock acquired NSV
 _allow_vm_block=true,  _safepoint_check_never    - after lock acquired NSV
 _allow_vm_block=false, _safepoint_check_always   - after safepoint check and lock acquired, other safepoints ok
 _allow_vm_block=false, _safepoint_check_never    - after lock acquired, safepoints are allowed (don't allow this!)

Changeset: 98b9d980 Author: Coleen Phillimore <coleenp@openjdk.org> Date: 2021-08-30 14:47:24 +0000 URL: https://git.openjdk.java.net/jdk/commit/98b9d98032a700490cda72f645dad505164ec699
30-08-2021
Yes, this is the meaning in the code. _allow_vm_block is the flag that installs the NSV, not _safepoint_check_never. With this change all _safepoint_check_never will be _allow_vm_block=true and install the NSV. I don't want to say more to confuse things more! We talked about renaming _allow_vm_block to be _enable_nsv. Maybe that would make things clearer. Could do in another RFE. Could also change the 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()); } But that'll require more testing so definitely not part of this RFE!
27-08-2021
Turns out we weren't quite on the same page :) There are a number of different ways to use the flags and checks to get the desired outcome. I outlined one way above. Here's the way Coleen has done this - and I hope I get it right. _allow_vm_block is read to mean "a VM thread is allowed to acquire this lock" - simple as that: VMThread or GC Thread, safepoint or not. But we know that if a lock with _allow_vm_block==true is shared with JavaThreads then those JavaThreads must never hold that lock while they safepoint. Consequently, if _allow_vm_block==true then locking by a JavaThread will install the NSV logic. So we can then read _allow_vm_block as "ensures there is a NSV in force incase a VM thread wants to acquire this at a safepoint". That means that _allow_vm_block is partially decoupled from the safepoint-check state, but as previously discussed _safepoint_check_never also requires the NSV, so we may as well use safepoint_check_never to imply _allow_vm_block==true which implies to use NSV. But safepoint_check_always does not constrain the value of _allow_vm_block.
27-08-2021
> If we acquire A without a safepoint check then it is safe for a VM thread to block acquiring it - hence _allow_vm_block==true is fine. This was a logical deduction based on the expressed semantics of the meaning of _allow_vm_block, not a reflection on what our code may or may not do in this context today. If we acquire a safepoint_check_never lock then the VMThread can never try to acquire it at a safepoint as we will never reach a safepoint whilst holding that lock (assuming the NSV is installed etc). I'm not sure if there is a question about Patricio's example - as I stated at the end you can't nest locks that way: the NSV would catch it first. But I think we are on the same page here.
26-08-2021
[~dholmes]. Thank you for the deeper explanation but there's one part that I'm unclear about: > If we acquire A without a safepoint check then it is safe for a VM thread to block acquiring it - hence _allow_vm_block==true is fine. Without my change, if we acquire A without a safepoint check and _allow_vm_block is false, it is NOT safe for a VM thread to block while acquiring it and we assert if the VM thread tries to acquire it. safepoint_check_never doesn't currently imply allow_vm_block without this change. Without this change it was assumed that there is a condition where safepoint_check_never lock could only be acquired by JavaThreads, and disallowing the VM thread to take the lock would prevent deadlock. But as [~pchilanomate] pointed out, lock A=(safepoint_check_never, allow_vm_block = false) could deadlock even though the A is only taken by JavaThreads. Consider: JavaThread 1 : lock A lock B = _safepoint_check_always wait for safepoint in progress JavaThread 2: wait trying to acquire A This would hang waiting for a safepoint. So in fact, _safepoint_check_never needs to imply _allow_vm_block and _allow_vm_block is the flag used for adding NoSafepointVerifier, since some _safepoint_check_always locks also pass true for this flag. You clearly pointed out the implications for lock nesting in your last two paragraphs. Thank you.
26-08-2021
The relationship between _allow_vm_block and the safepoint checking state of a lock is not easy to express or understand. First, _allow_vm_block should be read as _allow_a_vm_thread_to_block_if_trying_to_acquire_this_lock. In other words if the lock is contended then it is okay for the VMThread (or a GC thread?) to block acquiring it, and we implicitly assume that to happen while a safepoint is active. So if _allow_vm_block is true it follows that to avoid deadlock, then the lock cannot be held by a JavaThread whilst at a safepoint. Second, we don't actually check _allow_vm_block when there is contention, but whenever we try to lock - hence the "block" part is wrong and this really should be _allow_a_vm_thread_to_acquire_this_lock (again implicitly at a safepoint). For simplicity, we constrain locks to being either safepoint_check_always or safepoint_check_never. If we acquire a lock A, and perform a safepoint check then we can block at the safepoint holding A. It follows that to avoid deadlock A must not be a lock that a VM thread might want to acquire. Or in other words A._allow_vm_block == false. Hence: safepoint_check == always => _allow_vm_block==false If we acquire A without a safepoint check then it is safe for a VM thread to block acquiring it - hence _allow_vm_block==true is fine. But we could also set _allow_vm_block==false in this case - it would simply mean this is a lock never acquired by any thread except a JavaThread (or it could potentially mean the VMThread can acquire it not at a safepoint, but we don't consider such locks here). So in essence for safepoint_check==never we don't care what the value of _allow_vm_block is, so for simplicity and convenience we can state: safepoint_check == never => _allow_vm_block==true and thus we have established a simple and direct relationship between the safepoint checking state of a mutex and _allow_vm_block. (To the point where the confusingly named _allow_vm_block could be elided.) We can also note that if we must not do a safepoint check when acquiring lock A then we must also not encounter a safepoint check anywhere in the critical section guarded by A. So it follows that when locking a _safepoint_check_never lock, we can take out a NSV. This then has implications for lock nesting: a safepoint-checking lock cannot be acquired whilst holding a non-safepoint-checking lock. But the reverse is okay.
26-08-2021