On Wed, 21 Dec 2022 21:33:28 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Now the `JvmtiVTMSTransitionDisabler` mechanism supports disabling VTMS >> transitions for all virtual threads only. It should also support disabling >> transitions for any specific virtual thread as well. This will improve >> scalability of the JVMTI functions operating on target virtual threads as >> the functions can be executed concurrently without blocking each other >> execution when target virtual threads are different. >> New constructor `JvmtiVTMSTransitionDisabler(jthread vthread)` is added >> which has jthread parameter of the target virtual thread. >> >> Testing: >> mach5 jobs are TBD (preliminary testing was completed): >> - all JVMTI, JDWP, JDI and JDB tests have to be run >> - Kitchensink >> - tier5 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > use owned_by_self vs is_locked in JvmtiVTMSTransition_lock asserts Hi Serguei, Some comments on the changes below. Thanks, Patricio src/hotspot/share/prims/jvmtiThreadState.cpp line 310: > 308: ml.wait(10); // wait while there is an active suspender or resumer > 309: } > 310: Atomic::inc(&_VTMS_transition_disable_for_one_count); I don't understand the purpose of this counter. We only seem to write to it and the only place where it is checked is to do a notify, but I don't see a corresponding check-and-wait on that counter somewhere else, as we do with _VTMS_transition_disable_for_all_count. src/hotspot/share/prims/jvmtiThreadState.cpp line 315: > 313: ml.wait(10); // wait while the virtual thread is in transition > 314: } > 315: java_lang_Thread::inc_VTMS_transition_disable_count(vth()); I think this sequence coupled with how start_VTMS_transition() works can have races. So the check vstate->is_in_VTMS_transition() could return false, but before we execute inc_VTMS_transition_disable_count() the target could have already called and returned from start_VTMS_transition(). Just to test this I added a nanosleep() call in between those two, and an assert before returning from JvmtiVTMSTransitionDisabler::JvmtiVTMSTransitionDisabler(jthread thread) that the target is not in the middle of a transition. Running the serviceability/jvmti/vthread/ tests gives me some crashes in that assert. Test patch: https://github.com/pchilano/jdk/commit/9bc22771bf324f69bc4f5a9786f1b21937aab3c7 If I compare with how the "disable all" case works, in there, the target transitioning sets a variable(_VTMS_transition_count) and then reads another one(_VTMS_transition_disable_for_all_count) set by the transition disabler. The transition disabler does the same but with the variables reversed, i.e. sets _VTMS_transition_disable_for_all_count and then reads _VTMS_transition_count. This correctly makes sure that if we read the value that allow us to return from the method then we have already written the flag that will stop the other thread. But in this "single" case I don't see the same pattern. ------------- PR: https://git.openjdk.org/jdk/pull/11690
