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

[Patricio Chilano Mateo]

On Mon, 26 Jul 2021 21:13:43 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Patricio Chilano Mateo has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   add assert in do_self_suspend
>
> src/hotspot/share/runtime/handshake.cpp line 615:
> 
>> 613:   assert(_lock.owned_by_self(), "Lock must be held");
>> 614:   assert(!_handshakee->has_last_Java_frame() || _handshakee->frame_anchor()->walkable(), "should have walkable stack");
>> 615: 
> 
> Can we add an assert:
> 
> `assert(_handshakee->thread_state() == _thread_blocked, "Caller should have transitioned to _thread_blocked);
> `
> ?

Done!

> src/hotspot/share/runtime/handshake.cpp line 616:
> 
>> 614:   assert(!_handshakee->has_last_Java_frame() || _handshakee->frame_anchor()->walkable(), "should have walkable stack");
>> 615: 
>> 616:   log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " suspended", p2i(_handshakee));
> 
> I think I prefer this logging to remain in the while loop. By placing it there you know the thread will actually wait() and you can also see if there are multiple suspend requests. It might even make sense to put the resume log statement in the loop after the wait() with an explicit check of is_suspended() to preclude a spurious wakeup.

So for the resume log statement, once the target wakes up from wait() the _suspended flag could be set not necessarily because of a spurious wakeup but because another JT suspended the target right after the resume. But I can add a log statement in HandshakeState::resume() to track all resumes. 
As for the suspend log statement I thought about not moving it but then I saw that we are already logging suspends in HandshakeState::suspend_with_handshake(), and since all these logging statements are done under _lock held there is already a sequence of suspend/resume events (if adding log in HandshakeState::resume()). 
What do you think?

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

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