time: rm the STATE_FIRING

Author: wathenjiang

tokio/src/runtime/time/entry.rs

@@ -21,8 +21,8 @@
 //!
 //! Each timer has a state field associated with it. This field contains either
 //! the current scheduled time, or a special flag value indicating its state.
-//! This state can either indicate that the timer is firing (and thus will be fired
-//! with an `Ok(())` result soon) or that it has already been fired/deregistered.
+//! This state can either indicate that whether the timer has already been
+//! fired/deregistered.
 //!
 //! This single state field allows for code that is firing the timer to
 //! synchronize with any racing `reset` calls reliably.
@@ -48,10 +48,10 @@
 //! There is of course a race condition between timer reset and timer
 //! expiration. If the driver fails to observe the updated expiration time, it
 //! could trigger expiration of the timer too early. However, because
-//! [`mark_firing`][mark_firing] performs a compare-and-swap, it will identify this race and
-//! refuse to mark the timer as firing.
+//! [`mark_deregistered`][mark_deregistered] performs a compare-and-swap,
+//! it will identify this race and refuse to mark the timer as deregistered.
 //!
-//! [mark_firing]: TimerHandle::mark_firing
+//! [mark_deregistered]: TimerHandle::mark_deregistered
 
 use crate::loom::cell::UnsafeCell;
 use crate::loom::sync::atomic::AtomicU64;
@@ -70,8 +70,7 @@ use std::{marker::PhantomPinned, pin::Pin, ptr::NonNull};
 type TimerResult = Result<(), crate::time::error::Error>;
 
 const STATE_DEREGISTERED: u64 = u64::MAX;
-const STATE_FIRING: u64 = STATE_DEREGISTERED - 1;
-const STATE_MIN_VALUE: u64 = STATE_FIRING;
+const STATE_MIN_VALUE: u64 = STATE_DEREGISTERED;
 /// The largest safe integer to use for ticks.
 ///
 /// This value should be updated if any other signal values are added above.
@@ -157,13 +156,13 @@ impl StateCell {
         }
     }
 
-    /// Marks this timer firing, if its scheduled time is not after `not_after`.
+    /// Marks this timer deregistered, if its scheduled time is not after `not_after`.
     ///
     /// If the timer is scheduled for a time after `not_after`, returns an Err
     /// containing the current scheduled time.
     ///
     /// SAFETY: Must hold the driver lock.
-    unsafe fn mark_firing(&self, not_after: u64) -> Result<(), u64> {
+    unsafe fn mark_deregistered(&self, not_after: u64) -> Result<(), u64> {
         // Quick initial debug check to see if the timer is already fired. Since
         // firing the timer can only happen with the driver lock held, we know
         // we shouldn't be able to "miss" a transition to a fired state, even
@@ -184,7 +183,7 @@ impl StateCell {
 
             match self.state.compare_exchange_weak(
                 cur_state,
-                STATE_FIRING,
+                STATE_DEREGISTERED,
                 Ordering::AcqRel,
                 Ordering::Acquire,
             ) {
@@ -204,15 +203,6 @@ impl StateCell {
     ///
     /// SAFETY: The driver lock must be held.
     unsafe fn fire(&self, result: TimerResult) -> Option<Waker> {
-        // Quick initial check to see if the timer is already fired. Since
-        // firing the timer can only happen with the driver lock held, we know
-        // we shouldn't be able to "miss" a transition to a fired state, even
-        // with relaxed ordering.
-        let cur_state = self.state.load(Ordering::Relaxed);
-        if cur_state == STATE_DEREGISTERED {
-            return None;
-        }
-
         // SAFETY: We assume the driver lock is held and the timer is not
         // fired, so only the driver is accessing this field.
         //
@@ -247,10 +237,9 @@ impl StateCell {
     fn extend_expiration(&self, new_timestamp: u64) -> Result<(), ()> {
         let mut prior = self.state.load(Ordering::Relaxed);
         loop {
-            if new_timestamp < prior || prior >= STATE_MIN_VALUE {
+            if new_timestamp < prior || prior == STATE_MIN_VALUE {
                 return Err(());
             }
-
             match self.state.compare_exchange_weak(
                 prior,
                 new_timestamp,
@@ -268,7 +257,7 @@ impl StateCell {
     /// ordering, but is conservative - if it returns false, the timer is
     /// definitely _not_ registered.
     pub(super) fn might_be_registered(&self) -> bool {
-        self.state.load(Ordering::Relaxed) != u64::MAX
+        self.state.load(Ordering::Relaxed) != STATE_DEREGISTERED
     }
 }
 
@@ -639,8 +628,8 @@ impl TimerHandle {
     ///
     /// SAFETY: The caller must ensure that the handle remains valid, the driver
     /// lock is held, and that the timer is not in any wheel linked lists.
-    pub(super) unsafe fn mark_firing(&self, not_after: u64) -> Result<(), u64> {
-        match self.inner.as_ref().state.mark_firing(not_after) {
+    pub(super) unsafe fn mark_deregistered(&self, not_after: u64) -> Result<(), u64> {
+        match self.inner.as_ref().state.mark_deregistered(not_after) {
             Ok(()) => {
                 // mark this as being firing in cached_when
                 self.inner.as_ref().set_cached_when(u64::MAX);

tokio/src/runtime/time/mod.rs

@@ -340,7 +340,7 @@ impl Handle {
                 if let Some(entry) = lock.get_mut_entries(&expiration).pop_back() {
                     // Try to expire the entry; this is cheap (doesn't synchronize) if
                     // the timer is not expired, and updates cached_when.
-                    match unsafe { entry.mark_firing(expiration.deadline) } {
+                    match unsafe { entry.mark_deregistered(expiration.deadline) } {
                         Ok(()) => {
                             // Entry was expired.
                             // SAFETY: We hold the driver lock, and just removed the entry from any linked lists.
@@ -390,9 +390,8 @@ impl Handle {
 
             if entry.as_ref().might_be_registered() {
                 lock.remove(entry);
+                entry.as_ref().handle().fire(Ok(()));
             }
-
-            entry.as_ref().handle().fire(Ok(()));
         }
     }