RFR 8242484: Rework thread deletion during VM termination

[David Holmes]

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

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