[jdk17] RFR: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6" [v3]

[Daniel D.Daugherty]

On Mon, 26 Jul 2021 22:40:53 GMT, Patricio Chilano Mateo <pchilanomate at openjdk.org> wrote:

>> Hi,
>> 
>> Please review this small patch. When self-suspending, a JavaThread might reach SafepointSynchronize::block() with a state of _thread_in_vm which is not listed as a valid state for safepoint polling. There are a couple of simple ways to fix this. As suggested by @dholmes-ora  we can avoid the handshake machinery altogether and directly self-suspend. Since this issue is intermittent and in tier7 which I didn't run as many times as other lower tiers it escaped my testing of 8270085.
>> 
>> Testing in mach5 tiers1-7. I also reproduced the test failures locally and verified that now both hs202t002.java and ThreadSuspendSelf.java are passing. 
>> 
>> Thanks,
>> Patricio
>
> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
> 
>   move log statement back inside loop

Marked as reviewed by dcubed (Reviewer).

In `src/hotspot/share/runtime/safepointMechanism.cpp`, I would love to see:


void SafepointMechanism::process(JavaThread *thread, bool allow_suspend) {
  // Read global poll and has_handshake after local poll
  OrderAccess::loadload();

  // local poll already checked, if used.
  bool need_rechecking;
  do {
    assert(is_a_block_safe_state(thread->thread_state()), "thread state is not safe for a SafepointSynchronize::block()");
    if (global_poll()) {
      // Any load in ::block() must not pass the global poll load.
      // Otherwise we might load an old safepoint counter (for example).
      OrderAccess::loadload();
      SafepointSynchronize::block(thread);
    }


We don't have an `is_a_block_safe_state()` function so I would be okay with
a simple check like this:

  assert(thread->thread_state() != _thread_in_vm,  "thread state is not safe for a SafepointSynchronize::block()");


I looked at both `SafepointSynchronize::handshake_safe()` and `safepoint_safe_with()`
and neither of those is quite right.

I'm okay with the changes as they are, but I'd love to see an assert()
added in `SafepointMechanism::process()` as I describe above. I think
this would help catch any other unexpected go-to-a-safepoint from
the wrong state errors.

If you decide to add `is_a_block_safe_state()`, then add it above
SafepointSynchronize::block() and use it to determine what safepoint
safe states are accepted by SafepointSynchronize::block(), i.e., share
the new function.

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

PR: https://git.openjdk.java.net/jdk17/pull/283