From 53d15ebafc4bf6d516e7cb1751bdc1ec8bbbd005 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 16 Feb 2023 13:59:37 +0800 Subject: [PATCH 1/4] Workaround spuriously waken GC worker. If a worker spuriously wakes up while the coordinator closes all buckets, it may read inconsistent states. --- src/scheduler/controller.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/scheduler/controller.rs b/src/scheduler/controller.rs index c39aea05e8..9d542d4fd4 100644 --- a/src/scheduler/controller.rs +++ b/src/scheduler/controller.rs @@ -128,7 +128,11 @@ impl GCController { CoordinatorMessage::Finish => {} } } - self.scheduler.deactivate_all(); + + { + let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); + self.scheduler.deactivate_all(); + } // Tell GC trigger that GC ended - this happens before EndOfGC where we resume mutators. self.mmtk.plan.base().gc_trigger.policy.on_gc_end(self.mmtk); From 2992c5b20bcfa2041acfdcaab69dd5672e925e6e Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 16 Feb 2023 15:04:31 +0800 Subject: [PATCH 2/4] Prevent opening buckets during schedule_collection --- src/scheduler/controller.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/scheduler/controller.rs b/src/scheduler/controller.rs index 9d542d4fd4..9bdcc5ad11 100644 --- a/src/scheduler/controller.rs +++ b/src/scheduler/controller.rs @@ -101,7 +101,18 @@ impl GCController { /// Coordinate workers to perform GC in response to a GC request. pub fn do_gc_until_completion(&mut self) { // Schedule collection. + // Note: GC workers will start executing work packets as soon as individual work packets + // are added. This means workers may open subsequent buckets while `schedule_collection` + // still has packets to be added to prior buckets. + // We use the`pending_coordinator_packets` to prevent the workers from opening any work + // buckets. + self.scheduler.pending_coordinator_packets.fetch_add(1, Ordering::SeqCst); ScheduleCollection.do_work_with_stat(&mut self.coordinator_worker, self.mmtk); + self.scheduler.pending_coordinator_packets.fetch_sub(1, Ordering::SeqCst); + { + let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); + self.scheduler.worker_monitor.1.notify_all(); + } // Tell GC trigger that GC started - this happens after ScheduleCollection so we // will know what kind of GC this is (e.g. nursery vs mature in gen copy, defrag vs fast in Immix) From 81be968d773c303a358536592fb847c5798c4c98 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Thu, 16 Feb 2023 15:05:36 +0800 Subject: [PATCH 3/4] Formatting --- src/scheduler/controller.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/scheduler/controller.rs b/src/scheduler/controller.rs index 9bdcc5ad11..7ca5d56503 100644 --- a/src/scheduler/controller.rs +++ b/src/scheduler/controller.rs @@ -106,9 +106,13 @@ impl GCController { // still has packets to be added to prior buckets. // We use the`pending_coordinator_packets` to prevent the workers from opening any work // buckets. - self.scheduler.pending_coordinator_packets.fetch_add(1, Ordering::SeqCst); + self.scheduler + .pending_coordinator_packets + .fetch_add(1, Ordering::SeqCst); ScheduleCollection.do_work_with_stat(&mut self.coordinator_worker, self.mmtk); - self.scheduler.pending_coordinator_packets.fetch_sub(1, Ordering::SeqCst); + self.scheduler + .pending_coordinator_packets + .fetch_sub(1, Ordering::SeqCst); { let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); self.scheduler.worker_monitor.1.notify_all(); From 6cb0d982c78453e70902f24a9f920b2fc54171f0 Mon Sep 17 00:00:00 2001 From: Kunshan Wang Date: Fri, 17 Feb 2023 11:31:20 +0800 Subject: [PATCH 4/4] Initiate and execute coordinator work Added methods to initiate and execute coordinator work packets. This will unify the handling of all coordinator work. --- src/scheduler/controller.rs | 85 +++++++++++++++++++++++-------------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/src/scheduler/controller.rs b/src/scheduler/controller.rs index 7ca5d56503..eeb70833ba 100644 --- a/src/scheduler/controller.rs +++ b/src/scheduler/controller.rs @@ -14,7 +14,7 @@ use crate::vm::VMBinding; use crate::MMTK; use atomic::Ordering; -use super::{GCWork, GCWorkScheduler, GCWorker}; +use super::{CoordinatorWork, GCWorkScheduler, GCWorker}; /// The thread local struct for the GC controller, the counterpart of `GCWorker`. pub struct GCController { @@ -69,22 +69,9 @@ impl GCController { /// Process a message. Return true if the GC is finished. fn process_message(&mut self, message: CoordinatorMessage) -> bool { - let worker = &mut self.coordinator_worker; - let mmtk = self.mmtk; match message { CoordinatorMessage::Work(mut work) => { - work.do_work_with_stat(worker, mmtk); - let old_count = self - .scheduler - .pending_coordinator_packets - .fetch_sub(1, Ordering::SeqCst); - if old_count == 1 { - // When the coordinator finishes executing all coordinator work packets, - // it is a chance to open more work buckets. - // Notify one worker so it can open buckets. - let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); - self.scheduler.worker_monitor.1.notify_one(); - } + self.execute_coordinator_work(work.as_mut(), true); false } CoordinatorMessage::Finish => { @@ -101,22 +88,7 @@ impl GCController { /// Coordinate workers to perform GC in response to a GC request. pub fn do_gc_until_completion(&mut self) { // Schedule collection. - // Note: GC workers will start executing work packets as soon as individual work packets - // are added. This means workers may open subsequent buckets while `schedule_collection` - // still has packets to be added to prior buckets. - // We use the`pending_coordinator_packets` to prevent the workers from opening any work - // buckets. - self.scheduler - .pending_coordinator_packets - .fetch_add(1, Ordering::SeqCst); - ScheduleCollection.do_work_with_stat(&mut self.coordinator_worker, self.mmtk); - self.scheduler - .pending_coordinator_packets - .fetch_sub(1, Ordering::SeqCst); - { - let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); - self.scheduler.worker_monitor.1.notify_all(); - } + self.initiate_coordinator_work(&mut ScheduleCollection, true); // Tell GC trigger that GC started - this happens after ScheduleCollection so we // will know what kind of GC this is (e.g. nursery vs mature in gen copy, defrag vs fast in Immix) @@ -145,6 +117,9 @@ impl GCController { } { + // Note: GC workers may spuriously wake up, examining the states of work buckets and + // trying to open them. Use lock to ensure workers do not wake up when we deactivate + // buckets. let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); self.scheduler.deactivate_all(); } @@ -157,8 +132,54 @@ impl GCController { // Otherwise, for generational GCs, workers will receive and process // newly generated remembered-sets from those open buckets. // But these remsets should be preserved until next GC. - EndOfGC.do_work_with_stat(&mut self.coordinator_worker, self.mmtk); + self.initiate_coordinator_work(&mut EndOfGC, false); self.scheduler.debug_assert_all_buckets_deactivated(); } + + /// The controller uses this method to start executing a coordinator work immediately. + /// + /// Note: GC workers will start executing work packets as soon as individual work packets + /// are added. If the coordinator work (such as `ScheduleCollection`) adds multiple work + /// packets into different buckets, workers may open subsequent buckets while the coordinator + /// work still has packets to be added to prior buckets. For this reason, we use the + /// `pending_coordinator_packets` to prevent the workers from opening any work buckets while + /// this coordinator work is being executed. + /// + /// # Arguments + /// + /// - `work`: The work to execute. + /// - `notify_workers`: Notify one worker after the work is finished. Useful for proceeding + /// to the next work bucket stage. + fn initiate_coordinator_work( + &mut self, + work: &mut dyn CoordinatorWork, + notify_workers: bool, + ) { + self.scheduler + .pending_coordinator_packets + .fetch_add(1, Ordering::SeqCst); + + self.execute_coordinator_work(work, notify_workers) + } + + fn execute_coordinator_work( + &mut self, + work: &mut dyn CoordinatorWork, + notify_workers: bool, + ) { + work.do_work_with_stat(&mut self.coordinator_worker, self.mmtk); + + self.scheduler + .pending_coordinator_packets + .fetch_sub(1, Ordering::SeqCst); + + if notify_workers { + // When a coordinator work finishes, there is a chance that all GC workers parked, and + // no work packets are added to any open buckets. We need to wake up one GC worker so + // that it can open more work buckets. + let _guard = self.scheduler.worker_monitor.0.lock().unwrap(); + self.scheduler.worker_monitor.1.notify_one(); + }; + } }