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

[David Holmes]

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.

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.

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

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