RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Wed Sep 2 08:38:06 UTC 2020


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