RFR 8242484: Rework thread deletion during VM termination

[David Holmes]

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.

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