[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR (XS) fix for a safepoint deadlock (8047720)
From:       "Daniel D. Daugherty" <daniel.daugherty () oracle ! com>
Date:       2014-06-30 17:08:56
Message-ID: 53B199A8.7040003 () oracle ! com
[Download RAW message or body]

David,

Thanks for the review. Obviously I can't list you as a reviewer
since I already pushed the changeset...


On 6/29/14 11:54 PM, David Holmes wrote:
> Correction ...
>
> On 30/06/2014 3:33 PM, David Holmes wrote:
>> Hi Dan,
>>
>> I see this has already gone in but I think it is worth looking closer at
>> this.
>>
>> On 28/06/2014 2:18 AM, Daniel D. Daugherty wrote:
>>> Greetings,
>>>
>>> I have a fix ready for the following bug:
>>>
>>>      8047720 Xprof hangs on Solaris
>>>      https://bugs.openjdk.java.net/browse/JDK-8047720
>>>
>>> Here is the webrev URL:
>>>
>>> http://cr.openjdk.java.net/~dcubed/8047720-webrev/0-jdk9-hs-rt/
>>>
>>> This deadlock occurred between the following threads:
>>>
>>>      Main thread   - Trying to stop the WatcherThread as part of
>>>                      shutting down the VM; this thread is blocked
>>>                      on the PeriodicTask_lock which keeps it from
>>>                      reaching a safepoint.
>>>      WatcherThread - Requested a VM_ForceSafepoint to complete
>>>                      a JavaThread::java_suspend() call as part
>>>                      of a FlatProfiler record_thread_ticks()
>>>                      call; this thread owns the PeriodicTask_lock
>>>                      since it is processing a periodic task.
>>>      VMThread      - Trying to start a safepoint; this thread is
>>>                      blocked waiting for the Main thread to reach
>>>                      a safepoint.
>>>
>>> The PeriodicTask_lock is one of the VM internal locks and is
>>> typically managed using Mutex::_no_safepoint_check_flag to
>>> avoid deadlocks. Yes, the irony is dripping on the floor... :-)
>>
>> What was overlooked here is that the holder of a lock that is acquired
>> without safepoint checks, must never block at a safepoint whilst holding
>> that lock.

I'm not sure about the above rule when you're talking about
internal VM threads that are really JavaThreads. JavaThreads
can run into all kinds of our special logic when doing thread
state transitions and the like. JavaThreads can also post JVM/TI
events which can lead to blocking at safepoints or blocking on
JVM/TI raw monitors in event handlers, etc...



>> In this case the blocking is indirect, caused by the
>> synchronous nature of the VM_Operation, rather than a direct result of
>> "blocking for the safepoint" (which the WatcherThread does not
>> participate in).

Right. The WatcherThread intentionally does not participate in
safepoints when using the PeriodicTask_lock. To me, that means
that other threads using that lock should not participate in
the safepoint protocol.

I believe we're trying to be consistent about how we use locks
with respect to honoring the safepoint protocol or not. Markus G
has a bug fix in process where he's trying to add sanity checks
that detect inconsistent use of locks. In this particular case,
I believe that the Main thread's use of the PeriodicTask_lock
with the safepoint protocol enabled would be seen as inconsistent.
However, I may not have all the details for what Markus is doing.


>> I wonder if the WatcherThread should really be using
>> the async variant of VM_ForceSafepoint here?

The WatcherThread is trying to do a JavaThread::java_suspend()
call. JavaThread::java_suspend() cannot return to the caller
until it is sure that the target thread will not execute any
more bytecodes. Switching to an async VM_ForceSafepoint would
break that invariant and cause subtle deadlocks because a target
JavaThread could acquire a Java monitor when the suspend
requesting thread thinks it is suspended. Been down that road
before and it is not pretty.



>>
>>> The interesting part of this deadlock is that I think that it
>>> is possible for other periodic tasks to hit it. Anything that
>>> causes the WatcherThread to start a safepoint while processing
>>> a periodic task should be susceptible to this race. Think about
>>> the -XX:+DeoptimizeALot option and how it causes VM_Deopt
>>> requests on thread state transitions... Interesting...
>>
>> I don't think so. You need three threads involved to get the deadlock.
>
> But that isn't the point. As you state this deadlock, at VM shutdown, 
> could impact any synchronous safepoint operations executed by the 
> WatcherThread.

Right. The problem is that the WatcherThread holds the
PeriodicTask_lock across the potential for VM operations
so we have to be very, very careful with non-WatcherThread
uses of that lock to avoid deadlocks.



>
> That aside ...
>
>> In the current case the main thread's locking of the PeriodicTask_lock
>> without a safepoint check is what causes the problem - that violates the
>> rules surrounding use of "no safepoint checks". The other methods that a
>> JavaThread might call that acquire the PeriodicTask_lock do perform the
>> safepoint checks, so they wouldn't deadlock. Hence it seems to me that
>> only WatcherThread::stop can lead to this problem. And as
>> WatcherThread::stop is only called from before_exit, and that can only
>> be called once, it seems to me that we could/should actually acquire the
>> lock with a safepoint check.

Agreed that only the WatcherThread::stop() call can lead to this deadlock
because it is that call that catches the PeriodicTask_lock being held
across a VM operation. That's the heart of this deadlock and we're only
differing on how to solve that deadlock.

The primary purpose of WatcherThread::stop() is setting the 
_should_terminate
flag so that the WatcherThread knows that it should stop dispatching 
periodic
tasks. _should_terminate is already volatile and the existing code does an
OrderAccess::fence() just to be sure the update is seen. The secondary
purpose of WatcherThread::stop() is waking up the WatcherThread so that it
stops in a more timely fashion. The WatcherThread waits for a fixed 
amount of
time so it will eventually wake up and see the new _should_terminate value.

If caller of WatcherThread::stop() cannot get the PeriodicTask_lock, then
that means the WatcherThread is active so WatcherThread::stop() only has
to set the flag. It is possible for the WatcherThread::stop() call to race
with the WatcherThread dropping the PeriodicTask_lock and waiting. In that
case, the WatcherThread waits for a fixed amount of time so it will
eventually wake up and see the new _should_terminate value.

I intentionally don't want WatcherThread::stop() to honor the safepoint
protocol when accessing the PeriodicTask_lock so that we remain consistent
with our use of that lock. See my comments above about Markus' work in this
area.

WatcherThread::stop() will honor the safepoint protocol a couple of lines
down from my changes:

1380   // it is ok to take late safepoints here, if needed
1381   MutexLocker mu(Terminator_lock);

The subsequent wait() loop also honors the safepoint protocol.

Dan


>
> David
> -----
>
>> Cheers,
>> David
>>
>>>
>>> Testing:
>>>      - I found a way to add delays to the right spots in the
>>>        VM to make the deadlock reproduce in just about every
>>>        run of the test associated with the bug. The new
>>>        os::naked_short_sleep() function is your friend. Thanks
>>>        to Fred for adding that! See the bug report for the
>>>        debugging diffs.
>>>      - 72 hours of running the test in the bug report with
>>>        delays enabled for product, fastdebug and jvmg bits
>>>        in parallel on my Solaris X86 server.
>>>      - JPRT test run
>>>      - Aurora Adhoc results are in process; we're having issues
>>>        with both a broken testbase build and infra problems
>>>        with results not being uploaded.
>>>

[prev in list] [next in list] [prev in thread] [next in thread]