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

[Robbin Ehn]

Hi Dan,

On 2019-06-18 21:14, Daniel D. Daugherty wrote:
> Hi Robbin,
> 
> I think this is your 5th or 6th webrev in two (business days). Talk
> about clearing your pending fix queue!!
> 
> 
> On 6/17/19 7: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.
>>
>> 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
> 
> src/hotspot/share/runtime/thread.hpp
>      L497:   // Is this a JavaThread and on ThreadsList (visible).
>      L498:   // Being on ThreadsList it must participate in the safepoint protocol.
>          Perhaps:
>              // Is this a JavaThread that is on the VM's current ThreadsList?
>              // If so it must participate in the safepoint protocol.
> 
>      L1258:   virtual bool is_active_Java_thread() const {
>      L1259:     return on_thread_list() && !is_terminated();
>      L1260:   }
>          on_thread_list() is a good choice for the opening window since
>          _on_thread_list is set to true while the Threads_lock is held
>          and the JavaThread is being added to the current ThreadsList.
> 
>          And !is_terminated() is a good choice for the closing window
>          since the '_terminated' value is set while the Threads_lock is
>          held and after the JavaThread is removed from the current
>          ThreadsList.
> 
>          The only edge case I found is in VM_Exit::set_vm_exited() where
>          we set '_vm_exited' for JavaThreads that are still running native
>          code. In that edge case, is_terminated() will still return true
>          for those JavaThreads and I think that's okay.
> 

Good to remind me!

> src/hotspot/share/runtime/thread.cpp
>      old L1808: Threads_lock->lock_without_safepoint_check();
>      new L1808:     Threads_lock->lock();
>          This may change (slightly) how long it takes for those straggler
>          JavaThreads to block on Threads_lock when block_if_vm_exited()
>          is called. I don't think it'll be a big deal, but I wanted to
>          mention it.
> 
> src/hotspot/share/runtime/mutex.cpp
>      old L51:   if (self->is_Java_thread()) {
>      new L51:   if (self->is_active_Java_thread()) {
>          L52:     self->clear_unhandled_oops();
>          Not sure why you changed this one? Is your thinking that the
>          terminated JavaThread shouldn't be running any more Java code
>          so there shouldn't be an unhandled oops left to clear?

If there is a safepoint and this thread is not participating those oops are not valid,
since GC might have moved the objects. So yes.

> 
>      L190:   // Safepoint checking logically implies active java_thread.
>          nit grammar: s/active/an active/
> 
>          I would also prefer 'JavaThread' to 'java_thread' in that comment.
> 
> src/hotspot/share/runtime/mutexLocker.cpp
>      No comments (gonna have to reprogram my brain that the Threads_lock
>      will be safepoint_check_always).
> 
> 
> src/hotspot/share/jfr/periodic/sampling/jfrThreadSampler.cpp
>      No comments.
> 
> src/hotspot/share/prims/jni.cpp
>      L4139:   // This thread will not be safepoint check, since it has
>          nit - grammar: s/not be/make a/
> 
>          Please consider using "MutexLocker ml(Threads_lock)" instead of
>          the direct "Threads_lock->lock()" and "Threads_lock->unlock()"
>          calls...
> 
> src/hotspot/share/runtime/handshake.cpp
>      No comments.
> 
> src/hotspot/share/runtime/threadSMR.cpp
>      L945:       // No safepoint check because this JavaThread is not on the
>      L946:       // Threads list.
>          Please consider updating the comment to:
> 
>                  // Will not make a safepoint check because this JavaThread
>                  // is not on the current ThreadsList.
> 
> src/hotspot/share/runtime/vmOperations.cpp
>      old L513:     Threads_lock->lock_without_safepoint_check();
>      new L513     Threads_lock->lock();
>          This may change (slightly) how long it takes for the caller
>          to block on Threads_lock when wait_if_vm_exited() is called.
>          I don't think it'll be a big deal, but I wanted to mention it.
> 
> src/hotspot/share/utilities/vmError.cpp
>      old L1793:   MutexLocker ml(Threads_lock->owned_by_self() ? NULL
>                                  : Threads_lock, Mutex::_no_safepoint_check_flag);
>      new L1793:   if (!Threads_lock->owned_by_self()) {
>      new L1794:     Threads_lock->try_lock();
>      new L1795:   }
>          The switch from "Threads_lock->lock(Mutex::_no_safepoint_check_flag)"
>          to "Threads_lock->try_lock()" could get "interesting". Instead of
>          blocking if the Threads_lock is held elsewhere during the crash,
>          this caller will now "motor on" if it can't lock the Threads_lock.
>          We're gonna have to keep an eye on the ErrorHandler tests... :-)
> 
> 
> Thumbs up! I think I have only comment changes above so it's your
> call on whether to make them or not.

Thanks, fix all above!

/Robbin

> 
> Dan
> 
> 
>>
>> Passes t1-3.
>>
>> Thanks, Robbin
>