Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 3 Jan 2024 13:55:24 GMT, Thomas Wuerthinger wrote: > Are these new compiler intrinsics required or an optional performance > optimization? Performance. If the intrinsic isn't there then some methods executed on virtual threads, or on a virtual thread as the target for some op, will have to call into the VM. The main concern was Thread.interrupted() as it gets called very frequently in locking and concurrency code. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1440487675
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 10:40:23 GMT, Serguei Spitsyn wrote: >>> You can't do this! The Java code knows nothing about JVM TI being >>> enabled/disabled and will call this function unconditionally. >> >> Indeed. I wonder if anyone is testing minimal builds to catch issues like >> this. > > Good catch, David! > Filed a cleanup bug: https://bugs.openjdk.org/browse/JDK-8322538 Are these new compiler intrinsics required or an optional performance optimization? This PR creates issues for us when updating the JDK build for Graal. CC @davleopo - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1440478960
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 14:15:48 GMT, Alan Bateman wrote: > Update: ignore this I mis-read that it updates the current thread's suspend > value, not the thread's suspend value. Thanks, Alan. I've also got confused with this and even filed a follow up bug. :) Yes, the initial design was the `_is_disable_suspend` is set/modified/accessed on the current thread only. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1433182764
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Mon, 18 Dec 2023 17:09:59 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 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
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 08:02:14 GMT, Alan Bateman wrote:
>> src/hotspot/share/prims/jvm.cpp line 4024:
>>
>>> 4022: #else
>>> 4023: fatal("Should only be called with JVMTI enabled");
>>> 4024: #endif
>>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
>> You can't do this! The Java code knows nothing about JVM TI being
>> enabled/disabled and will call this function unconditionally.
>
> Indeed. I wonder if anyone is testing minimal builds to catch issues like
> this.
Good catch, David!
Filed a cleanup bug: https://bugs.openjdk.org/browse/JDK-8322538
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432548911
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Wed, 20 Dec 2023 04:44:35 GMT, David Holmes wrote: > You can't do this! The Java code knows nothing about JVM TI being > enabled/disabled and will call this function unconditionally. Indeed. I wonder if anyone is testing minimal builds to catch issues like this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432377494
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
On Mon, 18 Dec 2023 17:09:59 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 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/prims/jvm.cpp line 4024:
> 4022: #else
> 4023: fatal("Should only be called with JVMTI enabled");
> 4024: #endif
You can't do this! The Java code knows nothing about JVM TI being
enabled/disabled and will call this function unconditionally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1432241016
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v8]
> 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 - Changes: https://git.openjdk.org/jdk/pull/17011/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=07 Stats: 229 lines in 15 files changed: 196 ins; 0 del; 33 mod Patch: https://git.openjdk.org/jdk/pull/17011.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17011/head:pull/17011 PR: https://git.openjdk.org/jdk/pull/17011
