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

Wed Aug 26 23:54:18 UTC 2020

Hi Robbin,

On 2020/08/27 0:08, Robbin Ehn wrote:
> Hi Yasumasa,
> 
> Thanks for fixing, seems good.
> Note that there are jdk tests for jdi which also runs this code under:
> test/jdk/com/sun/jdi/

webrev.02 passed test/jdk/com/sun/jdi/ on my Linux x64.

Thanks,

Yasumasa

> /Robbin
> 
> On 2020-08-26 16:33, Yasumasa Suenaga wrote:
>> Hi Robbin,
>>
>> I fixed them in new webrev. Could you review again?
>>
>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.02/
>>    diff from previous webrev: http://hg.openjdk.java.net/jdk/submit/rev/2ef7feb5681f
>>
>> It passed vmTestbase/nsk/{jdi,jdwp,jvmti} serviceability/{jdwp,jvmti} jtreg tests, so I think JVMTI functions works fine includes clear_frame_pop().
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2020/08/26 21:32, Robbin Ehn wrote:
>>> Hi Yasumasa,
>>>
>>> Yes that should work.
>>>
>>> Can you please add assert where you removed the:
>>> -  MutexLocker mu(JvmtiThreadState_lock);
>>> E.g.
>>> +  // If we are in a handshake we only know that the requesting thread should have locked it.
>>> +  assert(SafepointSynchronize::is_at_safepoint() || JvmtiThreadState_lock->is_locked(), "Safepoint or must be locked");
>>>
>>> Because I think you missing a MutexLocker in:
>>> jvmtiExport.cpp line ~1650:
>>>
>>>          // remove the frame's entry
>>>          ets->clear_frame_pop(cur_frame_number);
>>>
>>> In the method void JvmtiExport::post_method_exit(...).
>>>
>>> Thanks, Robbin
>>>
>>> On 2020-08-26 14:15, Yasumasa Suenaga wrote:
>>>> Hi Robbin,
>>>>
>>>> Thanks for your comment!
>>>>
>>>> How about this change?
>>>>
>>>>    webrev: http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.01/
>>>>    diff from previous webrev: http://hg.openjdk.java.net/jdk/submit/rev/7facd1dd39d6
>>>>
>>>> I still use JvmtiThreadState_lock because it has a different locking range from SR lock.
>>>>
>>>>
>>>> Thanks,
>>>>
>>>> Yasumasa
>>>>
>>>>
>>>> On 2020/08/26 18:13, Robbin Ehn wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> You cannot take the MutexLocker mu(JvmtiThreadState_lock) with safepoint checks inside a handshake.
>>>>> We are missing a NoSafepointVerifier for handshakes.
>>>>> (I have added this in my work in progress asynchronous handshake patch)
>>>>>
>>>>> Also this can deadlock with the handshake semaphore.
>>>>> (In my asynch handshake patch I have change the sema to a mutex, thus lock ranking works.)
>>>>>
>>>>> I solved this by just taking the mutex before the handshake.
>>>>> And removed the internal locking from set_frame_pop, etc...
>>>>> If there is an issue holding the JvmtiThreadState_lock to long, it should split to a per thread lock instead.
>>>>> (Since often the thread is suppose to be suspended, one could consider using the SR lock for serializing access to the per thread JvmtiThreadState instead.)
>>>>>
>>>>> Thanks, Robbin
>>>>>
>>>>> On 2020-08-26 09:34, 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.
>>>>>>
>>>>>>
>>>>>> 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
>>>>>>>