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

[Daniel D. Daugherty]

Done:

     JDK-8252521 possible race in java_suspend_self_with_safepoint_check
     https://bugs.openjdk.java.net/browse/JDK-8252521

Dan


On 8/28/20 1:18 PM, Reingruber, Richard wrote:
> Sounds good. Thanks for doing it.
>
> Richard.
>
> -----Original Message-----
> From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
> Sent: Freitag, 28. August 2020 17:58
> To: Reingruber, Richard <richard.reingruber at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: Re: RFR(s): 8252414: Redundant suspend check when determining if a java thread is safe
>
> Would you mind if I filed a bug on your behalf? Something like:
>
>       possible race in java_suspend_self_with_safepoint_check()
>
> Dan
>
> On 8/28/20 11:53 AM, Reingruber, Richard wrote:
>> Hi Dan,
>>
>> thanks for commenting on this.
>>
>> I'll start a new thread (probably after the weekend).
>>
>> Thanks, Richard.
>>
>> -----Original Message-----
>> From: Daniel D. Daugherty <daniel.daugherty at oracle.com>
>> Sent: Freitag, 28. August 2020 17:39
>> To: Reingruber, Richard <richard.reingruber at sap.com>; Robbin Ehn <robbin.ehn at oracle.com>; hotspot-runtime-dev at openjdk.java.net
>> Subject: Re: RFR(s): 8252414: Redundant suspend check when determining if a java thread is safe
>>
>> Hi Richard,
>>
>> This is the right place and the right people have been watching this
>> thread, but we should take this up in a new thread and likely with a
>> new bug ID...
>>
>> Dan
>>
>>
>> On 8/28/20 9:33 AM, Reingruber, Richard wrote:
>>> Hi Robbin,
>>>
>>> your change looks correct to me. Thanks!
>>>
>>> I see a related issue that allows a suspended thread to continue executing
>>> java. Hope it's ok to ask about it here.
>>>
>>> Assume a thread T is resumed and then taken off CPU in JavaThread::java_suspend_self_with_safepoint_check()
>>> just before restoring its prior state [1].
>>>
>>> In this situation T gets suspended again (by executing VM_ThreadSuspend)
>>>
>>> Now T is put back on the CPU. There are code paths where T won't check its
>>> suspend flags and return to executing java even though it is suspended.
>>>
>>>
>>> Example: JavaCallWrapper
>>>
>>>      Stack:
>>>
>>>      JavaThread::java_suspend_self_with_safepoint_check() : void
>>>      JavaThread::handle_special_runtime_exit_condition(bool) : void
>>>      JavaCallWrapper::JavaCallWrapper(const methodHandle &, Handle, JavaValue *, Thread *)
>>>
>>> Example: Polling page exception
>>>
>>>      Stack:
>>>
>>>      JavaThread::handle_special_runtime_exit_condition(bool) : void
>>>      SafepointSynchronize::block(JavaThread *) : void
>>>      SafepointMechanism::block_or_handshake(JavaThread *) : void
>>>      SafepointMechanism::block_if_requested_slow(JavaThread *) : void
>>>      SafepointMechanism::block_if_requested(JavaThread *) : void
>>>      ThreadSafepointState::handle_polling_page_exception() : void
>>>
>>> Would you agree or have I missed something?
>>>
>>> Note that there is no problem when T is put back on CPU while a handshake is
>>> executed on its behalf because after restoring its prior state T calls
>>> SafepointMechanism::block_if_requested() in
>>> JavaThread::java_suspend_self_with_safepoint_check().
>>>
>>> Thanks, Richard.
>>>
>>> [1] T is taken off CPU before restoring its prior state.
>>>        https://urldefense.com/v3/__https://github.com/openjdk/jdk/blob/30c2dbea95f817c93f84f0d8f7dc3b5c159b5e25/src/hotspot/share/runtime/thread.cpp*L2619__;Iw!!GqivPVa7Brio!N14B23JPPfP4dkCJeZPO3k0RlIRBIuKdiBY7lZZAHZeGHPnz00kp_R7ZxQO-4EwWeAdF$
>>>
>>> -----Original Message-----
>>> From: hotspot-runtime-dev <hotspot-runtime-dev-retn at openjdk.java.net> On Behalf Of Robbin Ehn
>>> Sent: Donnerstag, 27. August 2020 09:21
>>> To: hotspot-runtime-dev at openjdk.java.net
>>> Subject: RFR(s): 8252414: Redundant suspend check when determining if a java thread is safe
>>>
>>> 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
>>>
>>> Passes t1-5
>>>
>>> Thanks, Robbin