Fwd: Patch: Clean up fix for JDK-8047720

[Carsten Varming]

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>
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>> 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/20150204/e7aab00e/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: better.patch
Type: text/x-patch
Size: 3171 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150204/e7aab00e/better-0001.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: minimal.patch
Type: text/x-patch
Size: 1834 bytes
Desc: not available
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20150204/e7aab00e/minimal-0001.patch>