RFR 8242484: Rework thread deletion during VM termination

[David Holmes]

LGTM!

Thanks,
David

On 21/04/2020 11:22 am, Patricio Chilano wrote:
> 
> 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
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>
>