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

serguei.spitsyn at oracle.com serguei.spitsyn at oracle.com
Fri Sep 4 08:38:26 UTC 2020


Hi Yasumasa,

Only PopFrame, ForceEarlyReturn, NotifyFramePop and ResumeThread require 
the target thread to be suspended.
Other JVMTI functions do not require it.

I'm still thinking if it is a good idea to get rid of the 
UpdateForPopTopFrameClosure and SetFramePopClosure hadnshakes.
It has to be safe because the target thread is suspended.
Grabbing the JvmtiThreadState_lock is good enough to ensure MT-safety.

Thanks,
Serguei


On 9/4/20 01:28, Yasumasa Suenaga wrote:
> Hi Serguei,
>
> Thanks for your review!
>
> On 2020/09/04 15:43, serguei.spitsyn at oracle.com wrote:
>> Hi Yasumasa,
>>
>> Thanks for the answer and sorry for the latency. It is not easy to 
>> double check everything related to the JvmtiThreadState_lock use.
>> You are right, it is not safe to remove the MutexLockers around the 
>> direct handshakes (unfortunately, I'm still floating with the 
>> handshake closures).
>>
>> I also had this question:
>> Q: Why do we need UpdateForPopTopFrameClosure and SetFramePopClosure 
>> if we already protect data with the JvmtiThreadState_lock?
>
> We need to suspend target thread (e.g. debugee) if we want to do frame 
> operation. I think we cannot do frame operation safety without 
> stopping target thread.
>
> So I chose direct handshake to avoid stopping the other threads.
>
>
> Thanks,
>
> Yasumasa
>
>
>> However, I'd be even morenervous to get rid of these closures as it 
>> is hard to predict all the consequences. The concern is the 
>> JvmtiEnvThreadState functions like get_frame_pops, has_frame_pos and 
>> is_frame_pop. They were designed to be called on the current thread 
>> or in a VMops/Handshake. Changing the _frame_pops on other threads 
>> without handshakes (even under protection of the 
>> JvmtiThreadState_lock) will break this design as these functions are 
>> not protected with the JvmtiThreadState_lock.
>>
>> So, I'm getting along with your sync approach now.
>> There is a similar sync pattern with the EnterInterpOnlyModeClosure 
>> and functions enter_interp_only_mode/leave_interp_only_mode which are 
>> called in the recompute_thread_enabled and recompute_enabled. The 
>> JvmtiThreadState_lock is also grabbed around this handshake closure.
>>
>> You fix moved MutexLockers from the JvmtiEventController functions 
>> set_frame_pop and clear_frame_pop to all their callers which should 
>> be fine in general. The function 
>> JvmtiEventController::clear_to_frame_pop is used by the 
>> JvmtiEnvThreadState::clear_to_frame_pop which in turn not used itself.
>>
>> So, I'm okay with the fix.
>> Thank you for your patience and answers!
>>
>> Thanks,
>> Serguei
>>
>>
>> On 9/3/20 02:01, Yasumasa Suenaga wrote:
>>> Hi Serguei,
>>>
>>> Your suggestion would not grab lock if it performs in direct 
>>> handshake, then other threads can enter these functions. Is it safe?
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2020/09/03 16:34, serguei.spitsyn at oracle.com wrote:
>>>> Hi Yasumasa,
>>>>
>>>> Wood it be MT-safe to do this to avoid locks at handshake time? :
>>>>
>>>>   JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, 
>>>> JvmtiFramePop fpop) {
>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>> +  MutexLocker mu(Thread::current == 
>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>>     JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>>>>   }
>>>>
>>>>
>>>>   void
>>>>   JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, 
>>>> JvmtiFramePop fpop) {
>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>> +  MutexLocker mu(Thread::current == 
>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>>     JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>>>>   }
>>>>
>>>>
>>>>   void
>>>>   JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState 
>>>> *ets, JvmtiFramePop fpop) {
>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>> +  MutexLocker mu(Thread::current == 
>>>> ets->get_thread()->active_handshaker() ? NULL : 
>>>> JvmtiThreadState_lock);
>>>>     JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>>>   }
>>>>
>>>>
>>>> Thanks,
>>>> Serguei
>>>>
>>>>
>>>> On 9/2/20 01:42, serguei.spitsyn at oracle.com wrote:
>>>>> Hi Yasumasa,
>>>>>
>>>>> Thank you for the explanation. At least, I see the real motivation.
>>>>> I've overlooked this in the email thread.
>>>>>
>>>>> Thanks,
>>>>> Serguei
>>>>>
>>>>>
>>>>> On 9/2/20 00:00, Yasumasa Suenaga wrote:
>>>>>> Hi Serguei,
>>>>>>
>>>>>> On 2020/09/02 15:29, David Holmes wrote:
>>>>>>> Hi Serguei,
>>>>>>>
>>>>>>> On 2/09/2020 4:11 pm, serguei.spitsyn at oracle.com wrote:
>>>>>>>> Hi Yasumasa,
>>>>>>>>
>>>>>>>> It seems to me your update for sync with the 
>>>>>>>> JvmtiThreadState_lock is incorrect.
>>>>>>>> Let me explain it.
>>>>>>>> The original design was that the functions is_frame_pop, 
>>>>>>>> set_frame_pop, clear_frame_pop and clear_to_frame_pop are 
>>>>>>>> always called either on the current thread or in a VMop.
>>>>>>>> There are 3 levels of these functions: in JvmtiEnvThreadState, 
>>>>>>>> JvmtiEventController and JvmtiEventControllerPrivate.
>>>>>>>> You already found the JvmtiThreadState_lock is grabbed in the 
>>>>>>>> JvmtiEventController versions of these functions.
>>>>>>>> It is for MT-safety of the recompute_thread_enabled() which can 
>>>>>>>> be called not only on current thread and VMop.
>>>>>>>
>>>>>>> Right, but now that we use a handshake, not a VMop, we have no 
>>>>>>> safepoint to guarantee MT-safety and so we have to use the lock 
>>>>>>> to ensure that.
>>>>>>
>>>>>> In addition we cannot take the MutexLocker inside a handshakes 
>>>>>> because we might see deadlock with the handshake semaphore.
>>>>>> So we have to grab JvmtiThreadState_lock before execute_direct().
>>>>>>
>>>>>> https://mail.openjdk.java.net/pipermail/serviceability-dev/2020-August/032754.html 
>>>>>>
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> Yasumasa
>>>>>>
>>>>>>
>>>>>>> David
>>>>>>> -----
>>>>>>>
>>>>>>>> So, I think adding MutexLocker's to the jvmtiEnv.cpp and 
>>>>>>>> jvmtiExport.cpp is not needed:
>>>>>>>>
>>>>>>>> +      MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>> +      if (java_thread == JavaThread::current()) {
>>>>>>>> +        state->update_for_pop_top_frame();
>>>>>>>> +      } else {
>>>>>>>> +        UpdateForPopTopFrameClosure op(state);
>>>>>>>>
>>>>>>>> . . .
>>>>>>>>
>>>>>>>> +  MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>>     if (java_thread == JavaThread::current()) {
>>>>>>>>       int frame_number = state->count_frames() - depth;
>>>>>>>> state->env_thread_state(this)->set_frame_pop(frame_number);
>>>>>>>>     } else {
>>>>>>>> -    VM_SetFramePop op(this, state, depth);
>>>>>>>> -    VMThread::execute(&op);
>>>>>>>> -    err = op.result();
>>>>>>>> +    SetFramePopClosure op(this, state, depth);
>>>>>>>> +    bool executed = Handshake::execute_direct(&op, java_thread);
>>>>>>>> +    err = executed ? op.result() : JVMTI_ERROR_THREAD_NOT_ALIVE;
>>>>>>>>
>>>>>>>> . . .
>>>>>>>>
>>>>>>>> + MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>>           ets->clear_frame_pop(cur_frame_number);
>>>>>>>>
>>>>>>>>
>>>>>>>> Instead, they have to be restored in the JvmtiEventController 
>>>>>>>> functions:
>>>>>>>>
>>>>>>>>   void
>>>>>>>> JvmtiEventController::set_frame_pop(JvmtiEnvThreadState *ets, 
>>>>>>>> JvmtiFramePop fpop) {
>>>>>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? 
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> +  assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>>     JvmtiEventControllerPrivate::set_frame_pop(ets, fpop);
>>>>>>>>   }
>>>>>>>>   void
>>>>>>>> JvmtiEventController::clear_frame_pop(JvmtiEnvThreadState *ets, 
>>>>>>>> JvmtiFramePop fpop) {
>>>>>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? 
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> +  assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::clear_frame_pop(ets, fpop);
>>>>>>>>   }
>>>>>>>>   void
>>>>>>>> JvmtiEventController::clear_to_frame_pop(JvmtiEnvThreadState 
>>>>>>>> *ets, JvmtiFramePop fpop) {
>>>>>>>> -  MutexLocker mu(SafepointSynchronize::is_at_safepoint() ? 
>>>>>>>> NULL : JvmtiThreadState_lock);
>>>>>>>> +  assert_lock_strong(JvmtiThreadState_lock);
>>>>>>>> JvmtiEventControllerPrivate::clear_to_frame_pop(ets, fpop);
>>>>>>>>   }
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Serguei
>>>>>>>>
>>>>>>>>
>>>>>>>> On 9/1/20 21:34, serguei.spitsyn at oracle.com wrote:
>>>>>>>>> Hi Yasumasa,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 9/1/20 21:17, Yasumasa Suenaga wrote:
>>>>>>>>>> Hi David,
>>>>>>>>>>
>>>>>>>>>> On 2020/09/02 13:13, David Holmes wrote:
>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>
>>>>>>>>>>> On 31/08/2020 7:10 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>
>>>>>>>>>>>> I uploaded new webrev. Could you review again?
>>>>>>>>>>>>
>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.04/
>>>>>>>>>>>>
>>>>>>>>>>>> This webrev includes two changes:
>>>>>>>>>>>>
>>>>>>>>>>>>    1. Use assert_lock_strong() for JvmtiThreadState_lock
>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/c85f93d2042d
>>>>>>>>>>>>
>>>>>>>>>>>>    2. Check return value from execute_direct() with assert()
>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/8746e1651343
>>>>>>>>>>>
>>>>>>>>>>> The message for the assertion:
>>>>>>>>>>>
>>>>>>>>>>> assert(executed, "Direct handshake failed. Target thread is 
>>>>>>>>>>> still alive?");
>>>>>>>>>>>
>>>>>>>>>>> should be phrased:
>>>>>>>>>>>
>>>>>>>>>>> assert(executed, "Direct handshake failed. Target thread is 
>>>>>>>>>>> not alive?");
>>>>>>>>>>>
>>>>>>>>>>> otherwise it sounds like the expectation is that it should 
>>>>>>>>>>> not be alive.
>>>>>>>>>>>
>>>>>>>>>>> Other changes fine.
>>>>>>>>>>>
>>>>>>>>>>> No need to see updated webrev.
>>>>>>>>>>
>>>>>>>>>> Thanks for your review!
>>>>>>>>>> I will fix them before pushing.
>>>>>>>>>
>>>>>>>>> Please, hold on.
>>>>>>>>> I'm still reviewing this.
>>>>>>>>> It is not clear yet if sync with the JvmtiThreadState_lock is 
>>>>>>>>> fully correct.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Serguei
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yasumasa
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> David
>>>>>>>>>>> -----
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2020/08/31 15:22, Yasumasa Suenaga wrote:
>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2020/08/31 14:43, David Holmes wrote:
>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 28/08/2020 1:01 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2020/08/28 11:04, David Holmes wrote:
>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 28/08/2020 11:24 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2020/08/27 15:49, David Holmes wrote:
>>>>>>>>>>>>>>>>>> Sorry I just realized I reviewed version 00 :(
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Note that my comments on version 00 in my earlier email 
>>>>>>>>>>>>>>>> still apply.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I copied here your comment on webrev.00:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I see. It is a pity that we have now lost that 
>>>>>>>>>>>>>>>>>>> critical indicator that shows how this operation can 
>>>>>>>>>>>>>>>>>>> be nested within another operation. The possibility 
>>>>>>>>>>>>>>>>>>> of nesting is even more obscure with 
>>>>>>>>>>>>>>>>>>> JvmtiEnvThreadState::reset_current_location. And the 
>>>>>>>>>>>>>>>>>>> fact it is now up to the caller to handle that case 
>>>>>>>>>>>>>>>>>>> explicitly raises some concern - what will happen if 
>>>>>>>>>>>>>>>>>>> you call execute_direct whilst already in a 
>>>>>>>>>>>>>>>>>>> handshake with the target thread?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I heard deadlock would be happen if execute_direct() 
>>>>>>>>>>>>>>> calls in direct handshake. Thus we need to use 
>>>>>>>>>>>>>>> active_handshaker() in this change.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Okay. This is something we need to clarify with direct 
>>>>>>>>>>>>>> handshake usage information. I think it would be 
>>>>>>>>>>>>>> preferable if this was handled in execute_direct rather 
>>>>>>>>>>>>>> than the caller ... though it may also be the case that 
>>>>>>>>>>>>>> we need the writer of the handshake operation to give due 
>>>>>>>>>>>>>> consideration to nesting ...
>>>>>>>>>>>>>
>>>>>>>>>>>>> Agree, I also prefer to check whether caller is in direct 
>>>>>>>>>>>>> handshake in execute_direct().
>>>>>>>>>>>>> But I think this is another enhancement because we need to 
>>>>>>>>>>>>> change the behavior of execute_direct().
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Further comments:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   194 #ifdef ASSERT
>>>>>>>>>>>>>>>>>>>   195   Thread *current = Thread::current();
>>>>>>>>>>>>>>>>>>>   196 #endif
>>>>>>>>>>>>>>>>>>>   197   assert(get_thread() == current || current == 
>>>>>>>>>>>>>>>>>>> get_thread()->active_handshaker(),
>>>>>>>>>>>>>>>>>>>   198          "frame pop data only accessible from 
>>>>>>>>>>>>>>>>>>> same thread or direct handshake");
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Can you factor this out into a separate function so 
>>>>>>>>>>>>>>>>>>> that it is not repeated so often. Seems to me that 
>>>>>>>>>>>>>>>>>>> there should be a global function on Thread: 
>>>>>>>>>>>>>>>>>>> assert_current_thread_or_handshaker() [yes 
>>>>>>>>>>>>>>>>>>> unpleasant name but ...] that will allow us to stop 
>>>>>>>>>>>>>>>>>>> repeating this code fragment across numerous files. 
>>>>>>>>>>>>>>>>>>> A follow up RFE for that would be okay too (I see 
>>>>>>>>>>>>>>>>>>> some guarantees that should probably just be asserts 
>>>>>>>>>>>>>>>>>>> so they need a bit more checking).
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I filed it as another RFE:
>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8252479
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   331 Handshake::execute_direct(&op, _thread);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> You aren't checking the return value of 
>>>>>>>>>>>>>>>>>>> execute_direct, but I can't tell where _thread was 
>>>>>>>>>>>>>>>>>>> checked for still being alive ??
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   340 Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I know this is existing code but I have the same 
>>>>>>>>>>>>>>>>>>> query as above - no return value check and no clear 
>>>>>>>>>>>>>>>>>>> check that the JavaThread is still alive?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Existing code seems to assume that target thread is 
>>>>>>>>>>>>>>> alive, frame operations (e.g. PopFrame()) should be 
>>>>>>>>>>>>>>> performed on live thread. And also existing code would 
>>>>>>>>>>>>>>> not set any JVMTI error and cannot propagate it to 
>>>>>>>>>>>>>>> caller. So I do not add the check for thread state.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Okay. But note that for PopFrame the tests for isAlive 
>>>>>>>>>>>>>> and is-suspended have already been performed before we do 
>>>>>>>>>>>>>> the execute_direct; so in that case we could simply 
>>>>>>>>>>>>>> assert that execute_direct returns true. Similarly for 
>>>>>>>>>>>>>> other cases.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Ok, I will change as following in next webrev:
>>>>>>>>>>>>>
>>>>>>>>>>>>> ```
>>>>>>>>>>>>> bool result = Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>> guarantee(result, "Direct handshake failed. Target thread 
>>>>>>>>>>>>> is still alive?");
>>>>>>>>>>>>> ```
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Do we know if the existing tests actually test the 
>>>>>>>>>>>>>>>>>>> nested cases?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I saw some error with assertion for 
>>>>>>>>>>>>>>> JvmtiThreadState_lock and safepoint in vmTestbase at 
>>>>>>>>>>>>>>> first, so I guess nested call would be tested, but I'm 
>>>>>>>>>>>>>>> not sure.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> I have concerns with the added locking:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> MutexLocker mu(JvmtiThreadState_lock);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Who else may be holding that lock? Could it be our 
>>>>>>>>>>>>>>>>>> target thread that we have already initiated a 
>>>>>>>>>>>>>>>>>> handshake with? (The lock ranking checks related to 
>>>>>>>>>>>>>>>>>> safepoints don't help us detect deadlocks between a 
>>>>>>>>>>>>>>>>>> target thread and its handshaker. :( )
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I checked source code again, then I couldn't find the 
>>>>>>>>>>>>>>>>> point that target thread already locked 
>>>>>>>>>>>>>>>>> JvmtiThreadState_lock at direct handshake.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> I'm very unclear exactly what state this lock guards 
>>>>>>>>>>>>>>>> and under what conditions. But looking at:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnv.cpp
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Surely the lock is only needed in the direct-handshake 
>>>>>>>>>>>>>>>> case and not when operating on the current thread? Or 
>>>>>>>>>>>>>>>> is it there because you've removed the locking from the 
>>>>>>>>>>>>>>>> lower-level JvmtiEventController methods and so now you 
>>>>>>>>>>>>>>>> need to take the lock higher-up the call chain? (I find 
>>>>>>>>>>>>>>>> it hard to follow the call chains in the JVMTI code.)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> We need to take the lock higher-up the call chain. It is 
>>>>>>>>>>>>>>> suggested by Robbin, and works fine.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Okay. It seems reasonably safe in this context as there 
>>>>>>>>>>>>>> is little additional work done while holding the lock.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> It is far from clear now which functions are 
>>>>>>>>>>>>>>>>>> reachable from handshakes, which from safepoint 
>>>>>>>>>>>>>>>>>> VM_ops and which from both.
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> ! assert(SafepointSynchronize::is_at_safepoint() || 
>>>>>>>>>>>>>>>>>> JvmtiThreadState_lock->is_locked(), "Safepoint or 
>>>>>>>>>>>>>>>>>> must be locked");
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> This can be written as:
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> assert_locked_or_safepoint(JvmtiThreadState_lock);
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> or possibly the weak variant of that. ('m puzzled by 
>>>>>>>>>>>>>>>>>> the extra check in the strong version ... I think it 
>>>>>>>>>>>>>>>>>> is intended for the case of the VMThread executing a 
>>>>>>>>>>>>>>>>>> non-safepoint VMop.)
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> JvmtiEventController::set_frame_pop(), 
>>>>>>>>>>>>>>>>>> JvmtiEventController::clear_frame_pop() and 
>>>>>>>>>>>>>>>>>> JvmtiEventController::clear_to_frame_pop() are no 
>>>>>>>>>>>>>>>>>> longer called at safepoint, so I remove safepoint 
>>>>>>>>>>>>>>>>>> check from assert() in new webrev.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> You should use assert_lock_strong for this.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I will do that.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>> David
>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>    webrev: 
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.03/ 
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>      diff from previous webrev: 
>>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk/submit/rev/2a2c02ada080
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>> On 27/08/2020 4:34 pm, David Holmes wrote:
>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 27/08/2020 9:40 am, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>> Hi David,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> On 2020/08/27 8:09, David Holmes wrote:
>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> On 26/08/2020 5:34 pm, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>> Hi Patricio, David,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Thanks for your comment!
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I updated webrev which includes the fix which is 
>>>>>>>>>>>>>>>>>>>>>> commented by Patricio, and it passed submit repo. 
>>>>>>>>>>>>>>>>>>>>>> So I switch this mail thread to RFR.
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>    JBS: 
>>>>>>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8242427
>>>>>>>>>>>>>>>>>>>>>>    webrev: 
>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/webrev.00/ 
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> I understand David said same concerns as Patricio 
>>>>>>>>>>>>>>>>>>>>>> about active handshaker. This webrev checks 
>>>>>>>>>>>>>>>>>>>>>> active handshaker is current thread or not.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> How can the current thread already be in a 
>>>>>>>>>>>>>>>>>>>>> handshake with the target when you execute this code?
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> EnterInterpOnlyModeClosure might be called in 
>>>>>>>>>>>>>>>>>>>> handshake with UpdateForPopTopFrameClosure or with 
>>>>>>>>>>>>>>>>>>>> SetFramePopClosure.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> EnterInterpOnlyModeClosure is introduced in 
>>>>>>>>>>>>>>>>>>>> JDK-8238585 as an alternative in 
>>>>>>>>>>>>>>>>>>>> VM_EnterInterpOnlyMode.
>>>>>>>>>>>>>>>>>>>> VM_EnterInterpOnlyMode returned true in 
>>>>>>>>>>>>>>>>>>>> allow_nested_vm_operations(). Originally, it could 
>>>>>>>>>>>>>>>>>>>> have been called from other VM operations.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I see. It is a pity that we have now lost that 
>>>>>>>>>>>>>>>>>>> critical indicator that shows how this operation can 
>>>>>>>>>>>>>>>>>>> be nested within another operation. The possibility 
>>>>>>>>>>>>>>>>>>> of nesting is even more obscure with 
>>>>>>>>>>>>>>>>>>> JvmtiEnvThreadState::reset_current_location. And the 
>>>>>>>>>>>>>>>>>>> fact it is now up to the caller to handle that case 
>>>>>>>>>>>>>>>>>>> explicitly raises some concern - what will happen if 
>>>>>>>>>>>>>>>>>>> you call execute_direct whilst already in a 
>>>>>>>>>>>>>>>>>>> handshake with the target thread?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I can't help but feel that we need a more rigorous 
>>>>>>>>>>>>>>>>>>> and automated way of dealing with nesting ... 
>>>>>>>>>>>>>>>>>>> perhaps we don't even need to care and handshakes 
>>>>>>>>>>>>>>>>>>> should always allow nested handshake requests? 
>>>>>>>>>>>>>>>>>>> (Question more for Robbin and Patricio.)
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Further comments:
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   194 #ifdef ASSERT
>>>>>>>>>>>>>>>>>>>   195   Thread *current = Thread::current();
>>>>>>>>>>>>>>>>>>>   196 #endif
>>>>>>>>>>>>>>>>>>>   197   assert(get_thread() == current || current == 
>>>>>>>>>>>>>>>>>>> get_thread()->active_handshaker(),
>>>>>>>>>>>>>>>>>>>   198          "frame pop data only accessible from 
>>>>>>>>>>>>>>>>>>> same thread or direct handshake");
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Can you factor this out into a separate function so 
>>>>>>>>>>>>>>>>>>> that it is not repeated so often. Seems to me that 
>>>>>>>>>>>>>>>>>>> there should be a global function on Thread: 
>>>>>>>>>>>>>>>>>>> assert_current_thread_or_handshaker() [yes 
>>>>>>>>>>>>>>>>>>> unpleasant name but ...] that will allow us to stop 
>>>>>>>>>>>>>>>>>>> repeating this code fragment across numerous files. 
>>>>>>>>>>>>>>>>>>> A follow up RFE for that would be okay too (I see 
>>>>>>>>>>>>>>>>>>> some guarantees that should probably just be asserts 
>>>>>>>>>>>>>>>>>>> so they need a bit more checking).
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   331 Handshake::execute_direct(&op, _thread);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> You aren't checking the return value of 
>>>>>>>>>>>>>>>>>>> execute_direct, but I can't tell where _thread was 
>>>>>>>>>>>>>>>>>>> checked for still being alive ??
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>   340 Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I know this is existing code but I have the same 
>>>>>>>>>>>>>>>>>>> query as above - no return value check and no clear 
>>>>>>>>>>>>>>>>>>> check that the JavaThread is still alive?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Do we know if the existing tests actually test the 
>>>>>>>>>>>>>>>>>>> nested cases?
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> David
>>>>>>>>>>>>>>>>>>>>> -----
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Cheers,
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>> On 2020/08/26 10:13, Patricio Chilano wrote:
>>>>>>>>>>>>>>>>>>>>>>> Hi Yasumasa,
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> On 8/23/20 11:40 PM, Yasumasa Suenaga wrote:
>>>>>>>>>>>>>>>>>>>>>>>> Hi all,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I want to hear your opinions about the change 
>>>>>>>>>>>>>>>>>>>>>>>> for JDK-8242427.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> I'm trying to migrate following operations to 
>>>>>>>>>>>>>>>>>>>>>>>> direct handshake.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>>     - VM_UpdateForPopTopFrame
>>>>>>>>>>>>>>>>>>>>>>>>     - VM_SetFramePop
>>>>>>>>>>>>>>>>>>>>>>>>     - VM_GetCurrentLocation
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Some operations (VM_GetCurrentLocation and 
>>>>>>>>>>>>>>>>>>>>>>>> EnterInterpOnlyModeClosure) might be called at 
>>>>>>>>>>>>>>>>>>>>>>>> safepoint, so I want to use 
>>>>>>>>>>>>>>>>>>>>>>>> JavaThread::active_handshaker() in production 
>>>>>>>>>>>>>>>>>>>>>>>> VM to detect the process is in direct handshake 
>>>>>>>>>>>>>>>>>>>>>>>> or not.
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> However this function is available in debug VM 
>>>>>>>>>>>>>>>>>>>>>>>> only, so I want to hear the reason why it is 
>>>>>>>>>>>>>>>>>>>>>>>> for debug VM only, and there are no problem to 
>>>>>>>>>>>>>>>>>>>>>>>> use it in production VM. Of course another 
>>>>>>>>>>>>>>>>>>>>>>>> solutions are welcome.
>>>>>>>>>>>>>>>>>>>>>>> I added the _active_handshaker field to the 
>>>>>>>>>>>>>>>>>>>>>>> HandshakeState class when working on 8230594 to 
>>>>>>>>>>>>>>>>>>>>>>> adjust some asserts, where instead of checking 
>>>>>>>>>>>>>>>>>>>>>>> for the VMThread we needed to check for the 
>>>>>>>>>>>>>>>>>>>>>>> active handshaker of the target JavaThread. 
>>>>>>>>>>>>>>>>>>>>>>> Since there were no other users of it, there was 
>>>>>>>>>>>>>>>>>>>>>>> no point in declaring it and having to write to 
>>>>>>>>>>>>>>>>>>>>>>> it for the release bits. There are no issues 
>>>>>>>>>>>>>>>>>>>>>>> with having it in production though so you could 
>>>>>>>>>>>>>>>>>>>>>>> change that if necessary.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> webrev is here. It passed jtreg tests 
>>>>>>>>>>>>>>>>>>>>>>>> (vmTestbase/nsk/{jdi,jdwp,jvmti} 
>>>>>>>>>>>>>>>>>>>>>>>> serviceability/{jdwp,jvmti})
>>>>>>>>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8242427/proposal/ 
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Some comments on the proposed change.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEnvThreadState.cpp, 
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiEventController.cpp
>>>>>>>>>>>>>>>>>>>>>>> Why is the check to decide whether to call the 
>>>>>>>>>>>>>>>>>>>>>>> handshake or execute the operation with the 
>>>>>>>>>>>>>>>>>>>>>>> current thread different for 
>>>>>>>>>>>>>>>>>>>>>>> GetCurrentLocationClosure vs 
>>>>>>>>>>>>>>>>>>>>>>> EnterInterpOnlyModeClosure?
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> (GetCurrentLocationClosure)
>>>>>>>>>>>>>>>>>>>>>>> if ((Thread::current() == _thread) || 
>>>>>>>>>>>>>>>>>>>>>>> (_thread->active_handshaker() != NULL)) {
>>>>>>>>>>>>>>>>>>>>>>> op.do_thread(_thread);
>>>>>>>>>>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>>>>>>>>>>> Handshake::execute_direct(&op, _thread);
>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> vs
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> (EnterInterpOnlyModeClosure)
>>>>>>>>>>>>>>>>>>>>>>> if (target->active_handshaker() != NULL) {
>>>>>>>>>>>>>>>>>>>>>>> hs.do_thread(target);
>>>>>>>>>>>>>>>>>>>>>>> } else {
>>>>>>>>>>>>>>>>>>>>>>> Handshake::execute_direct(&hs, target);
>>>>>>>>>>>>>>>>>>>>>>> }
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> If you change VM_SetFramePop to use handshakes 
>>>>>>>>>>>>>>>>>>>>>>> then it seems you could reach 
>>>>>>>>>>>>>>>>>>>>>>> JvmtiEventControllerPrivate::enter_interp_only_mode() 
>>>>>>>>>>>>>>>>>>>>>>> with the current thread being the target.
>>>>>>>>>>>>>>>>>>>>>>> Also I think you want the second expression of 
>>>>>>>>>>>>>>>>>>>>>>> that check to be (target->active_handshaker() == 
>>>>>>>>>>>>>>>>>>>>>>> Thread::current()). So either you are the target 
>>>>>>>>>>>>>>>>>>>>>>> or the current active_handshaker for that 
>>>>>>>>>>>>>>>>>>>>>>> target. Otherwise active_handshaker() could be 
>>>>>>>>>>>>>>>>>>>>>>> not NULL because there is another JavaThread 
>>>>>>>>>>>>>>>>>>>>>>> handshaking the same target. Unless you are 
>>>>>>>>>>>>>>>>>>>>>>> certain that it can never happen, so if 
>>>>>>>>>>>>>>>>>>>>>>> active_handshaker() is not NULL it is always the 
>>>>>>>>>>>>>>>>>>>>>>> current thread, but even in that case this way 
>>>>>>>>>>>>>>>>>>>>>>> is safer.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> src/hotspot/share/prims/jvmtiThreadState.cpp
>>>>>>>>>>>>>>>>>>>>>>> The guarantee() statement exists in release 
>>>>>>>>>>>>>>>>>>>>>>> builds too so the "#ifdef ASSERT" directive 
>>>>>>>>>>>>>>>>>>>>>>> should be removed, otherwise "current" will not 
>>>>>>>>>>>>>>>>>>>>>>> be declared.
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>> Patricio
>>>>>>>>>>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>>>>> Yasumasa
>>>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>
>>>>
>>



More information about the serviceability-dev mailing list