From af5ba8c0eaf5130dbf9315f151ce061f9d662f27 Mon Sep 17 00:00:00 2001
From: Kunshan Wang <wks1986@gmail.com>
Date: Fri, 6 Aug 2021 16:49:18 +0800
Subject: [PATCH] Fix stop-the-world race.

With the associated commit in the mmtk-openjdk repository,
stop-the-world and start-the-world requests no longer have to be made in
the MMTk coordinator thread.

Considering that some VM may still require the thread that stops the
world and the thread that starts the world to be the same, we added a
constant in the Collection trait so that VM bindings can override it
when needed.
---
 src/scheduler/gc_work.rs | 67 ++++++++++++++++++++++------------------
 src/vm/collection.rs     |  9 ++++++
 2 files changed, 46 insertions(+), 30 deletions(-)

diff --git a/src/scheduler/gc_work.rs b/src/scheduler/gc_work.rs
index e7ed04f539..5b69f3802f 100644
--- a/src/scheduler/gc_work.rs
+++ b/src/scheduler/gc_work.rs
@@ -191,41 +191,43 @@ impl<ScanEdges: ProcessEdgesWork> StopMutators<ScanEdges> {
 
 impl<E: ProcessEdgesWork> GCWork<E::VM> for StopMutators<E> {
     fn do_work(&mut self, worker: &mut GCWorker<E::VM>, mmtk: &'static MMTK<E::VM>) {
-        if worker.is_coordinator() {
-            trace!("stop_all_mutators start");
-            debug_assert_eq!(mmtk.plan.base().scanned_stacks.load(Ordering::SeqCst), 0);
-            <E::VM as VMBinding>::VMCollection::stop_all_mutators::<E>(worker.tls);
-            trace!("stop_all_mutators end");
-            mmtk.scheduler.notify_mutators_paused(mmtk);
-            if <E::VM as VMBinding>::VMScanning::SCAN_MUTATORS_IN_SAFEPOINT {
-                // Prepare mutators if necessary
-                // FIXME: This test is probably redundant. JikesRVM requires to call `prepare_mutator` once after mutators are paused
-                if !mmtk.plan.base().stacks_prepared() {
-                    for mutator in <E::VM as VMBinding>::VMActivePlan::mutators() {
-                        <E::VM as VMBinding>::VMCollection::prepare_mutator(
-                            worker.tls,
-                            mutator.get_tls(),
-                            mutator,
-                        );
-                    }
+        // If the VM requires that only the coordinator thread can stop the world,
+        // we delegate the work to the coordinator.
+        if <E::VM as VMBinding>::VMCollection::COORDINATOR_ONLY_STW && !worker.is_coordinator() {
+            mmtk.scheduler
+                .add_coordinator_work(StopMutators::<E>::new(), worker);
+            return;
+        }
+
+        trace!("stop_all_mutators start");
+        debug_assert_eq!(mmtk.plan.base().scanned_stacks.load(Ordering::SeqCst), 0);
+        <E::VM as VMBinding>::VMCollection::stop_all_mutators::<E>(worker.tls);
+        trace!("stop_all_mutators end");
+        mmtk.scheduler.notify_mutators_paused(mmtk);
+        if <E::VM as VMBinding>::VMScanning::SCAN_MUTATORS_IN_SAFEPOINT {
+            // Prepare mutators if necessary
+            // FIXME: This test is probably redundant. JikesRVM requires to call `prepare_mutator` once after mutators are paused
+            if !mmtk.plan.base().stacks_prepared() {
+                for mutator in <E::VM as VMBinding>::VMActivePlan::mutators() {
+                    <E::VM as VMBinding>::VMCollection::prepare_mutator(
+                        worker.tls,
+                        mutator.get_tls(),
+                        mutator,
+                    );
                 }
-                // Scan mutators
-                if <E::VM as VMBinding>::VMScanning::SINGLE_THREAD_MUTATOR_SCANNING {
+            }
+            // Scan mutators
+            if <E::VM as VMBinding>::VMScanning::SINGLE_THREAD_MUTATOR_SCANNING {
+                mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
+                    .add(ScanStackRoots::<E>::new());
+            } else {
+                for mutator in <E::VM as VMBinding>::VMActivePlan::mutators() {
                     mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
-                        .add(ScanStackRoots::<E>::new());
-                } else {
-                    for mutator in <E::VM as VMBinding>::VMActivePlan::mutators() {
-                        mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
-                            .add(ScanStackRoot::<E>(mutator));
-                    }
+                        .add(ScanStackRoot::<E>(mutator));
                 }
             }
-            mmtk.scheduler.work_buckets[WorkBucketStage::Prepare]
-                .add(ScanVMSpecificRoots::<E>::new());
-        } else {
-            mmtk.scheduler
-                .add_coordinator_work(StopMutators::<E>::new(), worker);
         }
+        mmtk.scheduler.work_buckets[WorkBucketStage::Prepare].add(ScanVMSpecificRoots::<E>::new());
     }
 }
 
@@ -244,6 +246,11 @@ impl<VM: VMBinding> GCWork<VM> for EndOfGC {
             crate::util::edge_logger::reset();
         }
 
+        if <VM as VMBinding>::VMCollection::COORDINATOR_ONLY_STW {
+            assert!(worker.is_coordinator(),
+                    "VM only allows coordinator to resume mutators, but the current worker is not the coordinator.");
+        }
+
         mmtk.plan.base().set_gc_status(GcStatus::NotInGC);
         <VM as VMBinding>::VMCollection::resume_mutators(worker.tls);
     }
diff --git a/src/vm/collection.rs b/src/vm/collection.rs
index 9d22aebbce..e40076d077 100644
--- a/src/vm/collection.rs
+++ b/src/vm/collection.rs
@@ -6,6 +6,15 @@ use crate::vm::VMBinding;
 
 /// VM-specific methods for garbage collection.
 pub trait Collection<VM: VMBinding> {
+    /// If true, only the coordinator thread can call stop_all_mutators and the resume_mutators methods.
+    /// If false, any GC thread can call these methods.
+    ///
+    /// This constant exists because some VMs require the thread that resumes a thread to be the same thread that
+    /// stopped it.  The MMTk Core will use the appropriate thread to stop or start the world according to the value of
+    /// this constant.  If a VM does not have such a requirement, the VM binding shall set this to false to reduce an
+    /// unnecessary context switch.
+    const COORDINATOR_ONLY_STW: bool = true;
+
     /// Stop all the mutator threads. MMTk calls this method when it requires all the mutator to yield for a GC.
     /// This method is called by a single thread in MMTk (the GC controller).
     /// This method should not return until all the threads are yielded.