From 84fd95cbedf1e1c1826e378ffc4d1a3d335deff4 Mon Sep 17 00:00:00 2001
From: Connor Tsui <connor.tsui20@gmail.com>
Date: Fri, 8 Nov 2024 18:13:18 -0500
Subject: [PATCH] fix memory ordering bug + bad test

This commit fixes a memory ordering bug in the futex implementation
(`Relaxed` -> `Release` on `downgrade`).

This commit also removes a badly written test that deadlocked and
replaces it with a more reasonable test based on an already-tested
`downgrade` test from the parking-lot crate.
---
 library/std/src/sync/rwlock/tests.rs     | 79 ++++++++++--------------
 library/std/src/sys/sync/rwlock/futex.rs |  2 +-
 2 files changed, 33 insertions(+), 48 deletions(-)

diff --git a/library/std/src/sync/rwlock/tests.rs b/library/std/src/sync/rwlock/tests.rs
index a4af49dc82cce..4f30ef3cac398 100644
--- a/library/std/src/sync/rwlock/tests.rs
+++ b/library/std/src/sync/rwlock/tests.rs
@@ -3,8 +3,8 @@ use rand::Rng;
 use crate::sync::atomic::{AtomicUsize, Ordering};
 use crate::sync::mpsc::channel;
 use crate::sync::{
-    Arc, Barrier, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard,
-    RwLockWriteGuard, TryLockError,
+    Arc, MappedRwLockReadGuard, MappedRwLockWriteGuard, RwLock, RwLockReadGuard, RwLockWriteGuard,
+    TryLockError,
 };
 use crate::thread;
 
@@ -511,57 +511,42 @@ fn test_downgrade_basic() {
 }
 
 #[test]
-fn test_downgrade_readers() {
-    // This test creates 1 writing thread and `R` reader threads doing `N` iterations.
-    const R: usize = 10;
-    const N: usize = if cfg!(target_pointer_width = "64") { 100 } else { 20 };
+fn test_downgrade_observe() {
+    // Taken from the test `test_rwlock_downgrade` from:
+    // https://github.com/Amanieu/parking_lot/blob/master/src/rwlock.rs
 
-    // The writer thread will constantly update the value inside the `RwLock`, and this test will
-    // only pass if every reader observes all values between 0 and `N`.
-    let rwlock = Arc::new(RwLock::new(0));
-    let barrier = Arc::new(Barrier::new(R + 1));
-
-    // Create the writing thread.
-    let r_writer = rwlock.clone();
-    let b_writer = barrier.clone();
-    thread::spawn(move || {
-        for i in 0..N {
-            let mut write_guard = r_writer.write().unwrap();
-            *write_guard = i;
+    const W: usize = if cfg!(target_pointer_width = "64") { 100 } else { 20 };
+    const N: usize = 100;
 
-            let read_guard = RwLockWriteGuard::downgrade(write_guard);
-            assert_eq!(*read_guard, i);
+    // This test spawns `W` writer threads, where each will increment a counter `N` times, ensuring
+    // that the value they wrote has not changed after downgrading.
 
-            // Wait for all readers to observe the new value.
-            b_writer.wait();
-        }
-    });
+    let rw = Arc::new(RwLock::new(0));
 
-    for _ in 0..R {
-        let rwlock = rwlock.clone();
-        let barrier = barrier.clone();
-        thread::spawn(move || {
-            // Every reader thread needs to observe every value up to `N`.
-            for i in 0..N {
-                let read_guard = rwlock.read().unwrap();
-                assert_eq!(*read_guard, i);
-                drop(read_guard);
-
-                // Wait for everyone to read and for the writer to change the value again.
-                barrier.wait();
-
-                // Spin until the writer has changed the value.
-                loop {
-                    let read_guard = rwlock.read().unwrap();
-                    assert!(*read_guard >= i);
-
-                    if *read_guard > i {
-                        break;
-                    }
+    // Spawn the writers that will do `W * N` operations and checks.
+    let handles: Vec<_> = (0..W)
+        .map(|_| {
+            let rw = rw.clone();
+            thread::spawn(move || {
+                for _ in 0..N {
+                    // Increment the counter.
+                    let mut write_guard = rw.write().unwrap();
+                    *write_guard += 1;
+                    let cur_val = *write_guard;
+
+                    // Downgrade the lock to read mode, where the value protected cannot be modified.
+                    let read_guard = RwLockWriteGuard::downgrade(write_guard);
+                    assert_eq!(cur_val, *read_guard);
                 }
-            }
-        });
+            })
+        })
+        .collect();
+
+    for handle in handles {
+        handle.join().unwrap();
     }
+
+    assert_eq!(*rw.read().unwrap(), W * N);
 }
 
 #[test]
diff --git a/library/std/src/sys/sync/rwlock/futex.rs b/library/std/src/sys/sync/rwlock/futex.rs
index e0f4a91e0731b..961819cae8d6e 100644
--- a/library/std/src/sys/sync/rwlock/futex.rs
+++ b/library/std/src/sys/sync/rwlock/futex.rs
@@ -195,7 +195,7 @@ impl RwLock {
     #[inline]
     pub unsafe fn downgrade(&self) {
         // Removes all write bits and adds a single read bit.
-        let state = self.state.fetch_add(DOWNGRADE, Relaxed);
+        let state = self.state.fetch_add(DOWNGRADE, Release);
         debug_assert!(is_write_locked(state), "RwLock must be write locked to call `downgrade`");
 
         if has_readers_waiting(state) {