Fwd: Patch: Clean up fix for JDK-8047720

[Carsten Varming]

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> 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>
> 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/20150210/6b1abb94/attachment.html>