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

[Reingruber, Richard]

Hi Robbin,

> Thanks! Sorry, I already push and cannot credit you.
> (maybe I broke the 24h...)

No problem really.

> Had a quick glance, yes it looks like we are missing a check on
> _suspend_flag (e.g. call to ~handle_special_runtime_exit_condition,
> causing this handling to be recursive).

> Should be producible with a sleep before setting the state back.

Yes I think so.

> Create a issue in jira!

Will do on Monday if Dan hasn't.

Thanks, Richard.

-----Original Message-----
From: Robbin Ehn <robbin.ehn at oracle.com> 
Sent: Freitag, 28. August 2020 18:04
To: Reingruber, Richard <richard.reingruber at sap.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,

On 2020-08-28 15:33, Reingruber, Richard wrote:
> Hi Robbin,
> 
> your change looks correct to me. Thanks!
Thanks! Sorry, I already push and cannot credit you.
(maybe I broke the 24h...)

Had a quick glance, yes it looks like we are missing a check on
_suspend_flag (e.g. call to ~handle_special_runtime_exit_condition,
causing this handling to be recursive).

Should be producible with a sleep before setting the state back.

Create a issue in jira!

Thanks, Robbin

> 
> 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!JQs56n6YnkiVXt_nGgC8TVQQDa-YtrDVsgtxE4CMDkszZ4iQmghMsuIU0ojdRbpU$
> 
> -----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
>