RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
David Holmes
david.holmes at oracle.com
Mon Aug 31 05:43:35 UTC 2020
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 ...
>
>>>>> 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.
>>>>> 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