Hi Richard, On 4/24/20 6:41 PM, Reingruber, Richard wrote:
Exactly. As of now we don't handle the case where another handshake operation on the same handshakee is called inside _handshake_cl->do_thread(). If this happens we will deadlock.Hi Patricio,@Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it?Because the VMThread would not be able to decrement _processing_sem to claim the operation and execute the closure for that handshakee. If another JavaThread is doing a direct handshake with that same handshakee and called a new VM operation inside the execution of the HandshakeClosure in do_handshake(), nobody would be able to decrement the _processing_sem anymore until the original direct operation finished and the semaphore is signaled again.Thanks, understood. On a higher level: a JavaThread can have at most one handshake operation being processed at at time.
Yes, that pattern actually relates with clearing _operation and predates direct handshakes. In theory we should always call do_handshake() first and then clear the handshake. This is what we do when the operation is processed by the handshaker, and it is necessary to be that way, otherwise if we clear the handshake first then the handshakee might transition from the safe state and never see that it actually has to stop for the handshake. Now, when the handshake operation is processed by the handshakee itself we don't have that issue, so it doesn't matter if we clear it before or after. The reason we do it before is to avoid the VMThread to execute unnecessary instructions in try_process(). This is specially true for the VM_HandshakeAllThreads operation case. If the VMThread sees that a JavaThread doesn't have an operation set, it can just continue to try to process the next JavaThread, instead of going through the unnecessary steps of checking the state of the JavaThread and trying to execute a try_wait() operation on the _processing_sem which we know won't succeed. Now for the direct handshake case doing it before or after doesn't really matter and so I just copied the pattern from the non-direct case to make it consistent in that same method.So this can happen despite the state of the handshakee is "handshake/safepoint safe". Changing the nested VM operation to be a direct handshake would have the same issue. Actually as the code is right now we would not even get pass setting the handshake operation because in that case we would block in the _handshake_turn_sem for the same reason.Don't really understand the details here, but that's ok. Interesting that _handshake_turn_sem gets signaled before or after do_handshake() depending if the handshake operation is processed by handshakee. Comments say "Disarm before/after executing operation" but not why :)
I tried to think of possible issues with that (independent of the closure logic) but I couldn't find a specific one. If the handshakee tries to process a pending handshake, process_by_self() will just return without calling process_self_inner() since it will detect it is already inside a handshake. And that behaviour makes sense since there is no point in trying to execute a new handshake operation if you are in the middle of another one. If the handshaker inside the closure checks for its own pending handshakes that also seems okay (this will by itself also check for safepoints in the transitions). Checking for safepoints in both cases seems more tricky but I couldn't think of a concrete issue with that. In any case I would also avoid checking for safepoints/handshakes inside the handshake closure. You might get issues related to the actual logic of the closure, like the typical deadlock because of trying to grab the same lock (although it's true that you always have to deal with that kind of problems when checking for safepoint/handshakes), or coming back from the safepoint/handshake and failing because some state you didn't expect to change in the middle of the handshake actually changed.So changing VM_SetFramePop to use direct handshakes in the future will probably create that last issue I mentioned. Now, since it is executed at a safepoint, with your workaround in enter_interp_only_mode() we avoid those nested issues in . Maybe 8239084 would have to be revisited to address nested operations in all cases. It is not clear to me now though if we should handle that in the handshake code or the caller of a certain operation should know it might be called in a nested scenario and should handle it.Last question: is it ok for the processor of a direct handshake operation to do safepoint/handshake checks? I wouldn't see a reason, why not. But certainly I would avoid it.
Thanks, Patricio
I'll look a bit more at the updated patch but at first glance looks good.Thanks, Richard. -----Original Message----- From: Patricio Chilano <patricio.chilano.ma...@oracle.com> Sent: Freitag, 24. April 2020 19:14 To: Reingruber, Richard <richard.reingru...@sap.com>; Yasumasa Suenaga <suen...@oss.nttdata.com>; serguei.spit...@oracle.com; Vladimir Ivanov <vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Richard, Just jumping into your last question for now. : ) On 4/24/20 1:08 PM, Reingruber, Richard wrote:Hi Yasumasa, Patricio,I will send review request to replace VM_SetFramePop to handshake in early next week in JDK-8242427. Does it help you? I think it gives you to remove workaround.I think it would not help that much. Note that when replacing VM_SetFramePop with a direct handshake you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. So you would have to change/replace VM_EnterInterpOnlyMode and I would have to adapt to these changes.Thanks for your information. I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and vmTestbase/nsk/jvmti/NotifyFramePop. I will modify and will test it after yours.Thanks :)Also my first impression was that it won't be that easy from a synchronization point of view to replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit() indirectly calls JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) where JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint. It's not directly clear to me, how this has to be handled.I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock because it affects other JVMTI operation especially FramePop event.Yes. To me it is unclear what synchronization is necessary, if it is called during a handshake. And also I'm unsure if a thread should do safepoint checks while executing a handshake. @Patricio, coming back to my question [1]: In the example you gave in your answer [2]: the java thread would execute a vm operation during a direct handshake operation, while the VMThread is actually in the middle of a VM_HandshakeAllThreads operation, waiting to handshake the same handshakee: why can't the VMThread just proceed? The handshakee would be safepoint safe, wouldn't it?Because the VMThread would not be able to decrement _processing_sem to claim the operation and execute the closure for that handshakee. If another JavaThread is doing a direct handshake with that same handshakee and called a new VM operation inside the execution of the HandshakeClosure in do_handshake(), nobody would be able to decrement the _processing_sem anymore until the original direct operation finished and the semaphore is signaled again. So this can happen despite the state of the handshakee is "handshake/safepoint safe". Changing the nested VM operation to be a direct handshake would have the same issue. Actually as the code is right now we would not even get pass setting the handshake operation because in that case we would block in the _handshake_turn_sem for the same reason. So changing VM_SetFramePop to use direct handshakes in the future will probably create that last issue I mentioned. Now, since it is executed at a safepoint, with your workaround in enter_interp_only_mode() we avoid those nested issues in . Maybe 8239084 would have to be revisited to address nested operations in all cases. It is not clear to me now though if we should handle that in the handshake code or the caller of a certain operation should know it might be called in a nested scenario and should handle it. I'll look a bit more at the updated patch but at first glance looks good. Thanks! PatricioThanks, Richard. [1] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301677&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301677 [2] https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14301763&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14301763 -----Original Message----- From: Yasumasa Suenaga <suen...@oss.nttdata.com> Sent: Freitag, 24. April 2020 17:23 To: Reingruber, Richard <richard.reingru...@sap.com>; Patricio Chilano <patricio.chilano.ma...@oracle.com>; serguei.spit...@oracle.com; Vladimir Ivanov <vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Richard, On 2020/04/24 23:44, Reingruber, Richard wrote:Hi Yasumasa,I will send review request to replace VM_SetFramePop to handshake in early next week in JDK-8242427. Does it help you? I think it gives you to remove workaround.I think it would not help that much. Note that when replacing VM_SetFramePop with a direct handshake you could not just execute VM_EnterInterpOnlyMode as a nested vm operation [1]. So you would have to change/replace VM_EnterInterpOnlyMode and I would have to adapt to these changes.Thanks for your information. I tested my patch with both vmTestbase/nsk/jvmti/PopFrame and vmTestbase/nsk/jvmti/NotifyFramePop. I will modify and will test it after yours.Also my first impression was that it won't be that easy from a synchronization point of view to replace VM_SetFramePop with a direct handshake. E.g. VM_SetFramePop::doit() indirectly calls JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) where JvmtiThreadState_lock is acquired with safepoint check, if not at safepoint. It's not directly clear to me, how this has to be handled.I think JvmtiEventController::set_frame_pop() should hold JvmtiThreadState_lock because it affects other JVMTI operation especially FramePop event. Thanks, YasumasaSo it appears to me that it would be easier to push JDK-8242427 after this (JDK-8238585).(The patch is available, but I want to see the result of PIT in this weekend whether JDK-8242425 works fine.)Would be interesting to see how you handled the issues above :) Thanks, Richard. [1] See question in comment https://bugs.openjdk.java.net/browse/JDK-8230594?focusedCommentId=14302030&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14302030 -----Original Message----- From: Yasumasa Suenaga <suen...@oss.nttdata.com> Sent: Freitag, 24. April 2020 13:34 To: Reingruber, Richard <richard.reingru...@sap.com>; Patricio Chilano <patricio.chilano.ma...@oracle.com>; serguei.spit...@oracle.com; Vladimir Ivanov <vladimir.x.iva...@oracle.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Richard, I will send review request to replace VM_SetFramePop to handshake in early next week in JDK-8242427. Does it help you? I think it gives you to remove workaround. (The patch is available, but I want to see the result of PIT in this weekend whether JDK-8242425 works fine.) Thanks, Yasumasa On 2020/04/24 17:18, Reingruber, Richard wrote:Hi Patricio, Vladimir, and Serguei, now that direct handshakes are available, I've updated the patch to make use of them. In addition I have done some clean-up changes I missed in the first webrev. Finally I have implemented the workaround suggested by Patricio to avoid nesting the handshake into the vm operation VM_SetFramePop [1] Kindly review again: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1/ Webrev(delta): http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.1.inc/ I updated the JBS item explaining why the vm operation VM_EnterInterpOnlyMode can be replaced with a direct handshake: JBS: https://bugs.openjdk.java.net/browse/JDK-8238585 Testing: * JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds on all platforms. * Submit-repo: mach5-one-rrich-JDK-8238585-20200423-1436-10441737 Thanks, Richard. [1] An assertion in Handshake::execute_direct() fails, if called be VMThread, because it is no JavaThread. -----Original Message----- From: hotspot-dev <hotspot-dev-boun...@openjdk.java.net> On Behalf Of Reingruber, Richard Sent: Freitag, 14. Februar 2020 19:47 To: Patricio Chilano <patricio.chilano.ma...@oracle.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: RE: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Patricio, > > I'm really glad you noticed the problematic nesting. This seems to be a general issue: currently a > > handshake cannot be nested in a vm operation. Maybe it should be asserted in the > > Handshake::execute() methods that they are not called by the vm thread evaluating a vm operation? > > > > > Alternatively I think you could do something similar to what we do in > > > Deoptimization::deoptimize_all_marked(): > > > > > > EnterInterpOnlyModeClosure hs; > > > if (SafepointSynchronize::is_at_safepoint()) { > > > hs.do_thread(state->get_thread()); > > > } else { > > > Handshake::execute(&hs, state->get_thread()); > > > } > > > (you could pass “EnterInterpOnlyModeClosure” directly to the > > > HandshakeClosure() constructor) > > > > Maybe this could be used also in the Handshake::execute() methods as general solution? > Right, we could also do that. Avoiding to clear the polling page in > HandshakeState::clear_handshake() should be enough to fix this issue and > execute a handshake inside a safepoint, but adding that "if" statement > in Hanshake::execute() sounds good to avoid all the extra code that we > go through when executing a handshake. I filed 8239084 to make that change. Thanks for taking care of this and creating the RFE. > > > > I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is > > > always called in a nested operation or just sometimes. > > > > At least one execution path without vm operation exists: > > > > JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : void > > JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState *) : jlong > > JvmtiEventControllerPrivate::recompute_enabled() : void > > JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent, bool) : void (2 matches) > > JvmtiEventController::change_field_watch(jvmtiEvent, bool) : void > > JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) : jvmtiError > > jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass, jfieldID) : jvmtiError > > > > I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main intent to replace it with a > > handshake, but to avoid making the compiled methods on stack not_entrant.... unless I'm further > > encouraged to do it with a handshake :) > Ah! I think you can still do it with a handshake with the > Deoptimization::deoptimize_all_marked() like solution. I can change the > if-else statement with just the Handshake::execute() call in 8239084. > But up to you. : ) Well, I think that's enough encouragement :) I'll wait for 8239084 and try then again. (no urgency and all) Thanks, Richard. -----Original Message----- From: Patricio Chilano <patricio.chilano.ma...@oracle.com> Sent: Freitag, 14. Februar 2020 15:54 To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Richard, On 2/14/20 9:58 AM, Reingruber, Richard wrote:Hi Patricio, thanks for having a look. > I’m only commenting on the handshake changes. > I see that operation VM_EnterInterpOnlyMode can be called inside > operation VM_SetFramePop which also allows nested operations. Here is a > comment in VM_SetFramePop definition: > > // Nested operation must be allowed for the VM_EnterInterpOnlyMode that is > // called from the JvmtiEventControllerPrivate::recompute_thread_enabled. > > So if we change VM_EnterInterpOnlyMode to be a handshake, then now we > could have a handshake inside a safepoint operation. The issue I see > there is that at the end of the handshake the polling page of the target > thread could be disarmed. So if the target thread happens to be in a > blocked state just transiently and wakes up then it will not stop for > the ongoing safepoint. Maybe I can file an RFE to assert that the > polling page is armed at the beginning of disarm_safepoint(). I'm really glad you noticed the problematic nesting. This seems to be a general issue: currently a handshake cannot be nested in a vm operation. Maybe it should be asserted in the Handshake::execute() methods that they are not called by the vm thread evaluating a vm operation? > Alternatively I think you could do something similar to what we do in > Deoptimization::deoptimize_all_marked(): > > EnterInterpOnlyModeClosure hs; > if (SafepointSynchronize::is_at_safepoint()) { > hs.do_thread(state->get_thread()); > } else { > Handshake::execute(&hs, state->get_thread()); > } > (you could pass “EnterInterpOnlyModeClosure” directly to the > HandshakeClosure() constructor) Maybe this could be used also in the Handshake::execute() methods as general solution?Right, we could also do that. Avoiding to clear the polling page in HandshakeState::clear_handshake() should be enough to fix this issue and execute a handshake inside a safepoint, but adding that "if" statement in Hanshake::execute() sounds good to avoid all the extra code that we go through when executing a handshake. I filed 8239084 to make that change.> I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is > always called in a nested operation or just sometimes. At least one execution path without vm operation exists: JvmtiEventControllerPrivate::enter_interp_only_mode(JvmtiThreadState *) : void JvmtiEventControllerPrivate::recompute_thread_enabled(JvmtiThreadState *) : jlong JvmtiEventControllerPrivate::recompute_enabled() : void JvmtiEventControllerPrivate::change_field_watch(jvmtiEvent, bool) : void (2 matches) JvmtiEventController::change_field_watch(jvmtiEvent, bool) : void JvmtiEnv::SetFieldAccessWatch(fieldDescriptor *) : jvmtiError jvmti_SetFieldAccessWatch(jvmtiEnv *, jclass, jfieldID) : jvmtiError I tend to revert back to VM_EnterInterpOnlyMode as it wasn't my main intent to replace it with a handshake, but to avoid making the compiled methods on stack not_entrant.... unless I'm further encouraged to do it with a handshake :)Ah! I think you can still do it with a handshake with the Deoptimization::deoptimize_all_marked() like solution. I can change the if-else statement with just the Handshake::execute() call in 8239084. But up to you. : ) Thanks, PatricioThanks again, Richard. -----Original Message----- From: Patricio Chilano <patricio.chilano.ma...@oracle.com> Sent: Donnerstag, 13. Februar 2020 18:47 To: Reingruber, Richard <richard.reingru...@sap.com>; serviceability-dev@openjdk.java.net; hotspot-compiler-...@openjdk.java.net; hotspot-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net; hotspot-gc-...@openjdk.java.net Subject: Re: RFR(S) 8238585: Use handshake for JvmtiEventControllerPrivate::enter_interp_only_mode() and don't make compiled methods on stack not_entrant Hi Richard, I’m only commenting on the handshake changes. I see that operation VM_EnterInterpOnlyMode can be called inside operation VM_SetFramePop which also allows nested operations. Here is a comment in VM_SetFramePop definition: // Nested operation must be allowed for the VM_EnterInterpOnlyMode that is // called from the JvmtiEventControllerPrivate::recompute_thread_enabled. So if we change VM_EnterInterpOnlyMode to be a handshake, then now we could have a handshake inside a safepoint operation. The issue I see there is that at the end of the handshake the polling page of the target thread could be disarmed. So if the target thread happens to be in a blocked state just transiently and wakes up then it will not stop for the ongoing safepoint. Maybe I can file an RFE to assert that the polling page is armed at the beginning of disarm_safepoint(). I think one option could be to remove SafepointMechanism::disarm_if_needed() in HandshakeState::clear_handshake() and let each JavaThread disarm itself for the handshake case. Alternatively I think you could do something similar to what we do in Deoptimization::deoptimize_all_marked(): EnterInterpOnlyModeClosure hs; if (SafepointSynchronize::is_at_safepoint()) { hs.do_thread(state->get_thread()); } else { Handshake::execute(&hs, state->get_thread()); } (you could pass “EnterInterpOnlyModeClosure” directly to the HandshakeClosure() constructor) I don’t know JVMTI code so I’m not sure if VM_EnterInterpOnlyMode is always called in a nested operation or just sometimes. Thanks, Patricio On 2/12/20 7:23 AM, Reingruber, Richard wrote:// Repost including hotspot runtime and gc lists. // Dean Long suggested to do so, because the enhancement replaces a vm operation // with a handshake. // Original thread: http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-February/030359.html Hi, could I please get reviews for this small enhancement in hotspot's jvmti implementation: Webrev: http://cr.openjdk.java.net/~rrich/webrevs/8238585/webrev.0/ Bug: https://bugs.openjdk.java.net/browse/JDK-8238585 The change avoids making all compiled methods on stack not_entrant when switching a java thread to interpreter only execution for jvmti purposes. It is sufficient to deoptimize the compiled frames on stack. Additionally a handshake is used instead of a vm operation to walk the stack and do the deoptimizations. Testing: JCK and JTREG tests, also in Xcomp mode with fastdebug and release builds on all platforms. Thanks, Richard. See also my question if anyone knows a reason for making the compiled methods not_entrant: http://mail.openjdk.java.net/pipermail/serviceability-dev/2020-January/030339.html
