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

[Daniel D. Daugherty]

Chiming in on one comment:

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

on_thread_list is set when the JavaThread is added to the ThreadsList
and is never cleared. is_terminated() pre-dates the addition of the
on_thread_list flag which I think we added in the Thread-SMR project.

When we added the on_thread_list flag, we thought about changing it
to false when the JavaThread was removed, but we already had the
whole is_terminated() machinery that has been debugged for race
conditions, etc...

Dan


On 6/22/19 9:34 PM, David Holmes wrote:
> 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