RFR: 8275917: Some locks shouldn't allow_vm_block

[David Holmes]

On Tue, 26 Oct 2021 19:43:46 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> There were a few safepoint checking locks that passed allow_vm_block as true, that aren't taken by the VMThread.  I also made ClassInitError_lock behave like SystemDictionaryLock and the others in SystemDictionary::do_unloading so that doesn't require allow_vm_block either.  See the CR for more details.
> 
> Tested with tier1-6.

Hi Coleen,

So, IIUC the `allow_vm_block` flag leads to two checks:

1. If the VMThread tries to acquire any lock, then `allow_vm_block` must be true else it is a fatal error. (I have to keep reminding myself that the "block" in `allow_vm_block` is wrong, it should really just be "lock" as we do not check only when there is contention!)

2. For a non-safepoint lock, `allow_vm_block` must be true. (asserted at construction time)

So from that we can deduce that if a safepoint lock is never taken by the VMThread, then `allow_vm_block` doesn't need to be true and so should be set to false. And I can see that change, for example, with the `BeforeExit_lock`. So in looking at this PR for every safepoint lock where `allow_vm_block` has been changed from true to false, we have to confirm that the VMThread will never take that lock. From a reviewing perspective I'll rely on your analysis and testing on that. (I did look at a couple of locks out of interest :) ).

Regarding the change to the `ClassInitError_lock` ... it is valid because when a JavaThread holds that lock it doesn't execute code that can safepoint; so the VMThread would always find the lock free at a safepoint. So `allow_vm_block` being true works here. But instead you skip locking at the safepoint altogether, as done with the other locks, and so can now make `allow_vm_block` false. Really all code that takes such locks by a JavaThread should have a NSV covering the critical section to ensure it is actually safe for the VMThread to skip locking at a safepoint.

BTW it also seems to me that the `Terminator_lock`, which can be taken by a NJT (WatcherThread) but not the VMThread, can also have `allow_vm_block` set to false. (I just happened to pick that one to do some code analysis and noticed the VMThread never acquires it.)

Thanks,
David

-------------

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/6123