On Wed, 6 Nov 2024 16:31:24 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Regarding the pop_frame/early_ret/async_exception conditions, not checking >> for them after we started the transition would be an issue. >> For pop_frame/early_ret checks, the problem is that if any of them are >> installed in `JvmtiUnmountBeginMark()` while trying to start the transition, >> and later the call to freeze succeeds, when returning to the interpreter >> (monitorenter case) we will incorrectly follow the JVMTI code [1], instead >> of going back to `call_VM_preemptable` to clear the stack from the copied >> frames. As for the asynchronous exception check, if it gets installed in >> `JvmtiUnmountBeginMark()` while trying to start the transition, the >> exception would be thrown in the carrier instead, very likely while >> executing the unmounting logic. >> When unmounting from Java, although the race is also there when starting the >> VTMS transition as you mentioned, I think the end result will be different. >> For pop_frame/early_ret we will just bail out if trying to install them >> since the top frame will be a native method (`notifyJvmtiUnmount`). For the >> async exception, we would process it on return from `notifyJvmtiUnmount` >> which would still be done in the context of the vthread. >> >> [1] >> https://github.com/openjdk/jdk/blob/471f112bca715d04304cbe35c6ed63df8c7b7fee/src/hotspot/cpu/x86/macroAssembler_x86.cpp#L1629 > > Thank you for the comment! I'm okay with your modified suggestion in general > if there are no road blocks. > >> but does the specs say the event has to be posted in the context of the >> vthread? > > As Alan said below we do not have an official spec for this but still the > events need to be posted in vthread context. > >> For pop_frame/early_ret checks ... > > The pop_frame/early_ret conditions are installed in handshakes with a context > of `JvmtiVTMSTransitionDisabler`. As you noted the `JVMTI_ERROR_OPAQUE_FRAME` > might be also returned by the JVMTI `FramePop` and `ForceEarlyReturn*` for > some specific cases. So, it feels like it should not be a problem. I'm > thinking if adding an assert at the VTMS transition end would help. > >> Maybe we could go with this simplified code now and work on it later... > > Whatever works better for you. An alternate approach could be to file an > enhancement to simplify/refactor this. > It would be nice to fix a couple of nits though: > - the call to `java_lang_Thread::set_is_in_VTMS_transition()`is not needed > in `JvmtiUnmountBeginMark` > - the function `is_vthread_safe_to_preempt()` does not need the `vthread` > parameter Great, I applied the suggested simplification. I had to update test `VThreadEventTest.java` to check the stack during the mount/unmount events to only count the real cases. This is because now we are getting a variable number of spurious mount/unmount events (freeze failed) generated during the initialization of some class (`VirtualThreadEndEvent`) after the task is finished. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1831898126
