RFR: 8257831: Suspend with handshakes [v10]

[Robbin Ehn]

On Mon, 19 Apr 2021 04:19:28 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed nits
>
> src/hotspot/share/runtime/handshake.cpp line 630:
> 
>> 628: // This is the closure that prevents a suspended JavaThread from
>> 629: // escaping the suspend request.
>> 630: class ThreadSuspensionHandshake : public AsyncHandshakeClosure {
> 
> I still find ThreadSuspensionHandShake versus SuspendThreadHandshake to be too similarly named and with no obvious way to remember which one is which. Perhaps this one could be called ThreadSelfSuspensionHandshake instead?

Fixed

> src/hotspot/share/runtime/handshake.cpp line 636:
> 
>> 634:     JavaThread* target = thr->as_Java_thread();
>> 635:     target->handshake_state()->do_self_suspend();
>> 636:   }
> 
> As this must be the current thread we are dealing with can we rename the variables to indicate that.

Fixed

> src/hotspot/share/runtime/handshake.hpp line 128:
> 
>> 126:   // must take slow path, process_by_self(), if _lock is held.
>> 127:   bool should_process() {
>> 128:     // When doing thread suspension the holder of the _lock
> 
> Potentially this could be any async handshake operation - right? It seems odd to talk specifically about thread suspension in the core of the handshake code.

Changed

> src/hotspot/share/runtime/handshake.hpp line 131:
> 
>> 129:     // can add an asynchronous handshake to queue.
>> 130:     // To make sure it is seen by the handshakee, the handshakee must first
>> 131:     // check the _lock, if held go to slow path.
> 
> ..., _and_ if held ...

Fixed

> src/hotspot/share/runtime/handshake.hpp line 133:
> 
>> 131:     // check the _lock, if held go to slow path.
>> 132:     // Since the handshakee is unsafe if _lock gets locked after this check
>> 133:     // we know another thread cannot process any handshakes.
> 
> I can't quite understand this comment. I'm not sure what thread is calling this method and when, in relation to what the handshakee may be doing.

The handshakee is in a safe state, e.g. blocked and does:
Point A: set_thread_state_fence(_thread_blocked_trans);
...
Point B: _lock.is_locked()

While the processor thread does:
Point X: _lock.try_lock();
...
Point Z: thread->thread_state();

If point B is passed with a non-locked lock, point Z is guaranteed to see an unsafe thread and will not start to process ay handshakes.

Is that make sense, maybe the comment make more sense ? :)

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

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