RFR(s): 8252414: Redundant suspend check when determining if a java thread is safe

[David Holmes]

Hi Dan,

Sorry need to be a bit pedantic here ...

On 28/08/2020 9:14 am, Daniel D. Daugherty wrote:
> On 8/27/20 3:21 AM, Robbin Ehn wrote:
>> Hi all, please review.
>>
>> In 8221207 - "Redo JDK-8218446 - SuspendAtExit hangs" we fixed so a
>> thread is always blocked when suspended.
>>
>> And added this nice assert.
>> int JavaThread::java_suspend_self() {
>>   assert(thread_state() == _thread_blocked, "wrong state for 
>> java_suspend_self()");
>>
>> When checking if a thread is safepoint/handshake safe there no need to 
>> look at ext suspend flag anymore, since the thread is blocked.
>>
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8252414
>> Code:
>> http://cr.openjdk.java.net/~rehn/8252414/webrev/index.html
> 
> Sorry for the delay. I had to go back and reread the
> "8221207: Redo JDK-8218446 - SuspendAtExit hangs" review thread.
> 
> 
> src/hotspot/share/runtime/handshake.cpp
>      No comments.
> 
> src/hotspot/share/runtime/safepoint.cpp
>      No comments.
> 
>> When checking if a thread is safepoint/handshake safe there no need to
>> look at ext suspend flag anymore, since the thread is blocked. 
> 
> I wouldn't quite phrase it that way. We consider a thread to
> be is_ext_suspended() when it's running native code also. In
> that case, it is _thread_in_native, but if it should happen
> to return from native then we'll catch it in
> JavaThread::java_suspend_self_with_safepoint_check() so it
> will become properly blocked.

If a suspended thread is in native then we consider it 
is_external_suspend() but not is_ext_suspended(). The latter only occurs 
when the thread is back in the VM and self-suspends via 
JavaThread::java_suspend_self() (at which point it is already 
_thread_blocked). So Robbin's phrasing was quite accurate.

Full disclosure: yes I had the same initial thought "or else it is in 
native", but then went and checked the details. :)

Cheers,
David

> Thumbs up!
> 
> Dan
> 
> 
>>
>> Passes t1-5
>>
>> Thanks, Robbin
>