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

[Robbin Ehn]

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