diff --git a/src/cpu/jitv2/codegen.rs b/src/cpu/jitv2/codegen.rs index 345d01180..197d3c872 100644 --- a/src/cpu/jitv2/codegen.rs +++ b/src/cpu/jitv2/codegen.rs @@ -11053,6 +11053,12 @@ mod tests { let pages: [(&str, &[u32]); 5] = [("simple", &simple), ("narrow", &narrow), ("fp", &fp), ("likely", &likely), ("blocks", &blocks)]; + // The two compiles must see the same process-wide switches: other + // tests flip the entry check, interpreter fallback and the enable + // table while they run, each under its own lock. + let _table = crate::cpu::jitv2::opcode_support::table_stable(); + let _fallback = crate::cpu::jitv2::analyzer::FALLBACK_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let _entry = crate::cpu::jitv2::ENTRY_PREAMBLE_TEST_LOCK.lock().unwrap_or_else(|e| e.into_inner()); for (name, words) in pages { let mut page = [0u32; ENTRIES_PER_PAGE]; page[..words.len()].copy_from_slice(words); diff --git a/src/cpu/jitv2/jitv2.rs b/src/cpu/jitv2/jitv2.rs index d5e24d038..6442262fa 100644 --- a/src/cpu/jitv2/jitv2.rs +++ b/src/cpu/jitv2/jitv2.rs @@ -210,16 +210,13 @@ pub const COMPILE_QUEUE_CAPACITY: usize = 2048; /// waits seconds behind that compile's backlog, measured with jitcov on IP28). pub const HOT_PROMOTE_ARRIVALS: u32 = 256; -/// The hot lane: a small second queue workers drain before the FIFO. One per -/// process, like the JIT itself. Requests hold raw page pointers, so -/// `CompileQueue::drain_pending` empties it too before any flush. -pub static HOT_QUEUE: std::sync::LazyLock> = - std::sync::LazyLock::new(|| crossbeam_queue::ArrayQueue::new(256)); +/// Requests the hot lane holds (see `CompileQueue::hot`). +const HOT_QUEUE_CAPACITY: usize = 256; /// Put a copy of a request on the hot lane. A full lane just drops it: the /// page's FIFO request is still queued, so nothing is lost but the head start. -pub fn push_hot_request(req: CompileRequest) { - let _ = HOT_QUEUE.push(req); +pub fn push_hot_request(hot: &crossbeam_queue::ArrayQueue, req: CompileRequest) { + let _ = hot.push(req); } @@ -2383,6 +2380,32 @@ impl Drop for Jitv2 { } impl Jitv2 { + /// Stop the CPUs that run on `jit` and its compile pool for good, before + /// the machine frees the memory and devices their requests point into. + /// A queued request holds its page's generation pointer, into the + /// machine's RAM, and the pool outlives the machine's own drop. + /// + /// Not `CompileQueue::stop` under the lock: a worker leading a flush + /// takes that lock, and restarts the CPUs when the flush is done, so + /// stop both until neither comes back. + pub fn stop_for_drop(jit: &Mutex, stop_cpus: impl Fn()) { + loop { + stop_cpus(); + let workers = { + let mut g = jit.lock(); + g.compile_queue.running.store(false, Ordering::SeqCst); + std::mem::take(&mut g.compile_queue.threads) + }; + if workers.is_empty() { + break; + } + for w in workers { + let _ = w.join(); + } + } + jit.lock().compile_queue.drain_pending_queue(); + } + /// Allocate the full page pool up front, `capacity` slots, every one /// starting unclaimed (`PhysicalCodePage::new(UNCLAIMED_PFN, null)`) — /// see `Jitv2`'s own doc comment for why this is a single one-shot @@ -2812,6 +2835,11 @@ pub struct CompileQueue { /// `push` returning `Err` on full rather than blocking — preserves /// §6.4's "drop on full, hot pages re-trigger" semantics exactly. queue: Arc>, + /// The hot lane: a small second queue the workers drain before `queue`, + /// for pages executed hard while they wait. One per pool, as `queue` is: + /// a request holds raw pointers into its own JIT's pages and its own + /// machine's memory, so another machine's worker must never take it. + hot: Arc>, running: Arc, /// One `JoinHandle` per worker thread — empty when not running. Each /// thread hands back its own `Codegen` on exit (see `worker_loop`'s @@ -2897,6 +2925,7 @@ impl CompileQueue { Self { mips4: crate::cpu::jitv2::isa::mips4_enabled(), queue: Arc::new(crossbeam_queue::ArrayQueue::new(COMPILE_QUEUE_CAPACITY)), + hot: Arc::new(crossbeam_queue::ArrayQueue::new(HOT_QUEUE_CAPACITY)), running: Arc::new(AtomicBool::new(false)), threads: Vec::new(), cpu: Mutex::new(None), @@ -2998,6 +3027,11 @@ impl CompileQueue { self.queue.clone() } + /// The hot lane, as [`Self::queue_handle`] gives the FIFO. + pub fn hot_handle(&self) -> Arc> { + self.hot.clone() + } + /// Inject the CPU device handle — see the `cpu` field's doc comment for /// why this is a separate setter rather than a constructor parameter. /// Safe to call at any time (even while the worker is running); takes @@ -3072,11 +3106,11 @@ impl CompileQueue { /// after. `req.page` is still safely dereferenceable here — the pool /// hasn't been cleared yet at either call site (see this function's own /// doc comment above for exactly why that ordering is guaranteed). - fn drain_pending(queue: &crossbeam_queue::ArrayQueue) { + fn drain_pending(queue: &crossbeam_queue::ArrayQueue, hot: &crossbeam_queue::ArrayQueue) { while let Some(req) = queue.pop() { unsafe { (*req.page).clear_scheduled(); } } - while let Some(req) = HOT_QUEUE.pop() { + while let Some(req) = hot.pop() { unsafe { (*req.page).clear_scheduled(); } } } @@ -3086,7 +3120,7 @@ impl CompileQueue { /// `drain_pending`'s own doc comment for why this must run and when /// it's safe to call (no worker concurrently popping). pub fn drain_pending_queue(&mut self) { - Self::drain_pending(&self.queue); + Self::drain_pending(&self.queue, &self.hot); } /// Spawn the compile-thread pool with one freshly-reserved shared arena @@ -3177,6 +3211,7 @@ impl CompileQueue { for i in 0..self.thread_count { let codegen = crate::cpu::jitv2::codegen::Codegen::new_with_shared_arena(shared.clone(), state.clone()); let queue = self.queue.clone(); + let hot = self.hot.clone(); let running = self.running.clone(); let bus = bus.clone(); let cpu = self.cpu.lock().clone(); @@ -3190,7 +3225,7 @@ impl CompileQueue { self.threads.push( std::thread::Builder::new() .name(format!("jitv2-compile-{i}")) - .spawn(move || Self::worker_loop(queue, running, bus, codegen, cpu, world, jitv2, function_count, stats, barrier, quiesce_in_progress, thread_count, worker_mips4)) + .spawn(move || Self::worker_loop(queue, hot, running, bus, codegen, cpu, world, jitv2, function_count, stats, barrier, quiesce_in_progress, thread_count, worker_mips4)) .expect("jitv2-compile spawn"), ); } @@ -3363,6 +3398,7 @@ impl CompileQueue { /// worker up front and simply left running/idle after they all exit). fn worker_loop( queue: Arc>, + hot: Arc>, running: Arc, bus: Arc, mut codegen: crate::cpu::jitv2::codegen::Codegen, @@ -3544,7 +3580,7 @@ impl CompileQueue { // crash this was confirmed to cause). Safe to drain once // here even though the queue is shared with every other // (already-parked, not popping) worker. - Self::drain_pending(&queue); + Self::drain_pending(&queue, &hot); unsafe { jit.lock().flush_from_jit_thread(codegen.function_count()); } // Build ONE fresh shared arena and rebuild this (leader's // own) Codegen on top of it — every other, parked worker @@ -3673,8 +3709,8 @@ impl CompileQueue { continue; } // The hot lane first: pages being executed right now that are - // still waiting (see `HOT_QUEUE`). - match HOT_QUEUE.pop().or_else(|| queue.pop()) { + // still waiting (see `CompileQueue::hot`). + match hot.pop().or_else(|| queue.pop()) { Some(req) => { // Always the deferred/non-forced path — never // handle_request's forced-seal one. Forced sealing @@ -5563,7 +5599,9 @@ mod tests { // function with worker threads still live against `pages` — see // the page-cross test's own comment for the confirmed // use-after-free this guards. - let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); + // Well under a second alone; with a test on every core the four + // workers can wait a long time for one. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(120); let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { loop { let published = pages.iter().filter(|p| p.is_runnable(0)).count(); @@ -5591,9 +5629,11 @@ mod tests { #[test] fn compile_queue_send_drops_when_full() { + // Declared before the queue so it outlives it: dropping the queue + // drains the requests, and each clears its page's scheduled flag. + let mut page = PhysicalCodePage::new(0, std::ptr::null()); let q = CompileQueue::new(); // Don't start the worker: nothing drains, so capacity fills exactly. - let mut page = PhysicalCodePage::new(0, std::ptr::null()); let stats = JitStats::default(); let mut accepted = 0; for i in 0..(COMPILE_QUEUE_CAPACITY + 10) { @@ -5627,6 +5667,22 @@ mod tests { thinking a compile is still in flight for it forever"); } + #[test] + fn a_hot_request_stays_with_its_own_pool() { + // A request points into its own JIT's pages: another machine's + // pool, which may outlive that JIT, must never see it. + let mut page = PhysicalCodePage::new(0, std::ptr::null()); + assert!(page.try_schedule_page()); + let mut ours = CompileQueue::new(); + let mut theirs = CompileQueue::new(); + push_hot_request(&ours.hot_handle(), CompileRequest { page: &mut page as *mut PhysicalCodePage, compiled_for_fr1: true }); + theirs.drain_pending_queue(); + assert!(page.is_scheduled(), "another pool's flush discarded this pool's hot request"); + assert_eq!(theirs.hot_handle().len(), 0); + ours.drain_pending_queue(); + assert!(!page.is_scheduled()); + } + #[test] fn compile_queue_start_is_idempotent() { let dev: Arc = Arc::new(FakeDevice(AtomicU64::new(0))); diff --git a/src/cpu/mips_exec.rs b/src/cpu/mips_exec.rs index f99d5c7bb..cbc86ebcc 100644 --- a/src/cpu/mips_exec.rs +++ b/src/cpu/mips_exec.rs @@ -913,6 +913,10 @@ pub struct MipsExecutor { /// every dispatch. #[cfg(feature = "jitv2")] pub jitv2_compile_queue_handle: std::sync::Arc>, + /// The same pool's hot lane (`CompileQueue::hot_handle`), held for the + /// same reason. + #[cfg(feature = "jitv2")] + pub jitv2_hot_queue_handle: std::sync::Arc>, /// Cheap handle to `jitv2.lock().stats`, cloned once at construction — /// same reasoning as `jitv2_compile_queue_handle`: `JitStats` is already /// `Arc`-shared internally (see `Jitv2::stats`'s own doc comment), so @@ -2346,6 +2350,8 @@ impl MipsExecutor { #[cfg(feature = "jitv2")] let jitv2_compile_queue_handle = jitv2.compile_queue.queue_handle(); #[cfg(feature = "jitv2")] + let jitv2_hot_queue_handle = jitv2.compile_queue.hot_handle(); + #[cfg(feature = "jitv2")] let jitv2_stats = jitv2.stats.clone(); let mut executor = Self { @@ -2425,6 +2431,8 @@ impl MipsExecutor { #[cfg(feature = "jitv2")] jitv2_compile_queue_handle, #[cfg(feature = "jitv2")] + jitv2_hot_queue_handle, + #[cfg(feature = "jitv2")] jitv2_stats, #[cfg(feature = "jitv2")] pcp: std::ptr::null_mut(), @@ -3066,6 +3074,7 @@ va={:#018x} phys={:#010x} (code pfn {:#x}, page {:#010x}, word {}/{})", { let j = jit.lock(); self.jitv2_compile_queue_handle = j.compile_queue.queue_handle(); + self.jitv2_hot_queue_handle = j.compile_queue.hot_handle(); self.jitv2_stats = j.stats.clone(); } self.jitv2 = jit; @@ -3726,7 +3735,7 @@ va={:#018x} phys={:#010x} (code pfn {:#x}, page {:#010x}, word {}/{})", // Already queued, and being executed hard while it // waits: jump the queue (jitv2::HOT_QUEUE). crate::pf_instant!(crate::perfetto::events::JIT_HOT_REQUEST, pc); - crate::cpu::jitv2::jitv2::push_hot_request(req); + crate::cpu::jitv2::jitv2::push_hot_request(&self.jitv2_hot_queue_handle, req); } break 'gate self.step_int(); } @@ -10202,9 +10211,7 @@ impl MipsCpu { pub fn register_locks(&self) { - use crate::locks::register_lock_fn; - let ex = self.executor.clone(); - register_lock_fn("cpu::executor", move || ex.is_locked()); + crate::locks::register_mutex("cpu::executor", &self.executor); } fn try_lock_executor(&self) -> Result>, String> { diff --git a/src/dev/hal2.rs b/src/dev/hal2.rs index d67331d2c..ca03a48bd 100644 --- a/src/dev/hal2.rs +++ b/src/dev/hal2.rs @@ -1019,12 +1019,12 @@ impl Hal2 { } pub fn register_locks(self: &Arc) { - use crate::locks::register_lock_fn; - let me = self.clone(); register_lock_fn("hal2::state", move || me.state.is_locked()); - let me = self.clone(); register_lock_fn("hal2::ca_state", move || me.ca_state.is_locked()); - let me = self.clone(); register_lock_fn("hal2::cb_state", move || me.cb_state.is_locked()); - let me = self.clone(); register_lock_fn("hal2::at_state", move || me.at_state.is_locked()); - let me = self.clone(); register_lock_fn("hal2::ar_state", move || me.ar_state.is_locked()); + use crate::locks::register_mutex_fn; + register_mutex_fn("hal2::state", self, |h| h.state.is_locked()); + register_mutex_fn("hal2::ca_state", self, |h| h.ca_state.is_locked()); + register_mutex_fn("hal2::cb_state", self, |h| h.cb_state.is_locked()); + register_mutex_fn("hal2::at_state", self, |h| h.at_state.is_locked()); + register_mutex_fn("hal2::ar_state", self, |h| h.ar_state.is_locked()); } } diff --git a/src/dev/hpc3.rs b/src/dev/hpc3.rs index ebb0467d0..a07621851 100644 --- a/src/dev/hpc3.rs +++ b/src/dev/hpc3.rs @@ -1400,15 +1400,23 @@ impl Hpc3 { self.scsi_dev1.as_ref() } + /// Let go of the bus and of the interrupt wiring that loops back through + /// the SCSI channels (each channel holds the line that holds both), so a + /// dropped machine's memory can be freed. The machine is stopped. + pub fn release(&self) { + for chan in &self.pdma_channels { + let mut chan = chan.lock(); + chan.sys_mem = None; + chan.irq = None; + } + } + pub fn register_locks(&self) { - use crate::locks::register_lock_fn; - let state = self.state.clone(); - register_lock_fn("hpc3::state", move || state.is_locked()); - let eeprom = self.eeprom.clone(); - register_lock_fn("hpc3::eeprom", move || eeprom.is_locked()); + use crate::locks::register_mutex; + register_mutex("hpc3::state", &self.state); + register_mutex("hpc3::eeprom", &self.eeprom); for (i, chan) in self.pdma_channels.iter().enumerate() { - let chan = chan.clone(); - register_lock_fn(format!("hpc3::pdma_channels[{}]", i), move || chan.is_locked()); + register_mutex(format!("hpc3::pdma_channels[{}]", i), chan); } // Delegate to child components self.seeq.register_locks(); diff --git a/src/dev/ioc.rs b/src/dev/ioc.rs index 306789fac..e770a2710 100644 --- a/src/dev/ioc.rs +++ b/src/dev/ioc.rs @@ -677,14 +677,12 @@ impl Ioc { } pub fn register_locks(&self) { - use crate::locks::register_lock_fn; - let s = self.state.clone(); - register_lock_fn("ioc::state", move || s.is_locked()); + use crate::locks::{register_mutex, register_mutex_fn}; + register_mutex("ioc::state", &self.state); // SCC (Z85c30) channels self.scc.register_locks(); // PS/2 - let ps2 = self.ps2.clone(); - register_lock_fn("ps2::state", move || ps2.is_state_locked()); + register_mutex_fn("ps2::state", &self.ps2, |p| p.is_state_locked()); } } diff --git a/src/dev/ioc3_eth/tests.rs b/src/dev/ioc3_eth/tests.rs index 64a54bb4f..12186d75a 100644 --- a/src/dev/ioc3_eth/tests.rs +++ b/src/dev/ioc3_eth/tests.rs @@ -759,7 +759,9 @@ fn a_frame_waits_for_the_negotiation_deadline_without_a_wake() { } fn waits_for(cond: impl Fn() -> bool) -> bool { - let end = Instant::now() + Duration::from_secs(2); + // Generous: it returns as soon as `cond` holds, and the receive thread + // can wait a long time for a core when every test is running. + let end = Instant::now() + Duration::from_secs(30); while Instant::now() < end { if cond() { return true; diff --git a/src/dev/isp1040/tests.rs b/src/dev/isp1040/tests.rs index 8a96b9d66..f911743fd 100644 --- a/src/dev/isp1040/tests.rs +++ b/src/dev/isp1040/tests.rs @@ -731,7 +731,9 @@ fn every_move_of_the_test_clock_reaches_a_waiting_selection() { } let _ = tx.send(()); }); - match rx.recv_timeout(Duration::from_secs(5)) { + // A lost move hangs the loop for good, so the bound only has to be + // longer than 2000 handoffs take when the threads wait for a core. + match rx.recv_timeout(Duration::from_secs(60)) { Ok(()) => {} Err(std::sync::mpsc::RecvTimeoutError::Timeout) => panic!("a clock move was lost: the selection never ended"), Err(e) => panic!("the clock loop failed: {e}"), diff --git a/src/dev/mc.rs b/src/dev/mc.rs index 8f11625ad..620524edf 100644 --- a/src/dev/mc.rs +++ b/src/dev/mc.rs @@ -563,18 +563,14 @@ impl MemoryController { pub(crate) fn vdma_stage(&self) -> &Mutex> { &self.vdma_stage } pub fn register_locks(&self) { - use crate::locks::register_lock_fn; - let giodma = self.giodma.clone(); - register_lock_fn("mc::giodma_state", move || giodma.state.is_locked()); - let state = self.state.clone(); - register_lock_fn("mc::state", move || state.is_locked()); - let threads = self.threads.clone(); - register_lock_fn("mc::threads", move || threads.is_locked()); + use crate::locks::{register_mutex, register_mutex_fn}; + register_mutex_fn("mc::giodma_state", &self.giodma, |g| g.state.is_locked()); + register_mutex("mc::state", &self.state); + register_mutex("mc::threads", &self.threads); // Held across REX3 bus calls (which take REX3's own locks and can // wait_idle), so it must be visible to the deadlock detector even // though only the MC-DMA thread ever acquires it. - let stage = self.vdma_stage.clone(); - register_lock_fn("mc::vdma_stage", move || stage.is_locked()); + register_mutex("mc::vdma_stage", &self.vdma_stage); } } diff --git a/src/dev/mgras/hq4.rs b/src/dev/mgras/hq4.rs index ed12da5f3..387ea5856 100644 --- a/src/dev/mgras/hq4.rs +++ b/src/dev/mgras/hq4.rs @@ -1890,10 +1890,11 @@ mod tests { // Still full, still busy: nothing was consumed or lost silently. assert_eq!(r.b.write(SH + hq4::RE_STATUS as u64, 32, 0), BUS_BUSY); r.b.mgras().start_engines(); - let mut tries = 0u32; + // A deadline, not a count of retries: the engine thread may wait a + // long time for a core when every test is running. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(30); while r.b.write(SH + hq4::RE_STATUS as u64, 32, 0) == BUS_BUSY { - tries += 1; - assert!(tries < 10_000_000, "the store never landed after the engine drained"); + assert!(std::time::Instant::now() < deadline, "the store never landed after the engine drained"); std::thread::yield_now(); } } diff --git a/src/dev/ng1/rex3.rs b/src/dev/ng1/rex3.rs index 8330da248..bd2ce1509 100644 --- a/src/dev/ng1/rex3.rs +++ b/src/dev/ng1/rex3.rs @@ -3810,29 +3810,18 @@ impl Rex3 { } pub fn register_locks(self: &Arc) { - use crate::locks::register_lock_fn; - let r = self.clone(); - register_lock_fn("rex3::dcb", move || r.dcb.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::vc2", move || r.vc2.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::xmap0", move || r.xmap0.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::xmap1", move || r.xmap1.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::cmap0", move || r.cmap0.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::cmap1", move || r.cmap1.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::processor_thread", move || r.processor_thread.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::refresh_thread", move || r.refresh_thread.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::vblank_cb", move || r.vblank_cb.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::renderer", move || r.renderer.is_locked()); - let r = self.clone(); - register_lock_fn("rex3::debug_state", move || r.debug_state.is_locked()); + use crate::locks::register_mutex_fn; + register_mutex_fn("rex3::dcb", self, |r| r.dcb.is_locked()); + register_mutex_fn("rex3::vc2", self, |r| r.vc2.is_locked()); + register_mutex_fn("rex3::xmap0", self, |r| r.xmap0.is_locked()); + register_mutex_fn("rex3::xmap1", self, |r| r.xmap1.is_locked()); + register_mutex_fn("rex3::cmap0", self, |r| r.cmap0.is_locked()); + register_mutex_fn("rex3::cmap1", self, |r| r.cmap1.is_locked()); + register_mutex_fn("rex3::processor_thread", self, |r| r.processor_thread.is_locked()); + register_mutex_fn("rex3::refresh_thread", self, |r| r.refresh_thread.is_locked()); + register_mutex_fn("rex3::vblank_cb", self, |r| r.vblank_cb.is_locked()); + register_mutex_fn("rex3::renderer", self, |r| r.renderer.is_locked()); + register_mutex_fn("rex3::debug_state", self, |r| r.debug_state.is_locked()); crate::locks::register_mutex("rex3::screen", &self.screen); } } diff --git a/src/dev/o2gfx.rs b/src/dev/o2gfx.rs index fe2f90035..276341ee2 100644 --- a/src/dev/o2gfx.rs +++ b/src/dev/o2gfx.rs @@ -1181,7 +1181,11 @@ pub struct CrimeRe { polled_busy: std::sync::atomic::AtomicBool, asleep: std::sync::atomic::AtomicBool, started: std::sync::atomic::AtomicBool, + /// Set by [`Self::stop`]: the engine's thread ends once the buffer is + /// empty. + quit: std::sync::atomic::AtomicBool, engine: std::sync::OnceLock, + worker: Mutex>>, /// The annotated trace, shared with CRIME and GBE. pub tr: Arc, /// `re rec`: interface buffer entries as text the replay test reads. @@ -1281,7 +1285,9 @@ impl CrimeRe { polled_busy: std::sync::atomic::AtomicBool::new(false), asleep: std::sync::atomic::AtomicBool::new(false), started: std::sync::atomic::AtomicBool::new(false), + quit: std::sync::atomic::AtomicBool::new(false), engine: std::sync::OnceLock::new(), + worker: Mutex::new(None), tr: Arc::new(O2Trace::default()), rec: Mutex::new(None), rec_on: std::sync::atomic::AtomicBool::new(false), @@ -1303,7 +1309,7 @@ impl CrimeRe { if self.started.swap(true, std::sync::atomic::Ordering::AcqRel) { return; } - std::thread::Builder::new() + let worker = std::thread::Builder::new() .name("CRIME-RE".into()) .spawn(move || { let re = get(&owner); @@ -1311,6 +1317,17 @@ impl CrimeRe { re.run(); }) .expect("CRIME-RE thread"); + *self.worker.lock().unwrap() = Some(worker); + } + + /// End the engine's thread once it has carried out what is queued, and + /// let go of the owner [`Self::start`] gave it: the thread holds the + /// machine's bus, which is not freed while it runs. + pub fn stop(&self) { + let Some(worker) = self.worker.lock().unwrap().take() else { return }; + self.quit.store(true, std::sync::atomic::Ordering::SeqCst); + worker.thread().unpark(); + let _ = worker.join(); } /// The engine's thread: carry out each write in order. With none left @@ -1320,7 +1337,7 @@ impl CrimeRe { /// backs off between looks: looking without pause took the buffer's /// tail away from the CPU on every write and lost a fifth of small /// triangles. - fn run(&self) -> ! { + fn run(&self) { use std::sync::atomic::Ordering::*; const IDLE_SPIN: std::time::Duration = std::time::Duration::from_micros(250); let backoff = crossbeam_utils::Backoff::new(); @@ -1336,6 +1353,9 @@ impl CrimeRe { } // Stays set across sleeps, so an idle engine wakes only for // the park backstop and goes straight back to sleep. + if self.quit.load(Relaxed) { + return; + } let since = *idle_since.get_or_insert_with(std::time::Instant::now); if since.elapsed() < IDLE_SPIN { backoff.spin(); diff --git a/src/dev/odyssey/tests/fifo.rs b/src/dev/odyssey/tests/fifo.rs index cfd2a9e83..8e90e12eb 100644 --- a/src/dev/odyssey/tests/fifo.rs +++ b/src/dev/odyssey/tests/fifo.rs @@ -951,7 +951,9 @@ fn a_refused_command_push_retries_behind_the_board_thread() { let (h, _rec) = threaded_board(); let words = preamble_twice(); - start_long_slice(&h, &words, 1000); + // The ring must fill and the retry be seen waiting while this one slice + // holds the board, however long the test's threads wait for a core. + start_long_slice(&h, &words, 4000); while h.fifo.stores() < RING_STORES { assert!(h.bus_write(consts::cmd::FIFO, 0, 4)); } diff --git a/src/dev/odyssey/tests/raster.rs b/src/dev/odyssey/tests/raster.rs index d8f695824..2210d0394 100644 --- a/src/dev/odyssey/tests/raster.rs +++ b/src/dev/odyssey/tests/raster.rs @@ -674,7 +674,9 @@ fn stop_releases_a_waiting_front_end() { if super::gpu_unit_tests() { return; } - let (h, _st) = slow_raster(1500); + // A slice long enough that a release by the stop, even one whose threads + // wait for a core, cannot be taken for a release by the slice. + let (h, _st) = slow_raster(4000); let ran = std::sync::Arc::new(Mutex::new(Vec::new())); let job = |i: u32| { let ran = ran.clone(); @@ -693,7 +695,7 @@ fn stop_releases_a_waiting_front_end() { assert!(!waiter.is_finished(), "the front end waits for room"); let stopper = sc.spawn(|| h.raster.stop()); let waited = waiter.join().unwrap(); - assert!(waited < Duration::from_millis(1000), "released by the stop after {waited:?}, not by the slice"); + assert!(waited < Duration::from_millis(3000), "released by the stop after {waited:?}, not by the slice"); stopper.join().unwrap(); }); assert_eq!(*ran.lock(), vec![0, 1]); diff --git a/src/dev/seeq8003.rs b/src/dev/seeq8003.rs index fbaedeb87..ff30a6dc3 100644 --- a/src/dev/seeq8003.rs +++ b/src/dev/seeq8003.rs @@ -679,10 +679,10 @@ impl Seeq8003 { } pub fn register_locks(self: &Arc) { - use crate::locks::register_lock_fn; - let me = self.clone(); register_lock_fn("seeq::state", move || me.state.is_locked()); - let me = self.clone(); register_lock_fn("seeq::net", move || me.net.is_locked()); - let me = self.clone(); register_lock_fn("seeq::nat_ctl::snapshot", move || me.net.try_lock().map_or(false, |n| n.nat.snapshot.is_locked())); + use crate::locks::register_mutex_fn; + register_mutex_fn("seeq::state", self, |s| s.state.is_locked()); + register_mutex_fn("seeq::net", self, |s| s.net.is_locked()); + register_mutex_fn("seeq::nat_ctl::snapshot", self, |s| s.net.try_lock().map_or(false, |n| n.nat.snapshot.is_locked())); } } diff --git a/src/dev/wd33c93a.rs b/src/dev/wd33c93a.rs index e9dd5657a..4ad0d8550 100644 --- a/src/dev/wd33c93a.rs +++ b/src/dev/wd33c93a.rs @@ -803,9 +803,9 @@ impl Wd33c93a { } pub fn register_locks(self: &Arc) { - use crate::locks::register_lock_fn; - let me = self.clone(); register_lock_fn("scsi::state", move || me.state.is_locked()); - let me = self.clone(); register_lock_fn("scsi::thread", move || me.thread.is_locked()); + use crate::locks::register_mutex_fn; + register_mutex_fn("scsi::state", self, |s| s.state.is_locked()); + register_mutex_fn("scsi::thread", self, |s| s.thread.is_locked()); } } diff --git a/src/dev/z85c30.rs b/src/dev/z85c30.rs index c5639afec..b93590744 100644 --- a/src/dev/z85c30.rs +++ b/src/dev/z85c30.rs @@ -546,13 +546,10 @@ impl Z85c30 { BUS_OK } pub fn register_locks(&self) { - use crate::locks::register_lock_fn; - let ch = self.channel_a.clone(); - register_lock_fn("scc::channel_a", move || ch.0.is_locked()); - let ch = self.channel_b.clone(); - register_lock_fn("scc::channel_b", move || ch.0.is_locked()); - let th = self.threads.clone(); - register_lock_fn("scc::threads", move || th.is_locked()); + use crate::locks::{register_mutex, register_mutex_fn}; + register_mutex_fn("scc::channel_a", &self.channel_a, |c| c.0.is_locked()); + register_mutex_fn("scc::channel_b", &self.channel_b, |c| c.0.is_locked()); + register_mutex("scc::threads", &self.threads); } } diff --git a/src/ip30_machine.rs b/src/ip30_machine.rs index 9bf60af8b..4258b0d90 100644 --- a/src/ip30_machine.rs +++ b/src/ip30_machine.rs @@ -982,6 +982,11 @@ impl Drop for OctaneMachine { slot.lock().0 = MachineRef::null(); } self.stop_cpus(); + // The compile pool's requests point into the memory freed below. + #[cfg(feature = "jitv2")] + for c in &self.cpus { + crate::cpu::jitv2::Jitv2::stop_for_drop(&c.jitv2(), || self.stop_cpus()); + } self.stop_devices(); crate::workstation::flush_on_drop(self); self.monitor.shutdown(); diff --git a/src/ip32_machine.rs b/src/ip32_machine.rs index 092f20e01..f78019b8b 100644 --- a/src/ip32_machine.rs +++ b/src/ip32_machine.rs @@ -286,8 +286,6 @@ impl O2Machine { drop(g); crate::cpu::jitv2::jitv2::start_compiling(&*cpu, &jit, cfg.jitv2.compile, bus.phys.clone() as Arc); } - // The compile pool keeps only a weak handle on the CPU. - std::mem::forget(cpu_device); } // CRIME's output is the CPU's IP2, pushed the moment it changes. @@ -756,6 +754,11 @@ impl Drop for O2Machine { if let Some(slot) = self.controller_slot.take() { slot.lock().0 = MachineRef::null(); } + // The CPU and its compile pool first: they reach the bus, which is + // freed once this returns. + self.cpu.clone().as_device().stop(); + #[cfg(feature = "jitv2")] + crate::cpu::jitv2::Jitv2::stop_for_drop(&self.cpu.jitv2(), || self.cpu.clone().as_device().stop()); self.display.display_stop.request(); self.running.store(false, Ordering::SeqCst); for t in self.threads.drain(..) { @@ -763,6 +766,8 @@ impl Drop for O2Machine { } crate::workstation::flush_on_drop(self); self.monitor.shutdown(); + // The rendering engine's thread runs from construction and holds the bus. + self.bus.crime.re.stop(); } } @@ -1192,3 +1197,23 @@ fn control_command(bus: &Ip32Bus, cmd: &str, arg: &str, rgba: &mut [u8]) -> Stri } } + +#[cfg(test)] +mod drop_tests { + use super::*; + use crate::config::{GraphicsBoard, MachineProfile}; + use crate::display_stop_tests::{big_stack, config, scratch}; + + /// A dropped O2 frees its bus and with it the decode table: a process may + /// have only 64, and a test binary builds many machines. + #[test] + fn a_dropped_o2_frees_its_bus_and_decode_table() { + let dir = scratch("o2-drop"); + let cfg = config(&dir, MachineProfile::O2Ip32, GraphicsBoard::Newport); + let machine = big_stack(move || O2Machine::new(&cfg).expect("O2")); + let phys = Arc::downgrade(&machine.bus.phys); + drop(machine); + assert_eq!(phys.strong_count(), 0, "the bus outlives its machine"); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/src/machine.rs b/src/machine.rs index 55d678436..d63a74731 100644 --- a/src/machine.rs +++ b/src/machine.rs @@ -941,9 +941,7 @@ impl Machine { // Register lock monitor device and all component locks { - use crate::locks::register_lock_fn; - let ep = eeprom_mc.clone(); - register_lock_fn("mc::eeprom", move || ep.is_locked()); + crate::locks::register_mutex("mc::eeprom", &eeprom_mc); mc.register_locks(); hpc3.register_locks(); if let Some(rex3) = &board.rex3 { rex3.register_locks(); } @@ -2090,10 +2088,21 @@ impl Drop for Machine { // The guest clock points into this Machine's CPU core. #[cfg(feature = "perfetto")] crate::perfetto::set_guest_cycles(None); + // The CPU, its compile pool and the device threads first: they reach + // the core, the bus and the devices, which are freed once this returns. + self.stop(); + #[cfg(feature = "jitv2")] + crate::cpu::jitv2::Jitv2::stop_for_drop(&self.cpu.jitv2(), || self.cpu.clone().as_device().stop()); crate::workstation::flush_on_drop(self); // Release the monitor port and this Machine's device handles, so the next // Machine in the same process (iris-gui Stop -> Start) gets the monitor. self.monitor.shutdown(); + // VINO's DMA thread runs from construction. It holds the bus. + self.board.vino.stop(); + self.hpc3.release(); + // The bus holds the devices and they hold the bus (DMA): let go, or + // none of it is freed. + self.board.phys.release_devices(); } } @@ -2202,6 +2211,34 @@ mod emulator_name_tests { } } +/// A dropped machine gives back its decode table: a process may have only 64, +/// and a test binary builds many machines. +#[cfg(test)] +mod drop_tests { + use super::*; + use crate::config::{GraphicsBoard, MachineProfile}; + use crate::display_stop_tests::{big_stack, config, scratch}; + + #[test] + fn a_dropped_machine_frees_its_bus_and_decode_table() { + let dir = scratch("ip22-drop"); + for (profile, board) in [ + (MachineProfile::IndyIp24, GraphicsBoard::Newport), + (MachineProfile::Indigo2Ip22, GraphicsBoard::Xz), + (MachineProfile::Indigo2Ip28, GraphicsBoard::SolidImpact), + ] { + let cfg = config(&dir, profile, board); + let machine = big_stack(move || Machine::new(cfg)); + let bus = Arc::downgrade(&machine.board.phys); + let cpu = Arc::downgrade(&machine.cpu); + drop(machine); + assert_eq!(bus.strong_count(), 0, "{profile:?} with {board:?}: the bus outlives its machine"); + assert_eq!(cpu.strong_count(), 0, "{profile:?} with {board:?}: the CPU outlives its machine"); + } + let _ = std::fs::remove_dir_all(&dir); + } +} + #[cfg(test)] mod controller_lifetime_tests { use super::*; diff --git a/src/ppmem/devmap.rs b/src/ppmem/devmap.rs index a81a068b6..440bb364d 100644 --- a/src/ppmem/devmap.rs +++ b/src/ppmem/devmap.rs @@ -654,12 +654,16 @@ fn fault_message(what: &[u8], phys: u64) { mod fault { use super::Inner; use std::io; - use std::sync::atomic::{AtomicPtr, Ordering}; + use std::sync::atomic::{AtomicPtr, AtomicUsize, Ordering}; use std::sync::Once; // Generous: a test binary builds many machines at once, each with a table. const MAX_MAPS: usize = 64; static REGISTRY: [AtomicPtr; MAX_MAPS] = [const { AtomicPtr::new(std::ptr::null_mut()) }; MAX_MAPS]; + /// Fault handlers looking at each slot's table. A fault in one table + /// reads every other table's bounds, so a table being dropped waits for + /// these before it is freed. + static READERS: [AtomicUsize; MAX_MAPS] = [const { AtomicUsize::new(0) }; MAX_MAPS]; static INSTALL: Once = Once::new(); pub(super) fn register(inner: *const Inner) -> io::Result { @@ -676,17 +680,26 @@ mod fault { pub(super) fn unregister(i: usize) { if i < MAX_MAPS { - REGISTRY[i].store(std::ptr::null_mut(), Ordering::Release); + // SeqCst with `dispatch`'s: a handler that still saw the table + // has raised its count by now. + REGISTRY[i].store(std::ptr::null_mut(), Ordering::SeqCst); + while READERS[i].load(Ordering::SeqCst) != 0 { + std::hint::spin_loop(); + } } } /// Offer a faulting host address to every live table. pub(super) fn dispatch(addr: usize) -> bool { - REGISTRY.iter().any(|slot| { - let p = slot.load(Ordering::Acquire); - // SAFETY: a registered table stays alive until it unregisters in - // `Drop`, and nothing dispatches through a table being dropped. - !p.is_null() && unsafe { (*p).fault(addr) } + REGISTRY.iter().zip(&READERS).any(|(slot, readers)| { + readers.fetch_add(1, Ordering::SeqCst); + let p = slot.load(Ordering::SeqCst); + // SAFETY: a table that unregisters waits for `readers` to fall to + // zero before it is freed, and nothing dispatches through its + // own table while it is being dropped. + let ours = !p.is_null() && unsafe { (*p).fault(addr) }; + readers.fetch_sub(1, Ordering::Release); + ours }) }