RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Fri Sep 4 08:38:26 UTC 2020
Hi Yasumasa,
Only PopFrame, ForceEarlyReturn, NotifyFramePop and ResumeThread require
the target thread to be suspended.
Other JVMTI functions do not require it.
I'm still thinking if it is a good idea to get rid of the
UpdateForPopTopFrameClosure and SetFramePopClosure hadnshakes.
It has to be safe because the target thread is suspended.
Grabbing the JvmtiThreadState_lock is good enough to ensure MT-safety.
Thanks,
Serguei
On 9/4/20 01:28, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Thanks for your review!
>
> On 2020/09/04 15:43, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thanks for the answer and sorry for the latency. It is not easy to
>> double check everything related to the JvmtiThreadState_lock use.
>> You are right, it is not safe to remove the MutexLockers around the
>> direct handshakes (unfortunately, I'm still floating with the
>> handshake closures).
>>
>> I also had this question:
>> Q: Why do we need UpdateForPopTopFrameClosure and SetFramePopClosure
>> if we already protect data with the JvmtiThreadState_lock?
>
> We need to suspend target thread (e.g. debugee) if we want to do frame
> operation. I think we cannot do frame operation safety without
> stopping target thread.
>
> So I chose direct handshake to avoid stopping the other threads.
>
>
> Thanks,
>
> Yasumasa
>
>
>> However, I'd be even morenervous to get rid of these closures as it
>> is hard to predict all the consequences. The concern is the
>> JvmtiEnvThreadState functions like get_frame_pops, has_frame_pos and
>> is_frame_pop. They were designed to be called on the current thread
>> or in a VMops/Handshake. Changing the _frame_pops on other threads
>> without handshakes (even under protection of the
>> JvmtiThreadState_lock) will break this design as these functions are
>> not protected with the JvmtiThreadState_lock.
>>
>> So, I'm getting along with your sync approach now.
>> There is a similar sync pattern with the EnterInterpOnlyModeClosure
>> and functions enter_interp_only_mode/leave_interp_only_mode which are
>> called in the recompute_thread_enabled and recompute_enabled. The
>> JvmtiThreadState_lock is also grabbed around this handshake closure.
>>
>> You fix moved MutexLockers from the JvmtiEventController functions
>> set_frame_pop and clear_frame_pop to all their callers which should
>> be fine in general. The function
>> JvmtiEventController::clear_to_frame_pop is used by the
>> JvmtiEnvThreadState::clear_to_frame_pop which in turn not used itself.
>>
>> So, I'm okay with the fix.
>> Thank you for your patience and answers!
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/3/20 02:01, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> Your suggestion would not grab lock if it performs in direct
>>> handshake, then other threads can enter these functions. Is it safe?
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/09/03 16:34, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Wood it be MT-safe to do this to avoid locks at handshake time? :
>>>>
>>>> JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets,
>>>> JvmtiFramePop fpop) {
>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> + MutexLocker mu(Thread::current ==
>>>> ets->get_thread()->active_handshaker() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>>>> }
>>>>
>>>>
>>>> void
>>>> JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets,
>>>> JvmtiFramePop fpop) {
>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> + MutexLocker mu(Thread::current ==
>>>> ets->get_thread()->active_handshaker() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>>>> }
>>>>
>>>>
>>>> void
>>>> JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState
>>>> *ets, JvmtiFramePop fpop) {
>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> + MutexLocker mu(Thread::current ==
>>>> ets->get_thread()->active_handshaker() ? NULL :
>>>> JvmtiThreadState_lock);
>>>> JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>>> }
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 9/2/20 01:42, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> Thank you for the explanation. At least, I see the real motivation.
>>>>> I've overlooked this in the email thread.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 9/2/20 00:00, Yasumasa Suenaga wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> On 2020/09/02 15:29, David Holmes wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> On 2/09/2020 4:11 pm, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> It seems to me your update for sync with the
>>>>>>>> JvmtiThreadState_lock is incorrect.
>>>>>>>> Let me explain it.
>>>>>>>> The original design was that the functions is_frame_pop,
>>>>>>>> set_frame_pop, clear_frame_pop and clear_to_frame_pop are
>>>>>>>> always called either on the current thread or in a VMop.
>>>>>>>> There are 3 levels of these functions: in JvmtiEnvThreadState,
>>>>>>>> JvmtiEventController and JvmtiEventControllerPrivate.
>>>>>>>> You already found the JvmtiThreadState_lock is grabbed in the
>>>>>>>> JvmtiEventController versions of these functions.
>>>>>>>> It is for MT-safety of the recompute_thread_enabled() which can
>>>>>>>> be called not only on current thread and VMop.
>>>>>>>
>>>>>>> Right, but now that we use a handshake, not a VMop, we have no
>>>>>>> safepoint to guarantee MT-safety and so we have to use the lock
>>>>>>> to ensure that.
>>>>>>
>>>>>> In addition we cannot take the MutexLocker inside a handshakes
>>>>>> because we might see deadlock with the handshake semaphore.
>>>>>> So we have to grab JvmtiThreadState_lock before execute_direct().
>>>>>>
>>>>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032754.html
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> So, I think adding MutexLocker's to the jvmtiEnv.cpp and
>>>>>>>> jvmtiExport.cpp is not needed:
>>>>>>>>
>>>>>>>> + MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>> + if (java_thread == JavaThread::current()) {
>>>>>>>> + state->update_for_pop_top_frame();
>>>>>>>> + } else {
>>>>>>>> + UpdateForPopTopFrameClosure op(state);
>>>>>>>>
>>>>>>>> . . .
>>>>>>>>
>>>>>>>> + MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>> if (java_thread == JavaThread::current()) {
>>>>>>>> int frame_number = state->count_frames() - depth;
>>>>>>>> state->env_thread_state(this)->set_frame_pop(frame_number);
>>>>>>>> } else {
>>>>>>>> - VM_SetFramePop op(this, state, depth);
>>>>>>>> - VMThread::execute(&op);
>>>>>>>> - err = op.result();
>>>>>>>> + SetFramePopClosure op(this, state, depth);
>>>>>>>> + bool executed = Handshake::execute_direct(&op, java_thread);
>>>>>>>> + err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
>>>>>>>>
>>>>>>>> . . .
>>>>>>>>
>>>>>>>> + MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>> ets->clear_frame_pop(cur_frame_number);
>>>>>>>>
>>>>>>>>
>>>>>>>> Instead, they have to be restored in the JvmtiEventController
>>>>>>>> functions:
>>>>>>>>
>>>>>>>> void
>>>>>>>> JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets,
>>>>>>>> JvmtiFramePop fpop) {
>>>>>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ?
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> + assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>>>>>>>> }
>>>>>>>> void
>>>>>>>> JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets,
>>>>>>>> JvmtiFramePop fpop) {
>>>>>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ?
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> + assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>>>>>>>> }
>>>>>>>> void
>>>>>>>> JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState
>>>>>>>> *ets, JvmtiFramePop fpop) {
>>>>>>>> - MutexLocker mu(SafepointSynchronize::is_at_safepoint() ?
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> + assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>>>>>>> }
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/1/20 21:34, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/1/20 21:17, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> On 2020/09/02 13:13, David Holmes wrote:
>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>
>>>>>>>>>>> On 31/08/2020 7:10 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>> I uploaded new webrev. Could you review again?
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/
>>>>>>>>>>>>
>>>>>>>>>>>> This webrev includes two changes:
>>>>>>>>>>>>
>>>>>>>>>>>> 1. Use assert_lock_strong() for JvmtiThreadState_lock
>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c85f93d2042d
>>>>>>>>>>>>
>>>>>>>>>>>> 2. Check return value from execute_direct() with assert()
>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/8746e1651343
>>>>>>>>>>>
>>>>>>>>>>> The message for the assertion:
>>>>>>>>>>>
>>>>>>>>>>> assert(executed, "Direct handshake failed. Target thread is
>>>>>>>>>>> still alive?");
>>>>>>>>>>>
>>>>>>>>>>> should be phrased:
>>>>>>>>>>>
>>>>>>>>>>> assert(executed, "Direct handshake failed. Target thread is
>>>>>>>>>>> not alive?");
>>>>>>>>>>>
>>>>>>>>>>> otherwise it sounds like the expectation is that it should
>>>>>>>>>>> not be alive.
>>>>>>>>>>>
>>>>>>>>>>> Other changes fine.
>>>>>>>>>>>
>>>>>>>>>>> No need to see updated webrev.
>>>>>>>>>>
>>>>>>>>>> Thanks for your review!
>>>>>>>>>> I will fix them before pushing.
>>>>>>>>>
>>>>>>>>> Please, hold on.
>>>>>>>>> I'm still reviewing this.
>>>>>>>>> It is not clear yet if sync with the JvmtiThreadState_lock is
>>>>>>>>> fully correct.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2020/08/31 15:22, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2020/08/31 14:43, David Holmes wrote:
>>>>>>>>>>>>>> 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 ...
>>>>>>>>>>>>>
>>>>>>>>>>>>> Agree, I also prefer to check whether caller is in direct
>>>>>>>>>>>>> handshake in execute_direct().
>>>>>>>>>>>>> But I think this is another enhancement because we need to
>>>>>>>>>>>>> change the behavior of execute_direct().
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ok, I will change as following in next webrev:
>>>>>>>>>>>>>
>>>>>>>>>>>>> ```
>>>>>>>>>>>>> bool result = Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>> guarantee(result, "Direct handshake failed. Target thread
>>>>>>>>>>>>> is still alive?");
>>>>>>>>>>>>> ```
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> 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