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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Sep 2 04:34:05 UTC 2020


Hi Yasumasa,


On 9/1/20 21:17, Yasumasa Suenaga wrote:
> Hi David,
>
> On 2020/09/02 13:13, David Holmes wrote:
>> Hi Yasumasa,
>>
>> On 31/08/2020 7:10 pm, Yasumasa Suenaga wrote:
>>> Hi David,
>>>
>>> I uploaded new webrev. Could you review again?
>>>
>>>    http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/
>>>
>>> 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
>>
>> The message for the assertion:
>>
>> assert(executed, "Direct handshake failed. Target thread is still 
>> alive?");
>>
>> should be phrased:
>>
>> assert(executed, "Direct handshake failed. Target thread is not 
>> alive?");
>>
>> otherwise it sounds like the expectation is that it should not be alive.
>>
>> Other changes fine.
>>
>> No need to see updated webrev.
>
> Thanks for your review!
> I will fix them before pushing.

Please, hold on.
I'm still reviewing this.
It is not clear yet if sync with the JvmtiThreadState_lock is fully correct.

Thanks,
Serguei

>
>
> Yasumasa
>
>
>> Thanks,
>> David
>> -----
>>
>>>
>>> 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
>>>>>>>>>>>>>>



More information about the serviceability-dev mailing list