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

[David Holmes]

Hi Robbin,

On 17/06/2019 4: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.

Okay.

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

Thinking about that one ... okay.

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

So the assumption here is that all uses of locking of Threads_lock 
without a safepoint check, by a JavaThread, only occur when the 
JavaThread is no longer on the _threads_list.

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

Unclear whether you are making things better, worse, or the same as they 
are currently ? But looking at the change I don't see the point. IIUC 
you are worried that using lock() rather than 
lock_without_safepoint_check() will allow the error-reporting thread to 
get trapped by a safepoint - right? But it is trying to acquire the 
Threads_lock so it can already get trapped by a safepoint.

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

Sounds complicated ...

> 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

Dan already picked up on a few typos and comment issues so I'll try not 
repeat those.

src/hotspot/share/runtime/mutex.cpp

   71     // Is it a JavaThread participating in the safeopint protocol.

Typo: safeopint

   72     if (self->is_active_Java_thread()) {

Given we check this in a loop, and given we may also check this at line 
#51, and I'm assuming this is not a property that can change, perhaps we 
should cache this up front into a local variable?

---

src/hotspot/share/runtime/thread.cpp

-     Threads_lock->lock_without_safepoint_check();
+     Threads_lock->lock();
       ShouldNotReachHere();
     }
   }

I share Dan's concern that going through the lock() path with all of its 
JavaThread related checks and actions, might not be appropriate at this 
stage of VM termination? Same with the change in 
VM_Exit::wait_if_vm_exited(). These paths are not well tested - I added 
assert(false) to them both and passed tiers 1-3.

+   virtual bool is_active_Java_thread() const {
+     return on_thread_list() && !is_terminated();
+   }

Not sure why the termination check is needed here. As we terminate after 
being removed from the threads_list we won't get past the 
on_thread_list() check for a terminated thread. ??

> Passes t1-3.

As noted above this is necessary but not sufficient testing for the code 
paths modified here. Some stress testing involving threads that are 
in-native when the VM tries to exit is needed - but I have no idea 
whether such tests already exist.

Thanks,
David

> Thanks, Robbin