On Fri, 8 Dec 2023 11:54:40 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 incrementally with one 
> additional commit since the last revision:
> 
>   review: (1) rename notifyJvmti method; (2) add try-final statements to 
> VirtualThread methods

@AlanBateman Thank you for reviewing an the comment.
>  It shouldn't be necessary to touch mount/unmount as the thread identity is 
> the carrier, not the virtual thread, when executing the "critical code".

Carrier thread also can be suspended when executing the "critical code".
Why do you think it can't be a problem? Do you think the deadlocking scenario 
described in the bug report is not possible?

>  toggle_is_in_critical_section needs to detect reentrancy, it is otherwise 
> too easy to  refactor the Java code, e.g. call threadState while holding the 
> interrupt lock.

Is your concern a recursive `interruptLock` enter? I was also thinking if this 
scenario is possible, so a counter can be used instead of boolean.

>  All the use-sides will need to use try-finally to more reliably revert the 
> critical section flag when rewinding.

Right, thanks. It is already done.

>  The naming is very problematic, we'll need to replace with methods that are 
> clearly named enter and exit critical section. Ongoing work in this area to 
> support monitors has to introduce some temporary pinning so there will be 
> enter/exitCriticalSection methods, that's a better place for the JVMTI hooks.

Okay. What about the Leonid's suggestion to name it 
`notifyJvmtiDisableSuspend()` ?

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

PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855730274

Reply via email to