[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 8242427: JVMTI frame pop operations should use Thread-Local Handshakes
From: Yasumasa Suenaga <suenaga () oss ! nttdata ! com>
Date: 2020-08-31 9:10:33
Message-ID: 41b91a4d-4eed-766e-48e1-87e08c1c8611 () oss ! nttdata ! com
[Download RAW message or body]
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
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
> > > > > > > > > > >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic