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

[Daniel D. Daugherty]

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://github.com/openjdk/jdk/blob/30c2dbea95f817c93f84f0d8f7dc3b5c159b5e25/src/hotspot/share/runtime/thread.cpp#L2619
>
> -----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