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 ...
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.
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
