Fix stop-the-world race. (#108)

Previously the MMTk coordinator thread pretents to be the VM thread and
calls SafepointSynchronize::begin() directly.  This makes it race with
the real VM thread.

This commit introduces a dedicated "VM companion thread". The companion
therad requests the VM thread to execute a stop-the-world VM operation
(a VMOp_ThirdPartyHeapOperation), and the VM operation waits for the
stop-the-world GC to finish and lets the VM thread start the world
again.  This ensures the real VM thread is the only thread that calls
SafepointSynchronize::begin().  The MMTk binding initiates stack
scanning during stop-the-world.

Also made minor fixes to MMTKCollectorThread and MMTKContextThread

-   Removed is_VM_thread and is_GC_thread methods.
    -   It is not the VM thread.
    -   The `is_GC_thread` method is never used in the real VM thread,
        and is removed in the upstream in a later revision.
-   Initialize their native thread names on start.
    -   The native names are visible in GDB when debugging.
    -   Prepended  "MMTk" to their names to make them more obvious.

Author: wks

mmtk/Cargo.toml

@@ -20,7 +20,7 @@ lazy_static = "1.1"
 # - change branch
 # - change repo name
 # But other changes including adding/removing whitespaces in commented lines may break the CI.
-mmtk = { git = "https://github.com/mmtk/mmtk-core.git", rev = "34acc5472942671f89d18d8313af46f9c4c7783e" }
+mmtk = { git = "https://github.com/mmtk/mmtk-core.git", rev = "aec43e51c9dc737bd533f4db5106857b4ac19407" }
 # Uncomment the following to build locally
 # mmtk = { path = "../repos/mmtk-core" }
 

openjdk/mmtkCollectorThread.cpp

@@ -28,9 +28,10 @@
 
 MMTkCollectorThread::MMTkCollectorThread(void* context): NamedThread() {
   third_party_heap_collector = context;
-  set_name("Collector Thread");
+  set_name("MMTk Collector Thread");
 }
 
 void MMTkCollectorThread::run() {
+  this->initialize_named_thread();
   start_worker((void*) this, third_party_heap_collector);
 }

openjdk/mmtkCollectorThread.hpp

@@ -39,10 +39,6 @@ class MMTkCollectorThread: public NamedThread {
     guarantee(false, "MMTkCollectorThread deletion must fix the race with VM termination");
   }
 
-  // Tester
-  bool is_VM_thread() const                      { return true; }
-  bool is_GC_thread() const                      { return true; }
-
   inline void* get_context() {
     return third_party_heap_collector;
   }

openjdk/mmtkContextThread.cpp

@@ -27,9 +27,10 @@
 #include "mmtkContextThread.hpp"
 
 MMTkContextThread::MMTkContextThread() : NamedThread() {
-  set_name("Controller Context Thread");
+  set_name("MMTk Controller Context Thread");
 }
 
 void MMTkContextThread::run() {
+  this->initialize_named_thread();
   start_control_collector((void*) this);
 }

openjdk/mmtkContextThread.hpp

@@ -38,10 +38,6 @@ class MMTkContextThread: public NamedThread {
     guarantee(false, "VMThread deletion must fix the race with VM termination");
   }
 
-  // Tester
-  bool is_VM_thread() const                      { return true; }
-  bool is_GC_thread() const                      { return true; }
-
   // Entry for starting vm thread
   virtual void run();
 };

openjdk/mmtkHeap.cpp

@@ -36,6 +36,7 @@
 #include "mmtkHeap.hpp"
 #include "mmtkMutator.hpp"
 #include "mmtkUpcalls.hpp"
+#include "mmtkVMCompanionThread.hpp"
 #include "oops/oop.inline.hpp"
 #include "runtime/atomic.hpp"
 #include "runtime/handles.inline.hpp"
@@ -105,6 +106,12 @@ jint MMTkHeap::initialize() {
   //barrier_set->initialize();
   BarrierSet::set_barrier_set(barrier_set);
 
+  _companion_thread = new MMTkVMCompanionThread();
+  if (!os::create_thread(_companion_thread, os::pgc_thread)) {
+    printf("Failed to create thread");
+    guarantee(false, "panic");
+  }
+  os::start_thread(_companion_thread);
   // Set up the GCTaskManager
   //  _mmtk_gc_task_manager = mmtkGCTaskManager::create(ParallelGCThreads);
   return JNI_OK;

openjdk/mmtkHeap.hpp

@@ -44,7 +44,7 @@
 class GCMemoryManager;
 class MemoryPool;
 //class mmtkGCTaskManager;
-
+class MMTkVMCompanionThread;
 class MMTkHeap : public CollectedHeap {
   MMTkCollectorPolicy* _collector_policy;
   SoftRefPolicy* _soft_ref_policy;
@@ -57,6 +57,7 @@ class MMTkHeap : public CollectedHeap {
   Monitor* _gc_lock;
   ContiguousSpace* _space;
   int _num_root_scan_tasks;
+  MMTkVMCompanionThread* _companion_thread;
 public:
 
   MMTkHeap(MMTkCollectorPolicy* policy);
@@ -75,6 +76,9 @@ class MMTkHeap : public CollectedHeap {
   virtual HeapWord* mem_allocate(size_t size, bool* gc_overhead_limit_was_exceeded);
   HeapWord* mem_allocate_nonmove(size_t size, bool* gc_overhead_limit_was_exceeded);
 
+  MMTkVMCompanionThread* companion_thread() const {
+    return _companion_thread;
+  }
 
 
   Name kind() const {

openjdk/mmtkUpcalls.cpp

@@ -32,28 +32,45 @@
 #include "mmtkHeap.hpp"
 #include "mmtkRootsClosure.hpp"
 #include "mmtkUpcalls.hpp"
+#include "mmtkVMCompanionThread.hpp"
 #include "runtime/mutexLocker.hpp"
 #include "runtime/os.hpp"
 #include "runtime/safepoint.hpp"
 #include "runtime/thread.hpp"
 #include "runtime/threadSMR.hpp"
 #include "runtime/vmThread.hpp"
 
-static bool gcInProgress = false;
+static size_t mmtk_start_the_world_count = 0;
 
 static void mmtk_stop_all_mutators(void *tls, void (*create_stack_scan_work)(void* mutator)) {
-  gcInProgress = true;
   MMTkHeap::_create_stack_scan_work = create_stack_scan_work;
-  SafepointSynchronize::begin();
+
+  log_debug(gc)("Requesting the VM to suspend all mutators...");
+  MMTkHeap::heap()->companion_thread()->request(MMTkVMCompanionThread::_threads_suspended, true);
+  log_debug(gc)("Mutators stopped. Now enumerate threads for scanning...");
+
+  {
+    JavaThreadIteratorWithHandle jtiwh;
+    while (JavaThread *cur = jtiwh.next()) {
+      MMTkHeap::heap()->report_java_thread_yield(cur);
+    }
+  }
+  log_debug(gc)("Finished enumerating threads.");
 }
 
 static void mmtk_resume_mutators(void *tls) {
   MMTkHeap::_create_stack_scan_work = NULL;
-  SafepointSynchronize::end();
-  MMTkHeap::heap()->gc_lock()->lock_without_safepoint_check();
-  gcInProgress = false;
-  MMTkHeap::heap()->gc_lock()->notify_all();
-  MMTkHeap::heap()->gc_lock()->unlock();
+
+  log_debug(gc)("Requesting the VM to resume all mutators...");
+  MMTkHeap::heap()->companion_thread()->request(MMTkVMCompanionThread::_threads_resumed, true);
+  log_debug(gc)("Mutators resumed. Now notify any mutators waiting for GC to finish...");
+
+  {
+    MutexLockerEx locker(MMTkHeap::heap()->gc_lock(), true);
+    mmtk_start_the_world_count++;
+    MMTkHeap::heap()->gc_lock()->notify_all();
+  }
+  log_debug(gc)("Mutators notified.");
 }
 
 static void mmtk_spawn_collector_thread(void* tls, void* ctx) {
@@ -76,15 +93,18 @@ static void mmtk_spawn_collector_thread(void* tls, void* ctx) {
 }
 
 static void mmtk_block_for_gc() {
-  gcInProgress = true;
   MMTkHeap::heap()->_last_gc_time = os::javaTimeNanos() / NANOSECS_PER_MILLISEC;
+  log_debug(gc)("Thread (id=%d) will block waiting for GC to finish.", Thread::current()->osthread()->thread_id());
   {
-    Monitor* gc_lock = MMTkHeap::heap()->gc_lock();
-    MutexLocker ml(gc_lock);
-    while (gcInProgress) {
-      gc_lock->wait();
+    MutexLocker locker(MMTkHeap::heap()->gc_lock());
+    size_t my_count = mmtk_start_the_world_count;
+    size_t next_count = my_count + 1;
+
+    while (mmtk_start_the_world_count < next_count) {
+      MMTkHeap::heap()->gc_lock()->wait();
     }
   }
+  log_debug(gc)("Thread (id=%d) resumed after GC finished.", Thread::current()->osthread()->thread_id());
 }
 
 static void* mmtk_get_mmtk_mutator(void* tls) {

openjdk/mmtkVMCompanionThread.cpp

@@ -0,0 +1,138 @@
+/*
+ * Copyright (c) 1998, 2017, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#include "precompiled.hpp"
+#include "mmtk.h"
+#include "mmtkVMCompanionThread.hpp"
+#include "runtime/mutex.hpp"
+
+MMTkVMCompanionThread::MMTkVMCompanionThread():
+    NamedThread(),
+    _desired_state(_threads_resumed),
+    _reached_state(_threads_resumed) {
+  set_name("MMTK VM Companion Thread");
+  _lock = new Monitor(Monitor::nonleaf,
+                      "MMTkVMCompanionThread::_lock",
+                      true,
+                      Monitor::_safepoint_check_never);
+}
+
+MMTkVMCompanionThread::~MMTkVMCompanionThread() {
+  guarantee(false, "MMTkVMCompanionThread deletion must fix the race with VM termination");
+}
+
+void MMTkVMCompanionThread::run() {
+  this->initialize_named_thread();
+
+  for (;;) {
+    // Wait for suspend request
+    log_trace(gc)("MMTkVMCompanionThread: Waiting for suspend request...");
+    {
+      MutexLockerEx locker(_lock, Mutex::_no_safepoint_check_flag);
+      assert(_reached_state == _threads_resumed, "Threads should be running at this moment.");
+      while (_desired_state != _threads_suspended) {
+        _lock->wait(true);
+      }
+      assert(_reached_state == _threads_resumed, "Threads should still be running at this moment.");
+    }
+
+    // Let the VM thread stop the world.
+    log_trace(gc)("MMTkVMCompanionThread: Letting VMThread execute VM op...");
+    VM_MMTkSTWOperation op(this);
+    // VMThread::execute() is blocking. The companion thread will be blocked
+    // here waiting for the VM thread to execute op, and the VM thread will
+    // be blocked in reach_suspended_and_wait_for_resume() until a GC thread
+    // calls request(_threads_resumed).
+    VMThread::execute(&op);
+
+    // Tell the waiter thread that the world has resumed.
+    log_trace(gc)("MMTkVMCompanionThread: Notifying threads resumption...");
+    {
+      MutexLockerEx locker(_lock, Mutex::_no_safepoint_check_flag);
+      assert(_desired_state == _threads_resumed, "start-the-world should be requested.");
+      assert(_reached_state == _threads_suspended, "Threads should still be suspended at this moment.");
+      _reached_state = _threads_resumed;
+      _lock->notify_all();
+    }
+  }
+}
+
+// Request stop-the-world or start-the-world.  This method is supposed to be
+// called by a GC thread.
+//
+// If wait_until_reached is true, the caller will block until all Java threads
+// have stopped, or until they have been waken up.
+//
+// If wait_until_reached is false, the caller will return immediately, while
+// the companion thread will ask the VM thread to perform the state transition
+// in the background. The caller may call the wait_for_reached method to block
+// until the desired state is reached.
+void MMTkVMCompanionThread::request(stw_state desired_state, bool wait_until_reached) {
+  assert(!Thread::current()->is_VM_thread(), "Requests can only be made by GC threads. Found VM thread.");
+  assert(Thread::current() != this, "Requests can only be made by GC threads. Found companion thread.");
+  assert(!Thread::current()->is_Java_thread(), "Requests can only be made by GC threads. Found Java thread.");
+
+  MutexLockerEx locker(_lock, Mutex::_no_safepoint_check_flag);
+  assert(_desired_state != desired_state, "State %d already requested.", desired_state);
+  _desired_state = desired_state;
+  _lock->notify_all();
+
+  if (wait_until_reached) {
+    while (_reached_state != desired_state) {
+      _lock->wait(true);
+    }
+  }
+}
+
+// Wait until the desired state is reached.  Usually called after calling the
+// request method.  Supposed to be called by a GC thread.
+void MMTkVMCompanionThread::wait_for_reached(stw_state desired_state) {
+  assert(!Thread::current()->is_VM_thread(), "Supposed to be called by GC threads. Found VM thread.");
+  assert(Thread::current() != this, "Supposed to be called by GC threads. Found companion thread.");
+  assert(!Thread::current()->is_Java_thread(), "Supposed to be called by GC threads. Found Java thread.");
+
+  MutexLockerEx locker(_lock, Mutex::_no_safepoint_check_flag);
+  assert(_desired_state == desired_state, "State %d not requested.", desired_state);
+
+  while (_reached_state != desired_state) {
+    _lock->wait(true);
+  }
+}
+
+// Called by the VM thread to indicate that all Java threads have stopped.
+// This method will block until the GC requests start-the-world.
+void MMTkVMCompanionThread::reach_suspended_and_wait_for_resume() {
+  assert(Thread::current()->is_VM_thread(), "reach_suspended_and_wait_for_resume can only be executed by the VM thread");
+
+  MutexLockerEx locker(_lock, Mutex::_no_safepoint_check_flag);
+
+  // Tell the waiter thread that the world has stopped.
+  _reached_state = _threads_suspended;
+  _lock->notify_all();
+
+  // Wait until resume-the-world is requested
+  while (_desired_state != _threads_resumed) {
+    _lock->wait(true);
+  }
+}

openjdk/mmtkVMCompanionThread.hpp

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 1998, 2016, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ *
+ */
+
+#ifndef MMTK_OPENJDK_MMTK_VM_COMPANION_THREAD_HPP
+#define MMTK_OPENJDK_MMTK_VM_COMPANION_THREAD_HPP
+
+#include "mmtkVMOperation.hpp"
+#include "runtime/mutex.hpp"
+#include "runtime/perfData.hpp"
+#include "runtime/thread.hpp"
+#include "runtime/vmOperations.hpp"
+
+// This thread cooperates with the VMThread to allow stopping the world without
+// blocking any GC threads.
+//
+// In HotSpot, the way to stop all Java threads for stop-the-world GC is
+// letting the VMThread execute a blocking VM_Operation.  However, the MMTk
+// expects the VM to provide two non-blocking methods to stop and start the
+// the world, whthout blocking the callers.  This thread bridges the API gap
+// by calling VMThread::execute on behalf of GC threads upon reques so that it
+// blocks this thread instead of GC threads.
+class MMTkVMCompanionThread: public NamedThread {
+public:
+  enum stw_state {
+    _threads_suspended,
+    _threads_resumed,
+  };
+private:
+  Monitor* _lock;
+  stw_state _desired_state;
+  stw_state _reached_state;
+
+public:
+  // Constructor
+  MMTkVMCompanionThread();
+  ~MMTkVMCompanionThread();
+
+  virtual void run() override;
+
+  // Interface for MMTk Core
+  void request(stw_state desired_state, bool wait_until_reached);
+  void wait_for_reached(stw_state reached_state);
+
+  // Interface for the VM_MMTkSTWOperation
+  void reach_suspended_and_wait_for_resume();
+};
+
+#endif // MMTK_OPENJDK_MMTK_VM_COMPANION_THREAD_HPP