From cbcfc62a6ea3646fb43f2c159cfdc19b3d932004 Mon Sep 17 00:00:00 2001 From: James Munns Date: Thu, 22 Jun 2023 01:38:50 +0200 Subject: [PATCH] Handle Scheduling Edge Cases (#446) This PR fixes two edge cases in scheduling. The first was observed in mnemos, where adding a timer BEFORE the newest deadline would not be represented in the "time to next ticks" turn report, which made mnemos sleep longer than it should have. The second was found while looking for the first (but not observed to cause any problems), where pending ticks (e.g. not advanced through force) that caused a sleep timer to expire would not be counted in the turn report of expired tasks on the next call to force. --- maitake/src/time/timer.rs | 6 +- maitake/src/time/timer/tests/wheel_tests.rs | 92 +++++++++++++++++++++ maitake/src/time/timer/wheel.rs | 11 +++ 3 files changed, 107 insertions(+), 2 deletions(-) diff --git a/maitake/src/time/timer.rs b/maitake/src/time/timer.rs index 83d5f748..80f8bf72 100644 --- a/maitake/src/time/timer.rs +++ b/maitake/src/time/timer.rs @@ -568,13 +568,15 @@ impl Timer { let pending_ticks = self.pending_ticks.swap(0, AcqRel) as Ticks; // we do two separate `advance` calls here instead of advancing once // with the sum, because `ticks` + `pending_ticks` could overflow. + let mut pend_exp = 0; if pending_ticks > 0 { - core.advance(pending_ticks); + let (expired, _next_deadline) = core.advance(pending_ticks); + pend_exp = expired; } let (expired, next_deadline) = core.advance(ticks); Turn { - expired, + expired: expired.saturating_add(pend_exp), next_deadline_ticks: next_deadline.map(|d| d.ticks), now: core.now(), tick_duration: self.tick_duration, diff --git a/maitake/src/time/timer/tests/wheel_tests.rs b/maitake/src/time/timer/tests/wheel_tests.rs index 106a34ac..06c02429 100644 --- a/maitake/src/time/timer/tests/wheel_tests.rs +++ b/maitake/src/time/timer/tests/wheel_tests.rs @@ -183,6 +183,49 @@ impl SleepGroupTest { } } +#[test] +fn pend_advance_wakes() { + static TIMER: Timer = Timer::new(Duration::from_millis(1)); + let mut test = SleepGroupTest::new(&TIMER); + + test.spawn_group(100, 2); + + // first tick --- timer is still at zero + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + test.assert(); + + // advance the timer by 50 ticks. + test.advance(50); + + // advance the timer by 50 more ticks + // but ONLY by pending + test.now += 50; + test.timer.pend_ticks(50); + + // Tick the scheduler, nothing should have happened + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + + // How many do we expect to complete "now"? (it's two) + let expected_complete: usize = test + .groups + .iter_mut() + .take_while(|(&t, _)| t <= test.now) + .map(|(_, g)| std::mem::replace(&mut g.tasks, 0)) + .sum(); + + // Call force, which will "notice" the pending ticks + let turn = test.timer.force_advance_ticks(0); + assert_eq!(turn.expired, expected_complete); + + // NOW the tasks will show up as scheduled, and complete + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, expected_complete); + + test.assert_all_complete(); +} + #[test] fn timer_basically_works() { static TIMER: Timer = Timer::new(Duration::from_millis(1)); @@ -260,6 +303,55 @@ fn schedule_after_start() { test.assert_all_complete(); } +#[test] +fn expired_shows_up() { + static TIMER: Timer = Timer::new(Duration::from_millis(1)); + let mut test = SleepGroupTest::new(&TIMER); + + test.spawn_group(150, 2); + + // first tick --- timer is still at zero + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + test.assert(); + + // advance the timer by 50 ticks. + test.advance(50); + + // Second tick - still nothing + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + test.assert(); + + // Add MORE items, sooner than the previous ones, to force a re-org + test.spawn_group(60, 3); + + // Tick - nothing happens, but the new sleeps should now be registered. + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + test.assert(); + + // advance the timer by 50 more ticks, NOT past our new sleeps, + // but forward + test.now += 50; + let turn = test.timer.force_advance_ticks(50); + assert_eq!(turn.ticks_to_next_deadline(), Some(10)); + + let tick = test.scheduler.tick(); + assert_eq!(tick.completed, 0); + test.assert(); + + // advance the timer by 10 ticks. + test.advance(10); + test.assert(); + + // advance the timer by 40 ticks. + test.advance(40); + test.assert(); + + test.assert_all_complete(); +} + #[test] fn max_sleep() { static TIMER: Timer = Timer::new(Duration::from_millis(1)); diff --git a/maitake/src/time/timer/wheel.rs b/maitake/src/time/timer/wheel.rs index 71074689..fb6813ad 100644 --- a/maitake/src/time/timer/wheel.rs +++ b/maitake/src/time/timer/wheel.rs @@ -165,12 +165,23 @@ impl Core { ?next_deadline, "wheel turned to" ); + + // If we need to reschedule something, we may need to recalculate the next deadline + let any = !pending_reschedule.is_empty(); + for entry in pending_reschedule { let deadline = unsafe { entry.as_ref().deadline }; + debug_assert!(deadline > self.now); debug_assert_ne!(deadline, 0); self.insert_sleep_at(deadline, entry) } + // Yup, we rescheduled something. Recalculate the next deadline in case one of those + // was sooner than the last calculated deadline + if any { + next_deadline = self.next_deadline(); + } + (fired, next_deadline) }