Hi Serguei,
For example, JvmtiThreadState_lock would be grabbed at
ciEnv::cache_jvmti_state() which is called by
CompileBroker::invoke_compiler_on_method(). Compiler thread might be touch the
protected resource even if target thread is suspended with direct handshake.
So I think they are needed both direct handshake and JvmtiThreadState_lock.
Thanks,
Yasumasa
On 2020/09/04 17:38, serguei.spit...@oracle.com wrote:
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.spit...@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.spit...@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.spit...@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.spit...@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.spit...@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
