Fwd: Patch: Clean up fix for JDK-8047720

[David Holmes]

Hi Carsten,

On 11/02/2015 8:18 AM, Carsten Varming wrote:
> Dear David,
>
> Any news regarding this fix? Anything I can do to help?

The bug has been assigned to Dan Daugherty so it depends on his 
workload. Verifying the safety of the change is a time consuming task.

Thanks,
David

> 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
>
>
>
>