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

[Daniel D. Daugherty]

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.

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?

     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.

Dan


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