RFR(xxs): 8213236: A partial removed/deleted JavaThread cannot transition

[Robbin Ehn]

Hi Dan,

On 01/11/2018 16:58, Daniel D. Daugherty wrote:
> On 11/1/18 7:57 AM, Robbin Ehn wrote:
>> Hi all please review,
>>
>> In 8212933 we moved the cancellation of a handshake to the VM thread, for sanity
>> an assert was added to make sure the JavaThread do not cancel. (technically this
>> works also, but to keep a simple model it should be done by the VM thread)
>>
>> A JavaThread that is terminated but not yet free should not follow the safepoint
>> protocol. Since the VM thread running the safepoint cannot see such a thread
>> it's confusing and error-prone doing safepoint checks and transitions from a
>> thread not 'attached' to the VM.
>>
>> Issue:
>> https://bugs.openjdk.java.net/browse/JDK-8213236
> 
> I updated the bug to be a bit more clear about what the problem is.
> Please let me know if my analysis is correct or not...

Yes, that is correct, thanks!

> 
> 
>>
>> Code:
>> diff -r 13ad2bfee99e src/hotspot/share/runtime/vmThread.cpp
>> --- a/src/hotspot/share/runtime/vmThread.cpp    Thu Nov 01 09:57:57 2018 +0100
>> +++ b/src/hotspot/share/runtime/vmThread.cpp    Thu Nov 01 12:40:47 2018 +0100
>> @@ -319,7 +319,9 @@
>>  // Notify the VMThread that the last non-daemon JavaThread has terminated,
>>  // and wait until operation is performed.
>>  void VMThread::wait_for_vm_thread_exit() {
>> -  { MutexLocker mu(VMOperationQueue_lock);
>> +  assert(Thread::current()->is_Java_thread(), "Should be a JavaThread");
>> +  assert(((JavaThread*)Thread::current())->is_terminated(), "Should be off 
>> threadslist");
> 
> This should be "should be terminated". is_terminated()
> doesn't necessarily mean the JavaThread is not on the
> global ThreadsList.

If the thread ask itself or if you hold Threads_locks it is implicit the same.
When we do exit we first take Threads_lock, remove our self from the global 
list, set state terminated and then unlock Threads_lock.

But I'm perfectly fine with "should be terminated" instead.

> 
> 
>> +  { MutexLockerEx mu(VMOperationQueue_lock, Mutex::_no_safepoint_check_flag);
> 
> Yup. I agree with this addition of Mutex::_no_safepoint_check_flag.
> 
> However, I think there is one more:
> 
> src/hotspot/share/runtime/thread.cpp:
>    before_exit(thread);
> 
>    thread->exit(true);
> 
>    // Stop VM thread.
>    {
>      // 4945125 The vm thread comes to a safepoint during exit.
>      // GC vm_operations can get caught at the safepoint, and the
>      // heap is unparseable if they are caught. Grab the Heap_lock
>      // to prevent this. The GC vm_operations will not be able to
>      // queue until after the vm thread is dead. After this point,
>      // we'll never emerge out of the safepoint before the VM exits.
> 
>      MutexLocker ml(Heap_lock);
> 
>      VMThread::wait_for_vm_thread_exit();
> 
> You fixed the one in wait_for_vm_thread_exit(), but I think
> that this: "MutexLocker ml(Heap_lock)" is also a problem.

Silly me missed that one :)
Yes, but it is used in vm op prolog meaning that VM thread takes it.
So if we hold it we can deadlock with vm thread.
E.g.
vmGCOperations.cpp L290
vm_operations_g1.cpp L214

I don't understand this comment:
"GC vm_operations can get caught at the safepoint, and the heap is unparseable 
if they are caught."

I don't think we should grab Heap_lock at all and we should just remove it.
If there is a problem with "unparseable heap", taking Heap_lock seems to the 
wrong solution.

/Robbin

> 
> Dan
> 
> 
> 
>> _should_terminate = true;
>>      VMOperationQueue_lock->notify();
>>    }
>>
>> Passes t1-2.
>>
>> Thanks Robbin
>