Fwd: Patch: Clean up fix for JDK-8047720

[Carsten Varming]

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> 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>
> 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/20150217/bc33289c/attachment-0001.html>