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

[coleen.phillimore at oracle.com]

Robbin,  This looks good.  Thank you for fixing Threads_lock to not be a 
sometimes lock.  That helps us.

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.

- 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();
+ }

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

Ship it!
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