On Tue, 6 Apr 2021 18:37:38 GMT, Patricio Chilano Mateo
<pchilanom...@openjdk.org> wrote:
>> Robbin Ehn has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains three commits:
>>
>> - Merge branch 'master' into SuspendInHandshake
>> - Merge branch 'master' into SuspendInHandshake
>> - 8257831: Suspend with handshake (review baseline)
>
> src/hotspot/share/runtime/handshake.hpp line 122:
>
>> 120: // must take slow path, process_by_self(), if _lock is held.
>> 121: bool should_process() {
>> 122: return !_queue.is_empty() || _lock.is_locked();
>
> While trying to think of the objectMonitor case I realize the order of this
> should probably be reversed with a load fence in between. Otherwise we could
> have the following scenario:
>
> Thread A blocks in ~ThreadInVMfromJava() -> wait_for_object_deoptimization()
> with _thread_blocked state.
> Thread Z executes SuspendThreadHandshake on A. Gets context switched before
> adding the async handshake.
> Thread A unblocks and transitions back to _thread_in_java. Calls
> SafepointMechanism::process_if_requested()-> process_if_requested_slow()
> since poll is armed. Checks that the handshake queue is empty. Gets context
> switched.
> Thread Z installs async handshake in queue, releases _lock and continues.
> Thread A checks _lock is unlocked so it doesn't call
> HandshakeState::process_by_self() to suspend and continues back to Java.
> (Poll will still be armed though because we will see it in
> update_poll_values()).
You are correct.
> src/hotspot/share/runtime/thread.hpp line 1142:
>
>> 1140: bool java_resume(); // higher-level resume logic called by the
>> public APIs
>> 1141: bool is_suspended() { return
>> _handshake.is_suspended(); }
>> 1142: bool suspend_request_pending() { return
>> _handshake.suspend_request_pending(); }
>
> If you use is_suspended() to check for suspension in objectMonitor::enter()
> then we can remove this method.
Fixed
-------------
PR: https://git.openjdk.java.net/jdk/pull/3191
