RFR: 8226228: Make Threads_lock an always safepoint checked lock.

[David Holmes]

Hi Dan,

On 23/06/2019 5:47 am, Daniel D. Daugherty wrote:
> Chiming in on one comment:
> 
>> +   virtual bool is_active_Java_thread() const {
>> +     return on_thread_list() && !is_terminated();
>> +   }
>>
>> Not sure why the termination check is needed here. As we terminate 
>> after being removed from the threads_list we won't get past the 
>> on_thread_list() check for a terminated thread. ?? 
> 
> on_thread_list is set when the JavaThread is added to the ThreadsList
> and is never cleared. is_terminated() pre-dates the addition of the
> on_thread_list flag which I think we added in the Thread-SMR project.
> 
> When we added the on_thread_list flag, we thought about changing it
> to false when the JavaThread was removed, but we already had the
> whole is_terminated() machinery that has been debugged for race
> conditions, etc...

Oh - thanks - so on_thread_list() is simply a micro-step state beyond 
_thread_new (possibly could have been encoded that way too ?). Really 
needs to have another name as it sounds more dynamic than it is.

Okay that part seems okay then.

Thanks,
David
-----
> Dan
> 
> 
> On 6/22/19 9:34 PM, David Holmes wrote:
>> Hi Robbin,
>>
>> On 17/06/2019 4:21 am, Robbin Ehn wrote:
>>> Hi all, please review.
>>>
>>> A safepoint checked lock always check for safepoint if the locker is an
>>> JavaThread. If it's not a JavaThread, the thread are not 
>>> participating in the safepoint protocol thus safepoints are not checked.
>>
>> Okay.
>>
>>> A JavaThread that is not on _the_ ThreadsList is not participating in 
>>> the safepoint protocol and for most purposes should be treat as 
>>> NonJavaThread. This applies to whether we should check for a safepoint.
>>
>> Thinking about that one ... okay.
>>
>>> This changeset suggest adding a method for checking this.
>>> We can use that method in the mutex code to determine if we should 
>>> treat locker as JavaThread or a NonJavaThread.
>>
>> So the assumption here is that all uses of locking of Threads_lock 
>> without a safepoint check, by a JavaThread, only occur when the 
>> JavaThread is no longer on the _threads_list.
>>
>>> The only problematic scenario is when we are an active JavaThread in 
>>> vmError.cpp but don't want to safepoint, but want the ThreadsList to 
>>> be stable.
>>> I choose a best effort approach.
>>
>> Unclear whether you are making things better, worse, or the same as 
>> they are currently ? But looking at the change I don't see the point. 
>> IIUC you are worried that using lock() rather than 
>> lock_without_safepoint_check() will allow the error-reporting thread 
>> to get trapped by a safepoint - right? But it is trying to acquire the 
>> Threads_lock so it can already get trapped by a safepoint.
>>
>>> Alternatives could be stop freeing the backing array if there is an
>>> an error or letting the thread running the error code become an
>>> inactive JavaThread thus avoiding safepointing.
>>
>> Sounds complicated ...
>>
>>> This fix also helps converting Heap_lock to an always lock (Heap_lock 
>>> needed changes not included).
>>>
>>> Issue:
>>> https://bugs.openjdk.java.net/browse/JDK-8226228
>>> Code:
>>> http://cr.openjdk.java.net/~rehn/8226228/webrev/index.html
>>
>> Dan already picked up on a few typos and comment issues so I'll try 
>> not repeat those.
>>
>> src/hotspot/share/runtime/mutex.cpp
>>
>>   71     // Is it a JavaThread participating in the safeopint protocol.
>>
>> Typo: safeopint
>>
>>   72     if (self->is_active_Java_thread()) {
>>
>> Given we check this in a loop, and given we may also check this at 
>> line #51, and I'm assuming this is not a property that can change, 
>> perhaps we should cache this up front into a local variable?
>>
>> ---
>>
>> src/hotspot/share/runtime/thread.cpp
>>
>> -     Threads_lock->lock_without_safepoint_check();
>> +     Threads_lock->lock();
>>       ShouldNotReachHere();
>>     }
>>   }
>>
>> I share Dan's concern that going through the lock() path with all of 
>> its JavaThread related checks and actions, might not be appropriate at 
>> this stage of VM termination? Same with the change in 
>> VM_Exit::wait_if_vm_exited(). These paths are not well tested - I 
>> added assert(false) to them both and passed tiers 1-3.
>>
>> +   virtual bool is_active_Java_thread() const {
>> +     return on_thread_list() && !is_terminated();
>> +   }
>>
>> Not sure why the termination check is needed here. As we terminate 
>> after being removed from the threads_list we won't get past the 
>> on_thread_list() check for a terminated thread. ??
>>
>>> Passes t1-3.
>>
>> As noted above this is necessary but not sufficient testing for the 
>> code paths modified here. Some stress testing involving threads that 
>> are in-native when the VM tries to exit is needed - but I have no idea 
>> whether such tests already exist.
>>
>> Thanks,
>> David
>>
>>> Thanks, Robbin
>