RFR: 8311218: fatal error: stuck in JvmtiVTMSTransitionDisabler::VTMS_transition_disable [v3]

[Serguei Spitsyn]

On Tue, 12 Dec 2023 23:42:07 GMT, Leonid Mesnik <lmesnik at openjdk.org> 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