On Tue, 5 Nov 2024 23:53:04 GMT, Patricio Chilano Mateo <pchilanom...@openjdk.org> wrote:
>> Yes, I see your idea to get rid of the pending unmount event code. Before >> commenting on that, note that we still need to check if the freeze failed to >> undo the transition, which would call for this RAII object that we currently >> have. So in line with your suggestion we could call `VTMS_vthread_mount()` >> in `~JvmtiUnmountBeginMark()` which would also do the right thing too. >> Something like this: >> https://github.com/pchilano/jdk/commit/1729b98f554469fedbbce52333eccea9d1c81514 >> We can go this simplified route, but note that we would post unmount/mount >> events even if we never unmounted or remounted because freeze failed. It's >> true that that is how it currently works when unmounting from Java fails, so >> I guess it's not new behavior. >> Maybe we could go with this simplified code now and work on it later. I >> think the unmount event should be always posted at the end of the >> transition, in `JvmtiVTMSTransitionDisabler::VTMS_unmount_end()`. I know >> that at that point we have already switched identity to the carrier, but >> does the specs say the event has to be posted in the context of the vthread? >> If we can do that then we could keep the simplified version and avoid this >> extra unmount/mount events. > > 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 ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21565#discussion_r1831367766
