Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:24:16 GMT, Serguei Spitsyn wrote: > Thank you, Alan. Fixed now. I believe, all your suggestions have been > addressed now. Thanks, it looks much better now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856485757
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:16:34 GMT, Serguei Spitsyn wrote: >> src/hotspot/share/runtime/javaThread.hpp line 320: >> >>> 318: bool _is_in_VTMS_transition; // thread is >>> in virtual thread mount state transition >>> 319: bool _is_in_tmp_VTMS_transition; // thread is >>> in temporary virtual thread mount state transition >>> 320: bool _is_in_critical_section; // thread is >>> in a locking critical section >> >> might make sense to add a comment, that his variable Is changed/read only by >> current thread and no sync is needed. > > Good suggestion, thanks. Fixed now. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427099325
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 18:04:02 GMT, Alan Bateman wrote: > Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to > before the try in all cases, I've pointed out the cases that we missed. Thank you, Alan. Fixed now. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856366484
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:19:43 GMT, Alan Bateman wrote: > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? Okay with me. We'll need to move the notifyJvmtiDisableSuspend(true) to before the try in all cases, I've pointed out the cases that we missed. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1856339508
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 17:06:05 GMT, Alan Bateman wrote: >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > >> Implemented this renaming suggestion. Let's wait if Alan ia okay with it. > > Are you planning to drop the changes to mount/unmount too? They shouldn't be > needed. > > notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called > before the try, not in the block. We have changes coming that will require > moving these hooks to critical section enter/exit methods, so the naming will > be less important then. Yes, I've dropped changes in the mount/unmount methods. I've already done renaming to `notifyJvmtiDisableSuspend(boolean)`. Let's see if it is okay with you. It is not a problem to rename it back to `notifyJvmtiCriticalLock(boolean)`. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427032721
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 16:57:25 GMT, Serguei Spitsyn wrote: > Implemented this renaming suggestion. Let's wait if Alan ia okay with it. Are you planning to drop the changes to mount/unmount too? They shouldn't be needed. notifyJvmtiCriticalLock(boolean) is okay for now but needs to be called before the try, not in the block. We have changes coming that will require moving these hooks to critical section enter/exit methods, so the naming will be less important then. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427000950
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:11:42 GMT, Serguei Spitsyn wrote: >> src/java.base/share/classes/java/lang/VirtualThread.java line 1164: >> >>> 1162: >>> 1163: @IntrinsicCandidate >>> 1164: private native void notifyJvmtiCriticalLock(boolean enter); >> >> The name is confusing to me, the CriticalLock looks like it is the section >> is critical and might be taken by a single thread only. Or it's just unclear >> what is critical here. >> However, the purpose is to disable suspend >> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name >> here? >> or comment what critical means here. > > Okay, thanks. I like your name suggestion but let's check with Alan first. Implemented this renaming suggestion. Let's wait if Alan ia okay with it. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426990736
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Thu, 14 Dec 2023 12:06:41 GMT, Serguei Spitsyn wrote: > 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? It's a different scenario. When mounting, the coordination of the interrupt status is done before the thread identity is changed. Similarly, when unmounting, the coordination is done after reverting the thread identity to the carrier. So if there is an agent randomly suspending threads when it shouldn't be an issue here. > > 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. Minimally an assert. A counter might be needed later. > Okay. What about the Leonid's suggestion to name it > `notifyJvmtiDisableSuspend()` ? We have changes in the works that require pinning during some critical sections so I think I prefer to use that terminology. We can move the notification to JVMTI to the enter/leave methods. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1855748841
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:54:43 GMT, Leonid Mesnik wrote: >> 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 > > src/hotspot/share/prims/jvm.cpp line 4013: > >> 4011: // Notification from VirtualThread about entering/exiting sync >> critical section. >> 4012: // Needed to avoid deadlocks with JVMTI suspend mechanism. >> 4013: JVM_ENTRY(void, JVM_VirtualThreadCriticalLock(JNIEnv* env, jobject >> vthread, jboolean enter)) > > the jobject vthread is not used. Can't be the method made static to reduce > the number of arguments? > It is the performance-critical code, I don't know if it is optimized by C2. Good question. In general, I'd like to keep this unified with the other `notiftJvmti` methods. Let me double check how it fits together. Also, I'm not sure how is going to impact the intrinsification. > src/hotspot/share/runtime/javaThread.hpp line 320: > >> 318: bool _is_in_VTMS_transition; // thread is >> in virtual thread mount state transition >> 319: bool _is_in_tmp_VTMS_transition; // thread is >> in temporary virtual thread mount state transition >> 320: bool _is_in_critical_section; // thread is >> in a locking critical section > > might make sense to add a comment, that his variable Is changed/read only by > current thread and no sync is needed. Good suggestion, thanks. - PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643218 PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426643663
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 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) 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
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik wrote:
>> 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
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1164:
>
>> 1162:
>> 1163: @IntrinsicCandidate
>> 1164: private native void notifyJvmtiCriticalLock(boolean enter);
>
> The name is confusing to me, the CriticalLock looks like it is the section is
> critical and might be taken by a single thread only. Or it's just unclear
> what is critical here.
> However, the purpose is to disable suspend
> Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name
> here?
> or comment what critical means here.
Okay, thanks. I like your name suggestion but let's check with Alan first.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 30:
>
>> 28: * @requires vm.continuations
>> 29: * @library /testlibrary
>> 30: * @run main/othervm -Xint SuspendWithInterruptLock
>
> Doesn't it make sense to add a testcase without -Xint also? Just to give
> stress testing with compilation.
Thanks. I was also thinking about this. Will add a sub-test without -Xint.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 36:
>
>> 34:
>> 35: public class SuspendWithInterruptLock {
>> 36: static boolean done;
>
> done is accessed from different threads, should be volatile.
Good suggestion, thanks.
> test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
> line 54:
>
>> 52: Thread.yield();
>> 53: }
>> 54: done = true;
>
> I think it is better to use done to stop all threads and set it to true in
> the main thread after some time. So you could be sure that the yielder hadn't
> been completed before the suspender started. But it is just proposal.
Thank you. Will consider this.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426638981
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426635613
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426636196
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1426637200
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 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) rename notifyJvmti method; (2) add try-final statements to
> VirtualThread methods
Changes requested by lmesnik (Reviewer).
src/hotspot/share/prims/jvm.cpp line 4013:
> 4011: // Notification from VirtualThread about entering/exiting sync critical
> section.
> 4012: // Needed to avoid deadlocks with JVMTI suspend mechanism.
> 4013: JVM_ENTRY(void, JVM_VirtualThreadCriticalLock(JNIEnv* env, jobject
> vthread, jboolean enter))
the jobject vthread is not used. Can't be the method made static to reduce the
number of arguments?
It is the performance-critical code, I don't know if it is optimized by C2.
src/hotspot/share/runtime/javaThread.hpp line 320:
> 318: bool _is_in_VTMS_transition; // thread is in
> virtual thread mount state transition
> 319: bool _is_in_tmp_VTMS_transition; // thread is in
> temporary virtual thread mount state transition
> 320: bool _is_in_critical_section; // thread is in
> a locking critical section
might make sense to add a comment, that his variable Is changed/read only by
current thread and no sync is needed.
src/java.base/share/classes/java/lang/VirtualThread.java line 1164:
> 1162:
> 1163: @IntrinsicCandidate
> 1164: private native void notifyJvmtiCriticalLock(boolean enter);
The name is confusing to me, the CriticalLock looks like it is the section is
critical and might be taken by a single thread only. Or it's just unclear what
is critical here.
However, the purpose is to disable suspend
Wouldn't be 'notifyJvmtiSuspendLock notifyJvmtiDisableSuspend' better name
here?
or comment what critical means here.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 30:
> 28: * @requires vm.continuations
> 29: * @library /testlibrary
> 30: * @run main/othervm -Xint SuspendWithInterruptLock
Doesn't it make sense to add a testcase without -Xint also? Just to give stress
testing with compilation.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 36:
> 34:
> 35: public class SuspendWithInterruptLock {
> 36: static boolean done;
done is accessed from different threads, should be volatile.
test/hotspot/jtreg/serviceability/jvmti/vthread/SuspendWithInterruptLock/SuspendWithInterruptLock.java
line 54:
> 52: Thread.yield();
> 53: }
> 54: done = true;
I think it is better to use done to stop all threads and set it to true in the
main thread after some time. So you could be sure that the yielder hadn't been
completed before the suspender started. But it is just proposal.
-
PR Review: https://git.openjdk.org/jdk/pull/17011#pullrequestreview-1778571090
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424694672
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424697179
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424687810
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424662055
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424663078
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1424683585
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
On Fri, 8 Dec 2023 11:54:40 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) rename notifyJvmti method; (2) add try-final statements to > VirtualThread methods I chatted briefly with @sspitsyn about this. A couple of points: - 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". - toggle_is_in_critical_section needs to detect reentrancy, it is otherwise too early to refactor the Java code, e.g. call threadState while holding the interrupt lock. - All the use-sides will need to use try-finally to more reliably revert the critical section flag when rewinding. - 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. - PR Comment: https://git.openjdk.org/jdk/pull/17011#issuecomment-1847063362
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/ccba940d..18f1752e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17011=02 - incr: https://webrevs.openjdk.org/?repo=jdk=17011=01-02 Stats: 80 lines in 9 files changed: 25 ins; 7 del; 48 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
