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

[Patricio Chilano Mateo]

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

>>> I thought about removing that check, but right now it's also preventing an
>>> escape from a suspend. The issue is that when going back to java, we always
>>> check if we were suspended and call java_suspend_self_with_safepoint_check()
>>> if true. After the do-while(is_external_suspend()) loop we call again
>>> SafepointMechanism::process_if_requested(this) to see if we need to stop for a
>>> safepoint/handshake. If there is an actual safepoint pending because of a
>>> VM_ThreadSuspend() then unless we keep that check the thread will not notice
>>> it was suspended after the safepoint and will go back to java.
>> 
>> 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?
>> 
>>> 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.
>> 
>> Well, I don't want to spam your PR with too many questions / comments. The
>> change still looks good to me.
>
>> 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!

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

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