On Thu, 14 Dec 2023 17:30:54 GMT, Serguei Spitsyn 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 incrementally with one
> additional commit since the last revision:
>
> review: 1) replace CriticalLock with DisableSuspend; 2) minor tweaks
src/java.base/share/classes/java/lang/VirtualThread.java line 746:
> 744: } else if ((s == PINNED) || (s == TIMED_PINNED)) {
> 745: try {
> 746: notifyJvmtiDisableSuspend(true);
Move to before the try.
src/java.base/share/classes/java/lang/VirtualThread.java line 853:
> 851: checkAccess();
> 852: try {
> 853: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 886:
> 884: if (oldValue) {
> 885: try {
> 886: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 917:
> 915: case RUNNING:
> 916: try {
> 917: notifyJvmtiDisableSuspend(true);
This one also needs to be before try.
src/java.base/share/classes/java/lang/VirtualThread.java line 1042:
> 1040: if (carrier != null) {
> 1041: try {
> 1042: notifyJvmtiDisableSuspend(true);
this one too.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080057
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080394
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080484
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080704
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427080811
