Fwd: Patch: Clean up fix for JDK-8047720

[Daniel D. Daugherty]

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/1926cdf3/attachment.html>