On Wed, 15 Feb 2023 20:53:23 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> src/hotspot/share/prims/jvmtiEnv.cpp line 1045: >> >>> 1043: JvmtiEnvBase::is_vthread_alive(vt_oop) && >>> 1044: !JvmtiVTSuspender::is_vthread_suspended(vt_oop)) || >>> 1045: java_thread->is_bound_vthread()) && >> >> This condition does not look correct. >> I'd expect it to be: >> ``` >> ((java_lang_VirtualThread::is_instance(vt_oop) && >> JvmtiEnvBase::is_vthread_alive(vt_oop)) || >> java_thread->is_bound_vthread()) && >> !JvmtiVTSuspender::is_vthread_suspended(vt_oop) && >> >> It is important to check for >> `!JvmtiVTSuspender::is_vthread_suspended(vt_oop)` for `bound_vthread` as >> well. >> The same issue is in the `JvmtiEnv::ResumeAllVirtualThreads`. > > The thing is that JvmtiVTSuspender::is_vthread_suspended() checks > _suspended_list/not_suspended_list which are not set with single > suspend/resume > functions(SuspendThread/SuspendThreadList/ResumeThread/ResumeThreadList) on a > BoundVirtualThread. For single suspend/resume on a BoundVirtualThread we go > through the same path as for platform threads. In any case, the > suspend_thread() call will just return JVMTI_ERROR_THREAD_SUSPENDED if a > thread is already suspended, but if you want to avoid the call altogether I > can add the equivalent check which would be !java_thread->is_suspended(). > > Now, I realized that although single-suspends will not change > _suspended_list/not_suspended_list for a BoundVirtualThread, > SuspendAllVirtualThreads/ResumeAllVirtualThreads can if a BoundVirtualThread > is part of the exception list. In the SuspendAllVirtualThreads case for > example, when restoring the resume state, the call to > JvmtiVTSuspender::is_vthread_suspended() would return true (we are already in > SR_all mode and the BoundVirtualThread will not be in _not_suspended_list), > meaning the BoundVirtualThread will be added to the not_suspended_list. > So we have to decide if we want to keep track of BoundVirtualThreads on these > lists or not. Given that I see JvmtiVTSuspender::is_vthread_suspended() is > always called on a VirtualThread the easiest thing would be to not track > them, and just add a check in > SuspendAllVirtualThreads/ResumeAllVirtualThreads where the elist is populated > so only VirtualThreads are added. What do you think? The `SuspendAllVirtualThreads` spec has this statement: Suspend all virtual threads except those in the exception list. Virtual threads that are currently suspended do not change state. And the `ResumeAllVirtualThreads` spec has this statement: Resume all virtual threads except those in the exception list. Virtual threads that are currently resumed do not change state So that, `SuspendAllVirtualThreads` should do nothing with the already suspended virtual threads. And `ResumeAllVirtualThreads` should do nothing with the already resumed virtual threads. It does not depend on the except_list. It is better to avoid suspending already suspended threads and resuming already resumed. So, at least a check for `!java_thread->is_suspended()` is needed. Did you observe any JVMTI or JDI suspend/resume tests failing? > Now, I realized that although single-suspends will not change > _suspended_list/not_suspended_list for a BoundVirtualThread, > SuspendAllVirtualThreads/ResumeAllVirtualThreads can if a BoundVirtualThread > is part of the exception list. In the SuspendAllVirtualThreads case for > example, when restoring the resume state, the call to > JvmtiVTSuspender::is_vthread_suspended() would return true (we are already in > SR_all mode and the BoundVirtualThread will not be in _not_suspended_list), > meaning the BoundVirtualThread will be added to the not_suspended_list. > So we have to decide if we want to keep track of BoundVirtualThreads on these > lists or not. Given that I see JvmtiVTSuspender::is_vthread_suspended() is > always called on a VirtualThread the easiest thing would be to not track > them, and just add a check in > SuspendAllVirtualThreads/ResumeAllVirtualThreads where the elist is populated > so only VirtualThreads are added. What do you think? I feel that following the VirtualThread's implementation code path would be more consistent, unified and free of surprises. But I also see that following the platform threads code path looks pretty simple. So, it can be okay. So, for this approach, I think, the code fragments which collect and restore resumed/suspended state for virtual threads need to be bypassed by the BoundVirtualThreads implementation. Please, let me know if it does not work by any reason. We also will have to rely on the testing here. ------------- PR: https://git.openjdk.org/jdk/pull/12512
