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

[Daniel D. Daugherty]

On 8/27/20 9:38 PM, David Holmes wrote:
> Hi Dan,
>
> Sorry need to be a bit pedantic here ...

With suspend/resume you must take things down to the pedantic level
so no apology needed.


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

You'd think I would remember that especially since I'm pretty sure
that is_ext_suspended() is something that I wrote many, many years
ago... sigh...

Yup... found the delta from Teamware...

$ sp -r1.321 src/share/vm/runtime/thread.hpp
src/share/vm/runtime/SCCS/s.thread.hpp:

D 1.321 01/10/17 20:38:49 dcubed 732 731        00092/00022/01306
MRs:
COMMENTS:
4510838 - Add _suspend_equivalent support for _thread_blocked state.
Change _is_suspended to _suspend_done_type; this allows us to know
which type of suspend is done first. Add SR_RequestTypes to abstract
suspend and resume requests. Change various suspend() and resume()
parameters to SR_RequestTypes. Add new query functions for suspend
status. Update a lot of comments

They say memory is the second thing to go... what's the first? :-)

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

As I should have. Thanks for being pedantic!

Dan

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