Fwd: Patch: Clean up fix for JDK-8047720

Tue Feb 17 18:34:31 UTC 2015

Carsten,

I believe I have all the changes that you recommended in your
better.patch file. Not 100% byte-for-byte, but I think we're
covered.

Dan

On 2/17/15 11:09 AM, Carsten Varming wrote:
> Dear Daniel,
>
> Great news.
>
> I was reading your comments on JDK-8072439 and just want to point out 
> the other race that my patch fixed:
>
> WatcherThread::run sees !_should_terminate, and calls sleep. 
> WatcherThread::stop takes PeriodicTask_lock, sets _should_terminate, 
> notify PeriodicTask_lock, and release the lock. Sleep wait forever on 
> PeriodicTask_lock. Notice that the wait forever happens only if there 
> are no periodic tasks scheduled, so this might not be reproducible 
> (even with suitable sleeps), yet it is a lurking bug.
>
> Let me know if there is anything I can do to help,
> Carsten
>
> On Tue, Feb 17, 2015 at 12:43 PM, Daniel D. Daugherty 
> <daniel.daugherty at oracle.com <mailto:daniel.daugherty at oracle.com>> wrote:
>
>     Carsten.
>
>     I've started diving into this issue. Please see the updates
>     that I've been making to:
>
>     JDK-8072439 fix for 8047720 may need more work
>     https://bugs.openjdk.java.net/browse/JDK-8072439
>
>     Dan
>
>
>     On 2/10/15 3:18 PM, Carsten Varming wrote:
>>     Dear David,
>>
>>     Any news regarding this fix? Anything I can do to help?
>>
>>     Best wishes,
>>     Carsten
>>
>>     On Wed, Feb 4, 2015 at 12:54 PM, Carsten Varming
>>     <varming at gmail.com <mailto:varming at gmail.com>> wrote:
>>
>>         Dear David,
>>
>>         I took a look at all occurrences of PeriodicTask_lock. We are
>>         in luck:
>>
>>         PeriodicTask::enroll and PeriodicTask::disenroll check for
>>         safepoints when acquiring PeriodicTask_lock.
>>
>>         PeriodicTask::time_to_wait and PeriodicTask::real_time_tick
>>         are called only by the watcher thread.
>>
>>         WatcherThread::unpark is used only in contexts where
>>         PeriodicTask_lock is owned by the caller.
>>
>>         WatcherThread::sleep is called only by the watcher thread.
>>
>>         I took another look at WatcherThread::sleep and realized that
>>         there is a path from acquiring PeriodicTask_lock to waiting
>>         on the lock where _should_terminate is not checked. I added
>>         that to the minimal fix attached.
>>
>>         Looking at these methods made me want to clean up a little
>>         more. See better.patch attached.
>>
>>         I feel pretty good about the patch with the limited usage of
>>         no_safepoint_check_flag with PeriodicTask_lock.
>>
>>         Carsten
>>
>>
>>         On Tue, Feb 3, 2015 at 11:28 PM, David Holmes
>>         <david.holmes at oracle.com <mailto:david.holmes at oracle.com>> wrote:
>>
>>             On 4/02/2015 1:28 PM, Carsten Varming wrote:
>>
>>                 Dear David Holmes,
>>
>>                 Please see inline response,
>>
>>                 On Tue, Feb 3, 2015 at 9:38 PM, David Holmes
>>                 <david.holmes at oracle.com <mailto:david.holmes at oracle.com>
>>                 <mailto:david.holmes at oracle.com
>>                 <mailto:david.holmes at oracle.com>>> wrote:
>>
>>                     On 4/02/2015 5:00 AM, Carsten Varming wrote:
>>
>>                         Greetings all,
>>
>>                         I was recently introduced to the deadlock
>>                 described in
>>                         JDK-8047720 and
>>                         fixed in JDK9. The fix seems a little messy
>>                 to me, and it looks
>>                         like it
>>                         left some very short races in the code. So I
>>                 decided to clean up the
>>                         code. See attached diff.
>>
>>                         The change takes a step back and acquires
>>                 PeriodicTask_lock in
>>                         WatcherThread::stop (like before the fix in
>>                 JDK9), but this time
>>                         safepoints are allowed to proceed when
>>                 acquiring PeriodicTask_lock,
>>                         preventing the deadlock.
>>
>>
>>                     It isn't obvious that blocking for a safepoint
>>                 whilst in this method
>>                     will always be a safe thing to do. That would
>>                 need to be examined in
>>                     detail - potential interactions can be very subtle.
>>
>>
>>                 Absolutely true. For reference, the pattern is
>>                 repeated with the
>>                 Terminator_lock a few lines below. The pattern is
>>                 also used in
>>                 Threads::destroy_vm before and after calling
>>                 before_exit, and the java
>>                 shutdown hooks are called right before the call to
>>                 before_exit. So there
>>                 is a lot of evidence that safepoints are allowed to
>>                 happen in this context.
>>
>>
>>             The thread calling before_exit is a JavaThread so of
>>             course it participates in safepoints. The issue is
>>             whether the interaction between that thread and the
>>             WatcherThread, via the PeriodicTask_lock, can allow for
>>             the JavaThread blocking at a safepoint whilst owning that
>>             lock. If another JavaThread can try to lock it without a
>>             safepoint check then we can get a deadlock.
>>
>>             Cheers,
>>             David
>>
>>
>>                 Thank you for taking your time to look at this,
>>                 Carsten
>>
>>
>>                     David
>>
>>
>>                         Let me know what you think,
>>                         Carsten
>>
>>
>>
>>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150217/462690ee/attachment.html>