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

[Reingruber, Richard]

Excellent!

Cheers, Richard.


> Am 28.08.2020 um 19:31 schrieb Daniel D. Daugherty <daniel.daugherty at oracle.com>:
> 
> 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
>