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

David Holmes david.holmes at oracle.com
Wed Sep 2 07:32:50 UTC 2020


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.

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?

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