RFR: 8273695: Safepoint deadlock on VMOperation_lock

[Daniel D.Daugherty]

On Tue, 21 Sep 2021 13:17:43 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

> We should not do any processing in SM::should_process().
> The query is used to determine if we need to process safepoint/handshakes and with this change StackWaterMark.
> When locking a Mutex which may be acquired in such processing, we must release that Mutex before we can start processing, otherwise we can deadlock.
> 
> This change adds a method to determine if StackWaterMarkSet::on_safepoint() will do any processing.
> In that case there are poll is armed, we do not allow suspend handshakes and there is no safepoint and no non-suspend handshakes, we still return true if StackWaterMarkSet needs processing.
> Thus the code querying should release any such Mutex and call process SM::process_if_requested().
> 
> The cross_modify_fence() do not have any such state, so we still need to emit that before returning false if poll is armed.
> 
> Passes t1-t4 and local stressing.

Thumbs up. Most of my comments are nits.

I have one question about a possible missing update_poll_values(thread) call
before cross_modify_fence().

src/hotspot/share/runtime/safepointMechanism.inline.hpp line 75:

> 73:   if (global_poll() || // Safepoint
> 74:       thread->handshake_state()->has_a_non_suspend_operation() || // Non-suspend handshake
> 75:       !StackWatermarkSet::processing_started(thread)) { // StackWater processing is not started

Perhaps: s/StackWater/StackWatermark/

src/hotspot/share/runtime/safepointMechanism.inline.hpp line 81:

> 79:   // 1: A suspend handshake targets the JavaThread.
> 80:   // 2: A safepoint takes place.
> 81:   // 3: The JavaThread wakes-up from blocked and do not allow suspend.

typo: s/do not/does not/

src/hotspot/share/runtime/safepointMechanism.inline.hpp line 83:

> 81:   // 3: The JavaThread wakes-up from blocked and do not allow suspend.
> 82:   // We are now about to avoid processing and thus no cross modify fence will be executed.
> 83:   // Therefore we executed it here.

typo: s/executed/execute/

src/hotspot/share/runtime/stackWatermarkSet.hpp line 82:

> 80:   static void start_processing(JavaThread* jt, StackWatermarkKind kind);
> 81: 
> 82:   // Returns true if all StackWaters have been started.

Perhaps: s/StackWaters/StackWatermarks/

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

Marked as reviewed by dcubed (Reviewer).

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