RFR: 8221207: Redo JDK-8218446 - SuspendAtExit hangs

[Daniel D. Daugherty]

On 3/21/19 5:52 PM, David Holmes wrote:
> Hi Dan,
>
> Thanks again for the eagle-eyed analysis ...
>
> On 22/03/2019 5:57 am, Daniel D. Daugherty wrote:
>> On 3/21/19 4:47 AM, David Holmes wrote:
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221207
>>>
>>> There was one small oversight in the original fix that led to 
>>> crashes, seen (randomly) in JDI tests. The safepoint check must not 
>>> happen if the thread-state is already _thread_in_native. I've 
>>> checked the thread-state on all call paths to confirm that.
>>>
>>> Incremental webrev from original fix: 
>>> http://cr.openjdk.java.net/~dholmes/8221207/webrev.inc/
>>>
>>> Full webrev: http://cr.openjdk.java.net/~dholmes/8221207/webrev/
>>
>> src/hotspot/share/runtime/thread.hpp
>>      No comments.
>>
>> src/hotspot/share/runtime/thread.cpp
>>      So here's the crashing stack:
>>
>>      V [libjvm.so+0x14b1fee] 
>> SafepointSynchronize::block(JavaThread*)+0xae
>>      V [libjvm.so+0x14ba78d] 
>> SafepointMechanism::block_if_requested_slow(JavaThread*)+0x6d
>>      V [libjvm.so+0x1637207] 
>> JavaThread::handle_special_runtime_exit_condition(bool)+0x67
>>      V [libjvm.so+0x1073b1e] 
>> JvmtiJavaThreadEventTransition::JvmtiJavaThreadEventTransition(JavaThread*)+0x1ae 
>>
>>      V [libjvm.so+0x1069990] 
>> JvmtiExport::post_class_prepare(JavaThread*, Klass*)+0x1b0
>>
>>      so we have a JvmtiJavaThreadEventTransition helper object to
>>      handle the transition from thread_in_vm -> thread_in_native:
>>
>>      src/hotspot/share/prims/jvmtiExport.cpp
>>
>>          class JvmtiJavaThreadEventTransition : StackObj {
>>          private:
>>            ResourceMark _rm;
>>            ThreadToNativeFromVM _transition;
>>            HandleMark _hm;
>>
>>          public:
>>            JvmtiJavaThreadEventTransition(JavaThread *thread) :
>>              _rm(),
>>              _transition(thread),
>>              _hm(thread)  {};
>>          };
>>
>>      so that's just a wrapper around ThreadToNativeFromVM:
>>
>>      src/hotspot/share/runtime/interfaceSupport.inline.hpp
>>
>>          class ThreadToNativeFromVM : public ThreadStateTransition {
>>           public:
>>            ThreadToNativeFromVM(JavaThread *thread) : 
>> ThreadStateTransition(thread) {
>>              // We are leaving the VM at this point and going 
>> directly to native code.
>>              // Block, if we are in the middle of a safepoint 
>> synchronization.
>>              assert(!thread->owns_locks(), "must release all locks 
>> when leaving VM");
>> thread->frame_anchor()->make_walkable(thread);
>>              trans_and_fence(_thread_in_vm, _thread_in_native);
>>              // Check for pending. async. exceptions or suspends.
>>              if (_thread->has_special_runtime_exit_condition()) 
>> _thread->handle_special_runtime_exit_condition(false);
>>            }
>>
>>            ~ThreadToNativeFromVM() {
>>              trans_from_native(_thread_in_vm);
>> assert(!_thread->is_pending_jni_exception_check(), "Pending JNI 
>> Exception Check");
>>              // We don't need to clear_walkable because it will 
>> happen automagically when we return to java
>>            }
>>          };
>>
>>      so trans_and_fence() calls transition_and_fence() which
>>      does this:
>>
>>          static inline void transition_and_fence(JavaThread *thread, 
>> JavaThreadState from, JavaThreadState to) {
>>            assert(thread->thread_state() == from, "coming from wrong 
>> thread state");
>>            assert((from & 1) == 0 && (to & 1) == 0, "odd numbers are 
>> transitions states");
>>            // Change to transition state
>>            thread->set_thread_state((JavaThreadState)(from + 1));
>>
>> InterfaceSupport::serialize_thread_state_with_handler(thread);
>>
>>            SafepointMechanism::block_if_requested(thread);
>>            thread->set_thread_state(to);
>>
>> CHECK_UNHANDLED_OOPS_ONLY(thread->clear_unhandled_oops();)
>>          }
>>
>>      So for this use of handle_special_runtime_exit_condition(false),
>>      a safepoint is already handled by the previous 
>> transition_and_fence()
>>      with the thread still in the right thread state. However, if that
>>      handle_special_runtime_exit_condition() honors a self-suspend
>>      request and there's another safepoint, then we run the risk of
>>      the VMThread seeing _thread_blocked during the self-suspend
>>      phase of the thread and then the thread will go ahead into
>>      thread_native without stopping for the safepoint.
>>
>>      Okay, but do we care? I don't think so. The thread will be off
>>      in native code and if it returns quickly and the safepoint is
>>      still active, then ~ThreadToNativeFromVM() should cause the
>>      thread to block for the safepoint.
>
> The way I looked at this is that we are trying to ensure that the 
> VMThread doesn't see _thread_blocked just before we restore the true 
> state, and allow the thread to escape the safepoint. In this case the 
> true state is _thread_in_native, which is safepoint-safe just as 
> _thread_blocked is, so it doesn't matter if the VMThread sees the true 
> state or _thread_blocked as the result is the same. Hence for 
> _thread_in_native we don't need to do the additional safepoint check.
>
>>      So this is a long winded way of saying I think the revised
>>      fix is okay. :-)
>
> Okay - thanks :)
>
>>      You added this comment for the new if-statement:
>>
>>      +// But it's more complicated than that as not all initial 
>> thread-states are suitable for
>>      +// doing safepoint checks. Fortunately, _thread_in_native is 
>> the only unsuitable state we
>>      +// can encounter based on our two callers.
>>
>>      and I'm okay with it.
>>
>>      Please consider adding this comment:
>>
>>      +  if (state != _thread_in_native) {
>>           // _thread_in_native will block for a safepoint when it 
>> transitions back.
>>           SafepointMechanism::block_if_requested(this);
>>      +  }
>
> It's a correct statement but I'm not sure its really relevant to the 
> context. But I didn't explain in the code why _thread_in_native is 
> special. My "But it's more complicated ..." comment doesn't explain 
> things properly. How about:
>
> // However, not all initial-states are allowed when performing a
> // safepoint check, as we should never be blocking at a safepoint
> // whilst in those states. Of these 'bad' states only _thread_in_native
> // is possible when executing this code (based on our two callers).
> // A thread that is _thread_in_native is already safepoint-safe and so
> // it doesn't matter whether the VMThread sees the _thread_blocked
> // state, or the _thread_in_native state, and so we don't need the
> // explicit safepoint check.

I like it!

Dan


>
> ?
>
> Thanks,
> David
> -----
>
>> Thumbs up!
>>
>> Dan
>>
>>
>>>
>>> Re-tested in mach5 tiers 1-3 and com/sun/jdi tests (but they passed 
>>> last time too.).
>>>
>>> Thanks,
>>> David
>>>
>>