On Fri, 23 Dec 2022 00:26:02 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> 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. > > I see the problem. Good catch, thanks! Yep my bad in earlier review. The one case needs the same synchronization/coordination as the all case. ------------- PR: https://git.openjdk.org/jdk/pull/11690
