On Thu, 16 Feb 2023 07:48:16 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
> 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. > Yes, the elist comment was related to the other issue. The reason why I didn't explicitly added this check is because it will we done in the suspend/resume call already (while holding the handshake lock which serializes the suspend/resume requests). So inside the suspend handshake for example either the target is suspended, so nothing is done, or the target is not suspended and we suspend it. Is there an actual issue I'm missing or is this to make a fast check and avoid the call? In any case I added the check. Also I see the specs for the single suspend/resume functions say the same thing about not changing the state and, for example, in SuspendThreadList we are also not making an explicit check before the call to suspend_thread(). So if there is an actual issue here, don't we need to add that check for them too? >Did you observe any JVMTI or JDI suspend/resume tests failing? > No. I'll retest the patch again though once I resolve all comments. > > 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, so the > `_suspended_list`/`not_suspended_list` won't be impacted. Please, let me know > if it does not work by any reason. We also will have to rely on the testing > here. > Sounds good. Yes, just following the platform thread code path is easier. Plus the other functions that are supported on virtual threads already have a branch for platform thread vs virtual thread, and the BoundVirtualThread already takes the platform thread path. Last commit is a fix for this issue in SuspendAllVirtualThreads/ResumeAllVirtualThreads, please take a look. ------------- PR: https://git.openjdk.org/jdk/pull/12512
