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

[Robbin Ehn]

Hi David,

On 2019-08-08 08:51, David Holmes wrote:
> Hi Robbin,
> 
> On 6/08/2019 4:29 pm, Robbin Ehn wrote:
>> Hi guys,
>>
>> Let's pick this up :) Been a while...
>>
>> I think that we were pretty happy after some explaining !?
>>
>> Here is full, let's call it v2:
>> http://cr.openjdk.java.net/~rehn/8226228/v2/webrev/
> 
> Looks fine.

Thanks!

> 
> Just to follow up on one thing I don't see being explained previously. I (and 
> Dan I think) had some concerns about this code:
> 
> 1803 void JavaThread::block_if_vm_exited() {
> 1804   if (_terminated == _vm_exited) {
> 1805     // _vm_exited is set at safepoint, and Threads_lock is never released
> 1806     // we will block here forever
> 1807     Threads_lock->lock();
> 1808     ShouldNotReachHere();
> 1809   }
> 1810 }
> 
> because the lock() code is potentially a lot more complex than the 
> lock_without_safepoint_check(). But AFAICS we only call the above when we 
> already have "thread->is_terminated()" in which case it is not an "active Java 
> thread" and so we take a degenerate path through the lock() code anyway.

Yes, you are correct. (same conclusion as you but forgot to mention it)

> 
>> Passes t1-5.
> 
> As previously noted that may not exercise all of the code paths touched by this, 
> but it should suffice for pushing the change and thus getting further testing in 
> the higher tiers.

Yes, thanks!

/Robbin

> 
> Thanks,
> David
> 
>> Thanks, Robbin
>>
>> On 2019-06-17 13:21, 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.
>>>
>>> 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.
>>>
>>> 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.
>>>
>>> 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.
>>> 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.
>>>
>>> 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
>>>
>>> Passes t1-3.
>>>
>>> Thanks, Robbin