RFR 8242484: Rework thread deletion during VM termination

[coleen.phillimore at oracle.com]

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.

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