RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
David Holmes
david.holmes at oracle.com
Fri Aug 28 02:04:04 UTC 2020
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 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.)
>
>> 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.
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