RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
David Holmes
david.holmes at oracle.com
Wed Sep 2 09:14:56 UTC 2020
Hi Serguei,
I have to confess I am not at all clear eactly what interactions the
JvmtiThreadState_lock is supposed to serializing. :(
I'm no longer convinced that holding that lock in place of executing at
a safepoint is either necessary nor sufficient.
David
On 2/09/2020 6:38 pm, serguei.spitsyn at oracle.com wrote:
> Hi David,
>
>
> On 9/2/20 00:32, David Holmes wrote:
>> Hi Serguei,
>>
>> On 2/09/2020 5:10 pm, serguei.spitsyn at oracle.com wrote:
>>> Hi David,
>>>
>>>
>>> On 9/1/20 23: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.
>>>
>>> Thank you for the comment.
>>> My understanding is that a handshake (at least, direct) is an
>>> equivalent of the current thread.
>>> Is it correct?
>>
>> A direct handshake operation can be executed by either the target
>> thread or the handshaker.
>
> My understanding is that it has to be MT-safe either it is executed by
> the target thread or the handshaker.
> So, the functions is_frame_pop, set_frame_pop, clear_frame_pop and
> clear_to_frame_pop are always executed on the current/target thread or
> the handshaker.
> However, the concern is about some internal calls from these function,
> e.g. recompute_thread_enabled.
> It is why the JvmtiThreadState_lock sync is moved from the
> JvmtiEventControllerPrivate to be around calls the JvmtiEnvThreadState
> functions.
> I do not see any particular issues now but the change looks a little bit
> risky to me as it is easy to overlook problems.
> It is a pity the JvmtiThreadState_lock can not be used inside handshakes.
> I wonder if this can be allowed to restore the original sync approach.
> We may observe more such problems with handshakes in the future if we
> convert more VMops into handshakes.
>
>> Not sure what difference that makes though. The JvmtiThreadState_lock
>> is a very coarse-grained locked and presumably needs to be held across
>> any operation that might access or update any thread-state whilst
>> another thread could be doing the same. It would be better if this
>> were per-thread of course but that isn't the way it works AFAIK.
>> Are you suggesting the lock is only needed to protect access to the
>> same thread's thread-state?
>
> I'm not that concerned that a per-thread lock is not used to protect
> thread states.
> It seems to be okay unless some performance problems or deadlocks are
> encountered.
>
> Thanks,
> Serguei
>
>> David
>>
>>> Thanks,
>>> Serguei
>>>
>>>>
>>>> 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