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

[David Holmes]

Sorry I just realized I reviewed version 00 :(

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. :( )

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.)

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
>>>>>