8072439: fix for 8047720 may need more work

Cleanup PeriodTask_lock usage. Also reviewed by varming@gmail.com.

Co-authored-by: Carsten Varming <varming@gmail.com>
Reviewed-by: dholmes, dcubed, mgronlun, coleenp

hotspot/src/share/vm/runtime/java.cpp

@@ -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 @@ void before_exit(JavaThread * thread) {
     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()) {

hotspot/src/share/vm/runtime/task.cpp

@@ -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 @@ void PeriodicTask::print_intervals() {
 #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 @@ void PeriodicTask::real_time_tick(int delay_time) {
 #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 @@ void PeriodicTask::real_time_tick(int delay_time) {
 }
 
 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(size_t interval_time) :
 }
 
 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 @@ void PeriodicTask::enroll() {
   _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++)

hotspot/src/share/vm/runtime/task.hpp

@@ -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 @@ class PeriodicTask: public CHeapObj<mtInternal> {
 
   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 @@ class PeriodicTask: public CHeapObj<mtInternal> {
 
   // 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

hotspot/src/share/vm/runtime/thread.cpp

@@ -1197,8 +1197,15 @@ WatcherThread::WatcherThread() : Thread(), _crash_protection(NULL) {
 }
 
 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 @@ int WatcherThread::sleep() const {
 
   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 @@ void WatcherThread::run() {
   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 @@ void WatcherThread::run() {
       }
     }
 
+    if (_should_terminate) {
+      // check for termination before posting the next tick
+      break;
+    }
+
     PeriodicTask::real_time_tick(time_waited);
   }
 
@@ -1319,27 +1332,19 @@ void WatcherThread::make_startable() {
 }
 
 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();
+  {
+    // Follow normal safepoint aware lock enter protocol since the
+    // WatcherThread is stopped by another JavaThread.
+    MutexLocker ml(PeriodicTask_lock);
+    _should_terminate = true;
 
-  _should_terminate = true;
-  OrderAccess::fence();  // ensure WatcherThread sees update in main loop
-
-  if (have_lock) {
     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::stop() {
 }
 
 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 @@ jint Threads::create_vm(JavaVMInitArgs* args, bool* canTryAgain) {
   }
 
   {
-    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

hotspot/src/share/vm/runtime/thread.hpp

@@ -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 @@ class WatcherThread: public Thread {
   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: