Carsten,
I believe I have all the changes that you recommended in your
better.patch file. Not 100% byte-for-byte, but I think we're
covered.
Dan
On 2/17/15 11:09 AM, Carsten Varming wrote:
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.daughe...@oracle.com <mailto:daniel.daughe...@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
<varm...@gmail.com <mailto:varm...@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.hol...@oracle.com <mailto:david.hol...@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.hol...@oracle.com <mailto:david.hol...@oracle.com>
<mailto:david.hol...@oracle.com
<mailto:david.hol...@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
