8072439: fix for 8047720 may need more work
Summary: Cleanup PeriodTask_lock usage. Also reviewed by varming@gmail.com.
Reviewed-by: dholmes, dcubed, mgronlun, coleenp
Contributed-by: varming@gmail.com, daniel.daugherty@oracle.com
--- a/hotspot/src/share/vm/runtime/java.cpp Mon Mar 02 05:32:01 2015 -0800
+++ b/hotspot/src/share/vm/runtime/java.cpp Mon Mar 02 16:31:25 2015 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -421,9 +421,11 @@
os::infinite_sleep();
}
- // Terminate watcher thread - must before disenrolling any periodic task
- if (PeriodicTask::num_tasks() > 0)
+ // Stop the WatcherThread. We do this before disenrolling various
+ // PeriodicTasks to reduce the likelihood of races.
+ if (PeriodicTask::num_tasks() > 0) {
WatcherThread::stop();
+ }
// Print statistics gathered (profiling ...)
if (Arguments::has_profile()) {
--- a/hotspot/src/share/vm/runtime/task.cpp Mon Mar 02 05:32:01 2015 -0800
+++ b/hotspot/src/share/vm/runtime/task.cpp Mon Mar 02 16:31:25 2015 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -47,6 +47,8 @@
#endif
void PeriodicTask::real_time_tick(int delay_time) {
+ assert(Thread::current()->is_Watcher_thread(), "must be WatcherThread");
+
#ifndef PRODUCT
if (ProfilerCheckIntervals) {
_ticks++;
@@ -60,6 +62,8 @@
#endif
{
+ // The WatcherThread does not participate in the safepoint protocol
+ // for the PeriodicTask_lock because it is not a JavaThread.
MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
int orig_num_tasks = _num_tasks;
@@ -74,8 +78,7 @@
}
int PeriodicTask::time_to_wait() {
- MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ?
- NULL : PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
+ assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock required");
if (_num_tasks == 0) {
return 0; // sleep until shutdown or a task is enrolled
@@ -98,14 +101,19 @@
}
PeriodicTask::~PeriodicTask() {
+ // This PeriodicTask may have already been disenrolled by a call
+ // to disenroll() before the PeriodicTask was deleted.
disenroll();
}
-/* enroll could be called from a JavaThread, so we have to check for
- * safepoint when taking the lock to avoid deadlocking */
+// enroll the current PeriodicTask
void PeriodicTask::enroll() {
- MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ?
- NULL : PeriodicTask_lock);
+ // Follow normal safepoint aware lock enter protocol if the caller does
+ // not already own the PeriodicTask_lock. Otherwise, we don't try to
+ // enter it again because VM internal Mutexes do not support recursion.
+ //
+ MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? NULL
+ : PeriodicTask_lock);
if (_num_tasks == PeriodicTask::max_tasks) {
fatal("Overflow in PeriodicTask table");
@@ -113,18 +121,21 @@
_tasks[_num_tasks++] = this;
WatcherThread* thread = WatcherThread::watcher_thread();
- if (thread) {
+ if (thread != NULL) {
thread->unpark();
} else {
WatcherThread::start();
}
}
-/* disenroll could be called from a JavaThread, so we have to check for
- * safepoint when taking the lock to avoid deadlocking */
+// disenroll the current PeriodicTask
void PeriodicTask::disenroll() {
- MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ?
- NULL : PeriodicTask_lock);
+ // Follow normal safepoint aware lock enter protocol if the caller does
+ // not already own the PeriodicTask_lock. Otherwise, we don't try to
+ // enter it again because VM internal Mutexes do not support recursion.
+ //
+ MutexLockerEx ml(PeriodicTask_lock->owned_by_self() ? NULL
+ : PeriodicTask_lock);
int index;
for(index = 0; index < _num_tasks && _tasks[index] != this; index++)
--- a/hotspot/src/share/vm/runtime/task.hpp Mon Mar 02 05:32:01 2015 -0800
+++ b/hotspot/src/share/vm/runtime/task.hpp Mon Mar 02 16:31:25 2015 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -54,6 +54,7 @@
static int _num_tasks;
static PeriodicTask* _tasks[PeriodicTask::max_tasks];
+ // Can only be called by the WatcherThread
static void real_time_tick(int delay_time);
#ifndef PRODUCT
@@ -98,6 +99,7 @@
// Calculate when the next periodic task will fire.
// Called by the WatcherThread's run method.
+ // Requires the PeriodicTask_lock.
static int time_to_wait();
// The task to perform at each period
--- a/hotspot/src/share/vm/runtime/thread.cpp Mon Mar 02 05:32:01 2015 -0800
+++ b/hotspot/src/share/vm/runtime/thread.cpp Mon Mar 02 16:31:25 2015 -0800
@@ -1197,8 +1197,15 @@
}
int WatcherThread::sleep() const {
+ // The WatcherThread does not participate in the safepoint protocol
+ // for the PeriodicTask_lock because it is not a JavaThread.
MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
+ if (_should_terminate) {
+ // check for termination before we do any housekeeping or wait
+ return 0; // we did not sleep.
+ }
+
// remaining will be zero if there are no tasks,
// causing the WatcherThread to sleep until a task is
// enrolled
@@ -1211,8 +1218,9 @@
jlong time_before_loop = os::javaTimeNanos();
- for (;;) {
- bool timedout = PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag, remaining);
+ while (true) {
+ bool timedout = PeriodicTask_lock->wait(Mutex::_no_safepoint_check_flag,
+ remaining);
jlong now = os::javaTimeNanos();
if (remaining == 0) {
@@ -1253,7 +1261,7 @@
this->initialize_thread_local_storage();
this->set_native_thread_name(this->name());
this->set_active_handles(JNIHandleBlock::allocate_block());
- while (!_should_terminate) {
+ while (true) {
assert(watcher_thread() == Thread::current(), "thread consistency check");
assert(watcher_thread() == this, "thread consistency check");
@@ -1289,6 +1297,11 @@
}
}
+ if (_should_terminate) {
+ // check for termination before posting the next tick
+ break;
+ }
+
PeriodicTask::real_time_tick(time_waited);
}
@@ -1319,27 +1332,19 @@
}
void WatcherThread::stop() {
- // Get the PeriodicTask_lock if we can. If we cannot, then the
- // WatcherThread is using it and we don't want to block on that lock
- // here because that might cause a safepoint deadlock depending on
- // what the current WatcherThread tasks are doing.
- bool have_lock = PeriodicTask_lock->try_lock();
-
- _should_terminate = true;
- OrderAccess::fence(); // ensure WatcherThread sees update in main loop
-
- if (have_lock) {
+ {
+ // Follow normal safepoint aware lock enter protocol since the
+ // WatcherThread is stopped by another JavaThread.
+ MutexLocker ml(PeriodicTask_lock);
+ _should_terminate = true;
+
WatcherThread* watcher = watcher_thread();
if (watcher != NULL) {
- // If we managed to get the lock, then we should unpark the
- // WatcherThread so that it can see we want it to stop.
+ // unpark the WatcherThread so it can see that it should terminate
watcher->unpark();
}
-
- PeriodicTask_lock->unlock();
}
- // it is ok to take late safepoints here, if needed
MutexLocker mu(Terminator_lock);
while (watcher_thread() != NULL) {
@@ -1359,9 +1364,7 @@
}
void WatcherThread::unpark() {
- MutexLockerEx ml(PeriodicTask_lock->owned_by_self()
- ? NULL
- : PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
+ assert(PeriodicTask_lock->owned_by_self(), "PeriodicTask_lock required");
PeriodicTask_lock->notify();
}
@@ -3558,8 +3561,8 @@
}
{
- MutexLockerEx ml(PeriodicTask_lock, Mutex::_no_safepoint_check_flag);
- // Make sure the watcher thread can be started by WatcherThread::start()
+ MutexLocker ml(PeriodicTask_lock);
+ // Make sure the WatcherThread can be started by WatcherThread::start()
// or by dynamic enrollment.
WatcherThread::make_startable();
// Start up the WatcherThread if there are any periodic tasks
--- a/hotspot/src/share/vm/runtime/thread.hpp Mon Mar 02 05:32:01 2015 -0800
+++ b/hotspot/src/share/vm/runtime/thread.hpp Mon Mar 02 16:31:25 2015 -0800
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 1997, 2014, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2015, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
@@ -701,7 +701,8 @@
static WatcherThread* _watcher_thread;
static bool _startable;
- volatile static bool _should_terminate; // updated without holding lock
+ // volatile due to at least one lock-free read
+ volatile static bool _should_terminate;
os::WatcherThreadCrashProtection* _crash_protection;
public: