RFR 8242484: Rework thread deletion during VM termination

[Patricio Chilano]

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