'Re: RFR 8242484: Rework thread deletion during VM termination'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR 8242484: Rework thread deletion during VM termination
From:       Patricio Chilano <patricio.chilano.mateo () oracle ! com>
Date:       2020-04-21 3:43:37
Message-ID: de2f0c3b-d850-130f-2a46-7e4b75407b3a () oracle ! com
[Download RAW message or body]


On 4/20/20 10:46 PM, David Holmes wrote:
> LGTM!
Thanks David!

Patricio
> Thanks,
> David
> 
> On 21/04/2020 11:22 am, Patricio Chilano wrote:
> > 
> > On 4/20/20 8:30 PM, David Holmes wrote:
> > > On 20/04/2020 11:51 pm, coleen.phillimore@oracle.com wrote:
> > > > 
> > > > 
> > > > On 4/20/20 9:32 AM, coleen.phillimore@oracle.com wrote:
> > > > > 
> > > > > 
> > > > > On 4/18/20 9:53 AM, David Holmes wrote:
> > > > > > Hi Coleen,
> > > > > > 
> > > > > > On 18/04/2020 4:24 am, coleen.phillimore@oracle.com wrote:
> > > > > > > On 4/17/20 1:27 PM, Patricio Chilano wrote:
> > > > > > > > Hi Coleen,
> > > > > > > > 
> > > > > > > > Thanks for looking at this.
> > > > > > > > 
> > > > > > > > On 4/17/20 12:57 PM, coleen.phillimore@oracle.com wrote:
> > > > > > > > > 
> > > > > > > > > This looks good to me also, although I'd like a reprisal of 
> > > > > > > > > the comment before the delete thread line that says something 
> > > > > > > > > ilke:
> > > > > > > > > 
> > > > > > > > > // We are here after VM_Exit::set_vm_exited() so we can't call
> > > > > > > > > // thread->smr_delete() or we will block on the Threads_lock.
> > > > > > > > Ok, I added back the original comment:
> > > > > > > > 
> > > > > > > > diff --git a/src/hotspot/share/runtime/thread.cpp 
> > > > > > > > b/src/hotspot/share/runtime/thread.cpp
> > > > > > > > --- a/src/hotspot/share/runtime/thread.cpp
> > > > > > > > +++ b/src/hotspot/share/runtime/thread.cpp
> > > > > > > > @@ -4472,6 +4472,11 @@
> > > > > > > > // exit_globals() will delete tty
> > > > > > > > exit_globals();
> > > > > > > > 
> > > > > > > > +  // We are here after VM_Exit::set_vm_exited() so we can't call
> > > > > > > > +  // thread->smr_delete() or we will block on the Threads_lock.
> > > > > > > > +  // Deleting the shutdown thread here is safe because another
> > > > > > > > +  // JavaThread cannot have an active ThreadsListHandle for
> > > > > > > > +  // this JavaThread.
> > > > > > > > delete thread;
> > > > > > 
> > > > > > Putting the comment back exactly as-was doesn't fit in well given 
> > > > > > the new code we have introduced. If we want to explain this in 
> > > > > > detail then it should be IMO when we first hit the code that 
> > > > > > interacts with ThreadSMR:
> > > > > > 
> > > > > > // We are no longer on the main thread list but could still be 
> > > > > > in a
> > > > > > // secondary list where another thread may try to interact 
> > > > > > with us.
> > > > > > // So wait until all such interactions are complete before we 
> > > > > > bring
> > > > > > !  // the VM to the termination safepoint. Normally this would be 
> > > > > > done
> > > > > > +  // using thread->smr_delete() below where we delete the 
> > > > > > thread, but
> > > > > > +  // we can't call that after the termination safepoint is 
> > > > > > active as we
> > > > > > +  // will deadlock on the Threads_lock. Once all interactions are
> > > > > > +  // complete it is safe to directly delete the thread at any time.
> > > > > > ThreadsSMRSupport::wait_until_not_protected(thread);
> > > > > 
> > > > > I disagree.  I think someone will find the 'delete thread' and not 
> > > > > know why it's there alone without smr_delete(), and might not see 
> > > > > this big comment or realize how it applies to delete. I think the 
> > > > > 'delete thread' shouldn't be alone.
> > > > 
> > > > Or even like:
> > > > // Thread must call wait_until_not_protected() before 
> > > > termination safepoint first.  See above.
> > > > delete thread;
> > > > 
> > > > If you want to move the comment above.
> > > 
> > > That works for me.
> > That sounds good. Are you okay with this diff from v1?
> > 
> > http://cr.openjdk.java.net/~pchilanomate/8242484/v2/inc/webrev/ 
> > <http://cr.openjdk.java.net/~pchilanomate/8242484/v2/inc/webrev/src/hotspot/share/runtime/thread.cpp.udiff.html> \
> >  
> > 
> > Thanks!
> > 
> > Patricio
> > > Thanks,
> > > David
> > > 
> > > > Coleen
> > > > > 
> > > > > Coleen
> > > > > 
> > > > > 
> > > > > > 
> > > > > > Thanks,
> > > > > > David
> > > > > > -----
> > > > > > 
> > > > > > > > 
> > > > > > > > > But why would it block? ... I don't see what thread has the 
> > > > > > > > > Threads_lock. Is this comment (that I want put back) accurate?
> > > > > > > > It will block because the VMThread owns the Threads_lock. The 
> > > > > > > > VMThread will start the terminating safepoint and exit but will 
> > > > > > > > never release the lock.
> > > > > > > 
> > > > > > > Thanks for the explanation.  In this code, when we call 
> > > > > > > VMThread::wait_for_vm_thread_exit, that sets the 
> > > > > > > _should_terminate flag, wakes up the VMThread and the VMThread 
> > > > > > > will grab the Threads_lock for the final safepoint.  Therefore 
> > > > > > > below at "delete thread" we can't grab the Threads_lock.
> > > > > > > 
> > > > > > > Comment looks good.
> > > > > > > Thanks,
> > > > > > > Coleen
> > > > > > > > 
> > > > > > > > Patricio
> > > > > > > > > thanks,
> > > > > > > > > Coleen
> > > > > > > > > 
> > > > > > > > > On 4/17/20 11:09 AM, Robbin Ehn wrote:
> > > > > > > > > > Hi Patricio, looks good, thanks.
> > > > > > > > > > 
> > > > > > > > > > /Robbin
> > > > > > > > > > 
> > > > > > > > > > On 2020-04-14 22:07, Patricio Chilano wrote:
> > > > > > > > > > > Hi,
> > > > > > > > > > > 
> > > > > > > > > > > Please review this small follow up for 8240918 to always 
> > > > > > > > > > > delete the JavaThread that calls Threads::destroy_vm().
> > > > > > > > > > > In 8240918 I added a conditional check in 
> > > > > > > > > > > Threads::destroy_vm() to prevent deleting a JavaThread when 
> > > > > > > > > > > there are handshakers blocked in its handshake_turn_sem 
> > > > > > > > > > > semaphore.  We can actually avoid this issue altogether if 
> > > > > > > > > > > after removing the JavaThread from the active list and 
> > > > > > > > > > > before triggering the terminating safepoint, we wait until 
> > > > > > > > > > > the JavaThread is no longer protected by a ThreadsList 
> > > > > > > > > > > reference.
> > > > > > > > > > > 
> > > > > > > > > > > Tested in mach5 tiers 1-6.
> > > > > > > > > > > 
> > > > > > > > > > > Bug:
> > > > > > > > > > > https://bugs.openjdk.java.net/browse/JDK-8242484
> > > > > > > > > > > 
> > > > > > > > > > > Webrev:
> > > > > > > > > > > http://cr.openjdk.java.net/~pchilanomate/8242484/v1/webrev/
> > > > > > > > > > > 
> > > > > > > > > > > Thanks,
> > > > > > > > > > > Patricio
> > > > > > > > > 
> > > > > > > > 
> > > > > > > 
> > > > > 
> > > > 
> > 


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic