RFR 8242484: Rework thread deletion during VM termination

[Patricio Chilano]

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 at oracle.com wrote:
>>>>
>>>>
>>>> On 4/20/20 9:32 AM, coleen.phillimore at oracle.com wrote:
>>>>>
>>>>>
>>>>> On 4/18/20 9:53 AM, David Holmes wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> On 18/04/2020 4:24 am, coleen.phillimore at 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 at 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
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>
>>