RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
serguei.spitsyn at oracle.com
serguei.spitsyn at oracle.com
Wed Sep 2 10:15:41 UTC 2020
Hi David,
I do not see any obvious problems yet.
But let me double-check everything tomorrow.
Thanks,
Serguei
On 9/2/20 02:14, David Holmes wrote:
> 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