On Mon, 18 Dec 2023 17:09:59 GMT, Serguei Spitsyn <sspit...@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
