Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 22:35:18 GMT, Serguei Spitsyn wrote:
>> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>>
>>> 1041: notifyJvmtiDisableSuspend(true);
>>> 1042: try {
>>> 1043: // include the carrier thread state and name when
>>> mounted
>>
>> This one too, can you move the comment to before the
>> notifyJvmtiDisableSuspend.
>
> Moved both comments out of try blocks.
> What about this one (it seems we would wont to do the same) ? :
>
> notifyJvmtiDisableSuspend(true);
> try {
> // unpark carrier thread when pinned
> synchronized (carrierThreadAccessLock()) {
> Thread carrier = carrierThread;
> if (carrier != null && ((s = state()) == PINNED || s
> == TIMED_PINNED)) {
> U.unpark(carrier);
> }
> }
> } finally {
> notifyJvmtiDisableSuspend(false);
> }
Moved 3 comments out of try blocks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427386103
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 19:50:00 GMT, Alan Bateman wrote:
>> Serguei Spitsyn has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> review: moved notifyJvmtiDisableSuspend(true) out of try-block
>
> src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
>
>> 1041: notifyJvmtiDisableSuspend(true);
>> 1042: try {
>> 1043: // include the carrier thread state and name when
>> mounted
>
> This one too, can you move the comment to before the
> notifyJvmtiDisableSuspend.
Moved both comments out of try blocks.
What about this one (it seems we would wont to do the same) ? :
notifyJvmtiDisableSuspend(true);
try {
// unpark carrier thread when pinned
synchronized (carrierThreadAccessLock()) {
Thread carrier = carrierThread;
if (carrier != null && ((s = state()) == PINNED || s ==
TIMED_PINNED)) {
U.unpark(carrier);
}
}
} finally {
notifyJvmtiDisableSuspend(false);
}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427373522
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
On Thu, 14 Dec 2023 18:26:55 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: moved notifyJvmtiDisableSuspend(true) out of try-block
src/java.base/share/classes/java/lang/VirtualThread.java line 918:
> 916: notifyJvmtiDisableSuspend(true);
> 917: try {
> 918: // if mounted then return state of carrier thread
Can you move this comment line to before the notifyJvmtiDisableSuspend(true)?
src/java.base/share/classes/java/lang/VirtualThread.java line 1043:
> 1041: notifyJvmtiDisableSuspend(true);
> 1042: try {
> 1043: // include the carrier thread state and name when
> mounted
This one too, can you move the comment to before the notifyJvmtiDisableSuspend.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198296
PR Review Comment: https://git.openjdk.org/jdk/pull/17011#discussion_r1427198673
Re: RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v5]
> 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: moved notifyJvmtiDisableSuspend(true) out of try-block - Changes: - all: https://git.openjdk.org/jdk/pull/17011/files - new: https://git.openjdk.org/jdk/pull/17011/files/4e5f6447..ad990422 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17011&range=03-04 Stats: 10 lines in 1 file changed: 5 ins; 5 del; 0 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
