Re: RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes

Fri, 04 Sep 2020 13:29:10 -0700 

On 8/31/20 5:10 AM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev. Could you review again?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/


src/hotspot/share/prims/jvmtiEnvBase.hpp
    No comments.

src/hotspot/share/prims/jvmtiEnv.cpp
    L1636: JvmtiEnv::PopFrame(JavaThread* java_thread) {
    L1719:       MutexLocker mu(JvmtiThreadState_lock);
    L1721:         state->update_for_pop_top_frame();
        Okay, this new JvmtiThreadState_lock grab replaces this old
        code from jvmtiEventController.cpp (update_for_pop_top_frame()
        calls clear_frame_pop()):
            old L989: JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {             old L990:   MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock); 

        I'm good with that.
    L1801:   // thread. All other usage needs to use a vm-safepoint-op for safety. 
        You forgot to update this comment for the new code:
             // thread. All other usage needs to use a handshake for safety. 

    L1771: JvmtiEnv::NotifyFramePop(JavaThread* java_thread, jint depth) {
    L1802:   MutexLocker mu(JvmtiThreadState_lock);
    L1805: state->env_thread_state(this)->set_frame_pop(frame_number);
        Okay, this new JvmtiThreadState_lock grab replaces this old
        code from jvmtiEventController.cpp
            old L982: JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {             old L983:   MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock); 

        I'm good with that.

src/hotspot/share/prims/jvmtiEnvBase.cpp
    old L1509:   ThreadsListHandle tlh;
        VM_UpdateForPopTopFrame::doit() held a ThreadsListHandle to keep
        the target JavaThread safe. UpdateForPopTopFrameClosure::do_thread()         doesn't have a ThreadsListHandle, but the closure is executed during 
        a handshake so do_thread() is only called if the target thread is
        in a handshake state, i.e., on a ThreadsList held by the handshake
        dispatch code.

    old L1520:   ThreadsListHandle tlh;
        VM_SetFramePop::doit() held a ThreadsListHandle to keep
        the target JavaThread safe. SetFramePopClosure::do_thread()
        doesn't have a ThreadsListHandle, but the closure is executed during 
        a handshake so do_thread() is only called if the target thread is
        in a handshake state, i.e., on a ThreadsList held by the handshake
        dispatch code.

src/hotspot/share/prims/jvmtiEnvThreadState.cpp
    L328 :      if ((current == _thread) || (_thread->active_handshaker() == current)) { 
        nit - please remove the excess parens so switch to this:
                if (current == _thread || _thread->active_handshaker() == current) { 

    L331:         bool executed = Handshake::execute_direct(&op, _thread);
        The 'executed' variable might be considered unused by the compiler
        in a non-ASSERT build, e.g., release build in Tier1 should catch this.
    L332:         assert(executed, "Direct handshake failed. Target thread is still alive?"); 
        Perhaps:
            ... Target thread must be alive."

        If I remember correctly, the target thread still has to be alive
        because we're dealing with a single-step or breakpoint event and
        it must be alive for those to be posted. It's been a long time
        since I've been down those code paths so you might want to make
        sure that's still true.

src/hotspot/share/prims/jvmtiEventController.cpp
    L337:   if ((target == current) || (target->active_handshaker() == current)) { 
        nit - please remove the excess parens so switch to this:
            if (target == current || target->active_handshaker() == current) { 

    L340:     bool executed = Handshake::execute_direct(&hs, target);
        The 'executed' variable might be considered unused by the compiler
        in a non-ASSERT build, e.g., release build in Tier1 should catch this.
    L341:     assert(executed, "Direct handshake failed. Target thread is still alive?"); 
        Perhaps:
            ... Target thread must be alive."

        If I remember correctly, the target thread still has to be alive
        because we're forcing the target thread to switch to interpreted
        mode. It's been a long time since I've been down those code paths
        so you might want to make sure that's still true.
    old L996: JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {     old L997:   MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock); 
        This function JvmtiEventController::clear_to_frame_pop() is only
        called by JvmtiEnvThreadState::clear_to_frame_pop(int frame_number).         I cannot find any code that calls JvmtiEnvThreadState::clear_to_frame_pop(). 

        JvmtiEnvThreadState::set_frame_pop(int frame_number) is
        called from jvmtiEnv.cpp and jvmtiEnvBase.cpp.
        JvmtiEnvThreadState::clear_frame_pop(int frame_number) is
        called from jvmtiExport.cpp and jvmtiThreadState.cpp.

        It looks like JvmtiEnvThreadState::clear_to_frame_pop() is
        unused to me, but another pair of eyes would be good. If it
        is unused, a followup bug for analysis and removal would be
        good. Just want to make sure that if the function was used
        in the past, that the removal was intentional rather than a
        typo. Easy to do with similarly named functions...

src/hotspot/share/prims/jvmtiExport.cpp
    L1563: void JvmtiExport::post_method_exit()
    L1649:           MutexLocker mu(JvmtiThreadState_lock);
    L1650:           ets->clear_frame_pop(cur_frame_number);
        Okay, this new JvmtiThreadState_lock grab replaces this old
        code from jvmtiEventController.cpp:
            old L989: JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, JvmtiFramePop fpop) {             old L990:   MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : JvmtiThreadState_lock); 

src/hotspot/share/prims/jvmtiThreadState.cpp
    L276:   guarantee(current == get_thread() || current == get_thread()->active_handshaker(), 
    L277:     "must be current thread or direct handshake");
        nit - please indent L277 so the double-quote lines with with the 'c' in current. 

src/hotspot/share/runtime/handshake.hpp
    No comments.

src/hotspot/share/runtime/handshake.cpp
    No comments.

src/hotspot/share/runtime/thread.hpp
    No comments.

src/hotspot/share/runtime/vmOperations.hpp
    No comments.


I think there are only nits in my comments so I don't really
need to see another webrev.



This webrev includes two changes:

  1. Use assert_lock_strong() for JvmtiThreadState_lock
       http://hg.openjdk.java.net/jdk/submit/rev/c85f93d2042d

  2. Check return value from execute_direct() with assert()
       http://hg.openjdk.java.net/jdk/submit/rev/8746e1651343


Thanks,

Yasumasa


On 2020/08/31 15:22, Yasumasa Suenaga wrote:

Hi David,

On 2020/08/31 14:43, David Holmes wrote:

Hi Yasumasa,

On 28/08/2020 1:01 pm, Yasumasa Suenaga wrote:

Hi David,

On 2020/08/28 11:04, David Holmes wrote:

Hi Yasumasa,

On 28/08/2020 11:24 am, Yasumasa Suenaga wrote:

Hi David,

On 2020/08/27 15:49, David Holmes wrote:

Sorry I just realized I reviewed version 00 :(


Note that my comments on version 00 in my earlier email still apply.


I copied here your comment on webrev.00:
I see. It is a pity that we have now lost that critical indicator that shows how this operation can be nested within another operation. The possibility of nesting is even more obscure with JvmtiEnvThreadState::reset_current_location. And the fact it is now up to the caller to handle that case explicitly raises some concern - what will happen if you call execute_direct whilst already in a handshake with the target thread?
I heard deadlock would be happen if execute_direct() calls in direct handshake. Thus we need to use active_handshaker() in this change.
Okay. This is something we need to clarify with direct handshake usage information. I think it would be preferable if this was handled in execute_direct rather than the caller ... though it may also be the case that we need the writer of the handshake operation to give due consideration to nesting ...
Agree, I also prefer to check whether caller is in direct handshake in execute_direct(). But I think this is another enhancement because we need to change the behavior of execute_direct(). 



Further comments:

src/hotspot/share/prims/jvmtiEnvThreadState.cpp

  194 #ifdef ASSERT
  195   Thread *current = Thread::current();
  196 #endif
  197   assert(get_thread() == current || current == get_thread()->active_handshaker(),   198          "frame pop data only accessible from same thread or direct handshake");
Can you factor this out into a separate function so that it is not repeated so often. Seems to me that there should be a global function on Thread: assert_current_thread_or_handshaker()  [yes unpleasant name but ...] that will allow us to stop repeating this code fragment across numerous files. A follow up RFE for that would be okay too (I see some guarantees that should probably just be asserts so they need a bit more checking). 

I filed it as another RFE:
   https://bugs.openjdk.java.net/browse/JDK-8252479


Thanks.




  331 Handshake::execute_direct(&op, _thread);
You aren't checking the return value of execute_direct, but I can't tell where _thread was checked for still being alive ?? 

---

src/hotspot/share/prims/jvmtiEventController.cpp

  340     Handshake::execute_direct(&hs, target);
I know this is existing code but I have the same query as above - no return value check and no clear check that the JavaThread is still alive?
Existing code seems to assume that target thread is alive, frame operations (e.g. PopFrame()) should be performed on live thread. And also existing code would not set any JVMTI error and cannot propagate it to caller. So I do not add the check for thread state.
Okay. But note that for PopFrame the tests for isAlive and is-suspended have already been performed before we do the execute_direct; so in that case we could simply assert that execute_direct returns true. Similarly for other cases. 

Ok, I will change as following in next webrev:

```
bool result = Handshake::execute_direct(&hs, target);
guarantee(result, "Direct handshake failed. Target thread is still alive?"); 
```


Thanks,

Yasumasa



Do we know if the existing tests actually test the nested cases?
I saw some error with assertion for JvmtiThreadState_lock and safepoint in vmTestbase at first, so I guess nested call would be tested, but I'm not sure. 



I have concerns with the added locking:

MutexLocker mu(JvmtiThreadState_lock);
Who else may be holding that lock? Could it be our target thread that we have already initiated a handshake with? (The lock ranking checks related to safepoints don't help us detect deadlocks between a target thread and its handshaker. :( )
I checked source code again, then I couldn't find the point that target thread already locked JvmtiThreadState_lock at direct handshake.
I'm very unclear exactly what state this lock guards and under what conditions. But looking at: 

src/hotspot/share/prims/jvmtiEnv.cpp
Surely the lock is only needed in the direct-handshake case and not when operating on the current thread? Or is it there because you've removed the locking from the lower-level JvmtiEventController methods and so now you need to take the lock higher-up the call chain? (I find it hard to follow the call chains in the JVMTI code.)
We need to take the lock higher-up the call chain. It is suggested by Robbin, and works fine.
Okay. It seems reasonably safe in this context as there is little additional work done while holding the lock.
It is far from clear now which functions are reachable from handshakes, which from safepoint VM_ops and which from both.
!   assert(SafepointSynchronize::is_at_safepoint() || JvmtiThreadState_lock->is_locked(), "Safepoint or must be locked"); 

This can be written as:

assert_locked_or_safepoint(JvmtiThreadState_lock);
or possibly the weak variant of that. ('m puzzled by the extra check in the strong version ... I think it is intended for the case of the VMThread executing a non-safepoint VMop.)
JvmtiEventController::set_frame_pop(), JvmtiEventController::clear_frame_pop() and JvmtiEventController::clear_to_frame_pop() are no longer called at safepoint, so I remove safepoint check from assert() in new webrev. 

You should use assert_lock_strong for this.


I will do that.


Thanks,
David
-----



Thanks,

Yasumasa



Thanks,
David
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.03/      diff from previous webrev: http://hg.openjdk.java.net/jdk/submit/rev/2a2c02ada080 


Thanks,

Yasumasa



Thanks,
David
-----


On 27/08/2020 4:34 pm, David Holmes wrote:

Hi Yasumasa,

On 27/08/2020 9:40 am, Yasumasa Suenaga wrote:

Hi David,

On 2020/08/27 8:09, David Holmes wrote:

Hi Yasumasa,

On 26/08/2020 5:34 pm, Yasumasa Suenaga wrote:

Hi Patricio, David,

Thanks for your comment!
I updated webrev which includes the fix which is commented by Patricio, and it passed submit repo. So I switch this mail thread to RFR. 

   JBS: https://bugs.openjdk.java.net/browse/JDK-8242427
   webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.00/
I understand David said same concerns as Patricio about active handshaker. This webrev checks active handshaker is current thread or not.
How can the current thread already be in a handshake with the target when you execute this code?
EnterInterpOnlyModeClosure might be called in handshake with UpdateForPopTopFrameClosure or with SetFramePopClosure.
EnterInterpOnlyModeClosure is introduced in JDK-8238585 as an alternative in VM_EnterInterpOnlyMode. VM_EnterInterpOnlyMode returned true in allow_nested_vm_operations(). Originally, it could have been called from other VM operations.
I see. It is a pity that we have now lost that critical indicator that shows how this operation can be nested within another operation. The possibility of nesting is even more obscure with JvmtiEnvThreadState::reset_current_location. And the fact it is now up to the caller to handle that case explicitly raises some concern - what will happen if you call execute_direct whilst already in a handshake with the target thread?
I can't help but feel that we need a more rigorous and automated way of dealing with nesting ... perhaps we don't even need to care and handshakes should always allow nested handshake requests? (Question more for Robbin and Patricio.) 

Further comments:

src/hotspot/share/prims/jvmtiEnvThreadState.cpp

  194 #ifdef ASSERT
  195   Thread *current = Thread::current();
  196 #endif
  197   assert(get_thread() == current || current == get_thread()->active_handshaker(),   198          "frame pop data only accessible from same thread or direct handshake");
Can you factor this out into a separate function so that it is not repeated so often. Seems to me that there should be a global function on Thread: assert_current_thread_or_handshaker()  [yes unpleasant name but ...] that will allow us to stop repeating this code fragment across numerous files. A follow up RFE for that would be okay too (I see some guarantees that should probably just be asserts so they need a bit more checking). 

  331         Handshake::execute_direct(&op, _thread);
You aren't checking the return value of execute_direct, but I can't tell where _thread was checked for still being alive ?? 

---

src/hotspot/share/prims/jvmtiEventController.cpp

  340     Handshake::execute_direct(&hs, target);
I know this is existing code but I have the same query as above - no return value check and no clear check that the JavaThread is still alive? 

---

Do we know if the existing tests actually test the nested cases?

Thanks,
David
-----



Thanks,

Yasumasa



David
-----



Cheers,

Yasumasa


On 2020/08/26 10:13, Patricio Chilano wrote:

Hi Yasumasa,

On 8/23/20 11:40 PM, Yasumasa Suenaga wrote:

Hi all,
I want to hear your opinions about the change for JDK-8242427.
I'm trying to migrate following operations to direct handshake. 

    - VM_UpdateForPopTopFrame
    - VM_SetFramePop
    - VM_GetCurrentLocation
Some operations (VM_GetCurrentLocation and EnterInterpOnlyModeClosure) might be called at safepoint, so I want to use JavaThread::active_handshaker() in production VM to detect the process is in direct handshake or not.
However this function is available in debug VM only, so I want to hear the reason why it is for debug VM only, and there are no problem to use it in production VM. Of course another solutions are welcome.
I added the _active_handshaker field to the HandshakeState class when working on 8230594 to adjust some asserts, where instead of checking for the VMThread we needed to check for the active handshaker of the target JavaThread. Since there were no other users of it, there was no point in declaring it and having to write to it for the release bits. There are no issues with having it in production though so you could change that if necessary.
webrev is here. It passed jtreg tests (vmTestbase/nsk/{jdi,jdwp,jvmti} serviceability/{jdwp,jvmti}) 
http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/proposal/

Some comments on the proposed change.
src/hotspot/share/prims/jvmtiEnvThreadState.cpp, src/hotspot/share/prims/jvmtiEventController.cpp Why is the check to decide whether to call the handshake or execute the operation with the current thread different for GetCurrentLocationClosure vs EnterInterpOnlyModeClosure? 

(GetCurrentLocationClosure)
if ((Thread::current() == _thread) || (_thread->active_handshaker() != NULL)) { 
      op.do_thread(_thread);
} else {
      Handshake::execute_direct(&op, _thread);
}

vs

(EnterInterpOnlyModeClosure)
if (target->active_handshaker() != NULL) {
     hs.do_thread(target);
} else {
     Handshake::execute_direct(&hs, target);
}
If you change VM_SetFramePop to use handshakes then it seems you could reach JvmtiEventControllerPrivate::enter_interp_only_mode() with the current thread being the target. Also I think you want the second expression of that check to be (target->active_handshaker() == Thread::current()). So either you are the target or the current active_handshaker for that target. Otherwise active_handshaker() could be not NULL because there is another JavaThread handshaking the same target. Unless you are certain that it can never happen, so if active_handshaker() is not NULL it is always the current thread, but even in that case this way is safer. 

src/hotspot/share/prims/jvmtiThreadState.cpp
The guarantee() statement exists in release builds too so the "#ifdef ASSERT" directive should be removed, otherwise "current" will not be declared. 

Thanks!

Patricio

Thanks,

Yasumasa

Reply via email to