RFR: 8254263: Remove special_runtime_exit_condition() check from ~ThreadInVMForHandshake()

[Richard Reingruber]

On Mon, 12 Oct 2020 18:35:51 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>>> I meant the block could be removed if we put
>>> ThreadInVMfromJava/ThreadInVMfromJava (TIVFJ) into
>>> handle_polling_page_exception(). In that case the thread would notice and honor
>>> the suspend when leaving the TIVFJ block. This would not be too late, would it?
>> 
>> Yes, it will honor the first suspend in the TIVFJ destructor by calling handle_special_runtime_exit_condition(), but
>> even in that case, we would still need the check in SS:block() because of the last call to
>> SafepointMechanism::process_if_requested(thread()) in java_suspend_self_with_safepoint_check(). That could be a new
>> VM_ThreadSuspend() operation to suspend the thread again. So that check in SS:block() is actually needed for all the
>> transitions to java not only the one in handle_polling_page_exception(). But I think we can remove that recursion if we
>> move that call to SafepointMechanism::process_if_requested(thread()) inside the do-while().
>
>> > Now, I'm thinking the call to SafepointMechanism::process_if_requested(this)
>> > in java_suspend_self_with_safepoint_check() should be moved inside the the
>> > do-while(is_external_suspend()). That way we guarantee after returning from
>> > java_suspend_self_with_safepoint_check() that no one will be assuming the
>> > thread is suspended. And in that case I think that check in SS::block can be
>> > removed.
>> 
>> Yes I agree.
> Great. I'll test it out.
> 
>> Well, I don't want to spam your PR with too many questions / comments. The
>> change still looks good to me.
> No problem, it's actually good to have more eyes looking to make sure I don't miss anything.  : )
> Thanks Richard!

> > I meant the block could be removed if we put
> > ThreadInVMfromJava/ThreadInVMfromJava (TIVFJ) into
> > handle_polling_page_exception(). In that case the thread would notice and honor
> > the suspend when leaving the TIVFJ block. This would not be too late, would it?
> 

> Yes, it will honor the first suspend in the TIVFJ destructor by calling
> handle_special_runtime_exit_condition(), but even in that case, we would still
> need the check in SS:block() because of the last call to
> SafepointMechanism::process_if_requested(thread()) in
> java_suspend_self_with_safepoint_check(). That could be a new
> VM_ThreadSuspend() operation to suspend the thread again.

Sorry, I have to ask again to really understand this myself. If it is a new
VM_ThreadSuspend() that started while the target thread T was still
_thread_blocked in java_suspend_self_with_safepoint_check(), then T can not
leave the do-while-loop because is_external_suspend() will return true as
setting the suspend flag for T cannot be reordered with the begin of the
safepoint.

Also if T could leave the loop then the safepoint could be ended before T
reaches SS::block(). It would escape then to java even though it was suspended.

See also review thread for JDK-8252521:
http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-September/041632.html

-------------

PR: https://git.openjdk.java.net/jdk/pull/577