RFR 8242484: Rework thread deletion during VM termination

[coleen.phillimore at oracle.com]

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.

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