RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]

[Alan Bateman]

On Mon, 18 Dec 2023 17:09:59 GMT, Serguei Spitsyn <sspitsyn at openjdk.org> wrote:

>> This fix is for JDK 23 but the intention is to back port it to 22 in RDP-1 time frame.
>> It is fixing a deadlock issue between `VirtualThread` class critical sections with the `interruptLock` (in methods: `unpark()`, `interrupt()`, `getAndClearInterrupt()`, `threadState()`, `toString()`), `JvmtiVTMSTransitionDisabler` and JVMTI `Suspend/Resume` mechanisms.
>> The deadlocking scenario is well described by Patricio in a bug report comment.
>> In simple words, a virtual thread should not be suspended during 'interruptLock' critical sections.
>> 
>> The fix is to record that a virtual thread is in a critical section (`JavaThread`'s `_in_critical_section` bit) by notifying the VM/JVMTI about begin/end of critical section.
>> This bit is used in `HandshakeState::get_op_for_self()` to filter out any `HandshakeOperation` if a target `JavaThread` is in a critical section.
>> 
>> Some of new notifications with `notifyJvmtiSync()` method is on a performance critical path. It is why this method has been intrincified.
>> 
>> New test was developed by Patricio:
>>  `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>> The test is very nice as it reliably in 100% reproduces the deadlock without the fix.
>> The test is never failing with this fix.
>> 
>> Testing:
>>  - tested with newly added test: `test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock`
>>  - tested with mach5 tiers 1-6
>
> Serguei Spitsyn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge
>  - review: improve an assert message
>  - review: moved a couple of comments out of try blocks
>  - review: moved notifyJvmtiDisableSuspend(true) out of try-block
>  - review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
>  - review: (1) rename notifyJvmti method; (2) add try-final statements to VirtualThread methods
>  - Resolved merge conflict in VirtualThread.java
>  - added @summary to new test SuspendWithInterruptLock.java
>  - add new test SuspendWithInterruptLock.java
>  - 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable

src/hotspot/share/runtime/javaThread.hpp line 652:

> 650: 
> 651:   bool is_disable_suspend() const                { return _is_disable_suspend; }
> 652:   void toggle_is_disable_suspend()               { _is_disable_suspend = !_is_disable_suspend; };

Looking at this again then I don't think it can be a bit that is toggled on and off will work. Consider the case where several threads attempt to poll the state of a virtual Thread with Thread::getState at the same time. This can't work without an atomic counter and further coordination. So I think further work is required on this issue.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432770204