Address comments

crossbeam-utils/src/atomic/atomic_cell.rs

@@ -666,7 +666,7 @@ fn lock(addr: usize) -> &'static SeqLock {
     // In order to protect from such cases, we simply choose a large prime number for `LEN`.
     const LEN: usize = 97;
 
-    const L: SeqLock = SeqLock::new();
+    const L: SeqLock = SeqLock::INIT;
 
     static LOCKS: [SeqLock; LEN] = [
         L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L, L,

crossbeam-utils/src/atomic/mod.rs

@@ -1,9 +1,16 @@
 //! Atomic types.
 
 cfg_if! {
-    // TODO(jeehoonkang): want to express `target_pointer_width >= "64"`, but it's not expressible
-    // in Rust for the time being.
-    if #[cfg(target_pointer_width = "64")] {
+    // Use "wide" sequence lock if the pointer width <= 32 for preventing its counter against wrap
+    // around.
+    //
+    // We are ignoring too wide architectures (pointer width >= 256), since such a system will not
+    // appear in a conceivable future.
+    //
+    // In narrow architectures (pointer width <= 16), the counter is still <= 32-bit and may be
+    // vulnerable to wrap around. But it's mostly okay, since in such a primitive hardware, the
+    // counter will not be increased that fast.
+    if #[cfg(any(target_pointer_width = "64", target_pointer_width = "128"))] {
         mod seq_lock;
     } else {
         #[path = "seq_lock_wide.rs"]

crossbeam-utils/src/atomic/seq_lock.rs

@@ -12,11 +12,9 @@ pub struct SeqLock {
 }
 
 impl SeqLock {
-    pub const fn new() -> Self {
-        Self {
-            state: AtomicUsize::new(0),
-        }
-    }
+    pub const INIT: Self = Self {
+        state: AtomicUsize::new(0),
+    };
 
     /// If not locked, returns the current stamp.
     ///
@@ -62,7 +60,7 @@ impl SeqLock {
     }
 }
 
-/// A RAII guard that releases the lock and increments the stamp when dropped.
+/// An RAII guard that releases the lock and increments the stamp when dropped.
 pub struct SeqLockWriteGuard {
     /// The parent lock.
     lock: &'static SeqLock,

crossbeam-utils/src/atomic/seq_lock_wide.rs

@@ -18,18 +18,22 @@ pub struct SeqLock {
 }
 
 impl SeqLock {
-    pub const fn new() -> Self {
-        Self {
-            state_hi: AtomicUsize::new(0),
-            state_lo: AtomicUsize::new(0),
-        }
-    }
+    pub const INIT: Self = Self {
+        state_hi: AtomicUsize::new(0),
+        state_lo: AtomicUsize::new(0),
+    };
 
     /// If not locked, returns the current stamp.
     ///
     /// This method should be called before optimistic reads.
     #[inline]
     pub fn optimistic_read(&self) -> Option<(usize, usize)> {
+        // The acquire loads from `state_hi` and `state_lo` synchronize with the release stores in
+        // `SeqLockWriteGuard::drop`.
+        //
+        // As a consequence, we can make sure that (1) all writes within the era of `state_hi - 1`
+        // happens before now; and therefore, (2) if `state_lo` is even, all writes within the
+        // critical section of (`state_hi`, `state_lo`) happens before now.
         let state_hi = self.state_hi.load(Ordering::Acquire);
         let state_lo = self.state_lo.load(Ordering::Acquire);
         if state_lo == 1 {
@@ -45,9 +49,25 @@ impl SeqLock {
     /// argument `stamp` should correspond to the one returned by method `optimistic_read`.
     #[inline]
     pub fn validate_read(&self, stamp: (usize, usize)) -> bool {
+        // Thanks to the fence, if we're noticing any modification to the data at the critical
+        // section of `(a, b)`, then the critical section's write of 1 to state_lo should be
+        // visible.
         atomic::fence(Ordering::Acquire);
+
+        // So if `state_lo` coincides with `stamp.1`, then either (1) we're noticing no modification
+        // to the data after the critical section of `(stamp.0, stamp.1)`, or (2) `state_lo` wrapped
+        // around.
+        //
+        // If (2) is the case, the acquire ordering ensures we see the new value of `state_hi`.
         let state_lo = self.state_lo.load(Ordering::Acquire);
+
+        // If (2) is the case and `state_hi` coincides with `stamp.0`, then `state_hi` also wrapped
+        // around, which we give up to correctly validate the read.
         let state_hi = self.state_hi.load(Ordering::Relaxed);
+
+        // Except for the case that both `state_hi` and `state_lo` wrapped around, the following
+        // condition implies that we're noticing no modification to the data after the critical
+        // section of `(stamp.0, stamp.1)`.
         (state_hi, state_lo) == stamp
     }
 
@@ -59,6 +79,8 @@ impl SeqLock {
             let previous = self.state_lo.swap(1, Ordering::Acquire);
 
             if previous != 1 {
+                // To synchronize with the acquire fence in `validate_read` via any modification to
+                // the data at the critical section of `(state_hi, previous)`.
                 atomic::fence(Ordering::Release);
 
                 return SeqLockWriteGuard {
@@ -72,7 +94,7 @@ impl SeqLock {
     }
 }
 
-/// A RAII guard that releases the lock and increments the stamp when dropped.
+/// An RAII guard that releases the lock and increments the stamp when dropped.
 pub struct SeqLockWriteGuard {
     /// The parent lock.
     lock: &'static SeqLock,
@@ -95,12 +117,16 @@ impl Drop for SeqLockWriteGuard {
         let state_lo = self.state_lo.wrapping_add(2);
 
         // Increase the high bits if the low bits wrap around.
+        //
+        // Release ordering for synchronizing with `optimistic_read`.
         if state_lo == 0 {
             let state_hi = self.lock.state_hi.load(Ordering::Relaxed);
             self.lock.state_hi.store(state_hi.wrapping_add(1), Ordering::Release);
         }
 
         // Release the lock and increment the stamp.
+        //
+        // Release ordering for synchronizing with `optimistic_read`.
         self.lock.state_lo.store(state_lo, Ordering::Release);
     }
 }