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

[Robbin Ehn]

Hi Coleen,

On 2019-08-07 23:46, coleen.phillimore at oracle.com wrote:
> 
> Robbin,  This looks good.  Thank you for fixing Threads_lock to not be a 
> sometimes lock.  That helps us.

Thanks.

> 
> http://cr.openjdk.java.net/~rehn/8226228/v2/webrev/src/hotspot/share/utilities/vmError.cpp.udiff.html
> 
> I also had a comment about the try_lock.  But it doesn't seem like it would 
> change behaviour.  If you get the lock, it is never unlocked though.  Could it 
> deadlock error handling through another path?  It's probably okay.

It probably can, but I do not think this make any different. Before we could 
deadlock on Threads_lock, if try_lock fail we may deadlock at a new place
instead.

> 
> - MutexLocker ml(Threads_lock->owned_by_self() ? NULL : Threads_lock, 
> Mutex::_no_safepoint_check_flag);
> + if (!Threads_lock->owned_by_self()) {
> + Threads_lock->try_lock();
> + }

The last line of the method is:
1850   ShouldNotReachHere();
Destructor of MutexLocker can never run.

> 
> http://cr.openjdk.java.net/~rehn/8226228/v2/webrev/src/hotspot/share/runtime/mutex.cpp.udiff.html
> 
> In one of my patches, I moved this code so I can move it again. Maybe the 
> NoSafepointVerifier checks should check is_active_java_thread() now.
> 
>   #ifdef CHECK_UNHANDLED_OOPS
>     // Clear unhandled oops in JavaThreads so we get a crash right away.
> - if (self->is_Java_thread()) {
> + if (self->is_active_Java_thread()) {
>       self->clear_unhandled_oops();
>     }
>   #endif // CHECK_UNHANDLED_OOPS

Yes.

> 
> Ship it!

Thanks!

/Robbin

> Thanks,
> Coleen
> 
> 
> On 8/6/19 2:29 AM, 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/
>>
>> Passes t1-5.
>>
>> 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
>