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

[Robbin Ehn]

Hi David,

On 2019-06-23 03:34, 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?
> 

Fixed above.

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

Dan explained this.

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

Testing more.

Thanks, Robbin

> 
> Thanks,
> David
> 
>> Thanks, Robbin