On Wed, 7 Apr 2021 15:48:55 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> The test worked because we didn't check for suspend when leaving a safepoint >> request back to native. >> @pchilano have more info on why the test even works today. > > Right, the reason the test was working so far but not anymore without this > changes is because before we didn't check for suspension on > ~ThreadBlockInVM() after leaving a safepoint safe state. The interaction is > kind of subtle: > In the test the main thread creates 10 working threads and sets one of those > to have the role of 'Suspender'. The 'Suspender' acquires agent_monitor(a > jvmtirawmonitor), suspends all working threads including itself by calling > jvmti->SuspendThreadList() and then it releases agent_monitor. In the mean > time the main thread just blocks on agent_monitor until 'Suspender' releases > it to check that everybody was indeed suspended. Now, at first glance this > looks like(at least for me) the test should deadlock because 'Suspender' > should never return from SuspendThreadList() to release agent_monitor since > he was suspended too. The reason 'Suspender' returns from SuspendThreadList() > is because he never actually checks for suspension after the safepoint > operation (VM_ThreadsSuspendJVMTI) finished. 'Suspender' waits on the > VMOperation_lock with as_suspend_equivalent=false (default) so it transitions > back to _thread_in_vm without suspending. And then when going back to native > in ~ThreadInVMfromNa tive() we also don't check for suspends (we now check it here too actually). > The JVMTI specs says this about SuspendThreadList: > > "If the calling thread is specified in the request_list array, this function > will not return until some other thread resumes it." > > So this patch actually fixes that. We also had to fix deadlock in this test in Loom. The fix is almost the same, and I agree with it. ------------- PR: https://git.openjdk.java.net/jdk/pull/3191
