From b427411b64aca7e9579408a68eb1182ec1ce1a33 Mon Sep 17 00:00:00 2001 From: Zach Reizner Date: Wed, 29 Jan 2020 15:34:42 -0800 Subject: data_model: fix flaky observe_mutate The original version of this test used sleeps, retries, and vague commentary about how the test is made reliable. The new version of test uses real synchronization primitives.. BUG=chromium:1045426 TEST=put all cores under load; cargo test -p data_model Change-Id: I7fa4ac45a9003e2ebb98c57ca6a03be17bdf65cf Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/2029925 Tested-by: Zach Reizner Tested-by: kokoro Auto-Submit: Zach Reizner Reviewed-by: Dylan Reid Commit-Queue: Zach Reizner --- data_model/src/volatile_memory.rs | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) (limited to 'data_model') diff --git a/data_model/src/volatile_memory.rs b/data_model/src/volatile_memory.rs index c02111e..d834f0b 100644 --- a/data_model/src/volatile_memory.rs +++ b/data_model/src/volatile_memory.rs @@ -401,9 +401,8 @@ impl<'a, T: DataInit> VolatileRef<'a, T> { mod tests { use super::*; - use std::sync::Arc; - use std::thread::{sleep, spawn}; - use std::time::Duration; + use std::sync::{Arc, Barrier}; + use std::thread::spawn; #[derive(Clone)] struct VecMem { @@ -472,35 +471,23 @@ mod tests { let a_clone = a.clone(); let v_ref = a.get_ref::(0).unwrap(); v_ref.store(99); + + let start_barrier = Arc::new(Barrier::new(2)); + let thread_start_barrier = start_barrier.clone(); + let end_barrier = Arc::new(Barrier::new(2)); + let thread_end_barrier = end_barrier.clone(); spawn(move || { - sleep(Duration::from_millis(10)); + thread_start_barrier.wait(); let clone_v_ref = a_clone.get_ref::(0).unwrap(); clone_v_ref.store(0); + thread_end_barrier.wait(); }); - // Technically this is a race condition but we have to observe the v_ref's value changing - // somehow and this helps to ensure the sleep actually happens before the store rather then - // being reordered by the compiler. assert_eq!(v_ref.load(), 99); - // Granted we could have a machine that manages to perform this many volatile loads in the - // amount of time the spawned thread sleeps, but the most likely reason the retry limit will - // get reached is because v_ref.load() is not actually performing the required volatile read - // or v_ref.store() is not doing a volatile write. A timer based solution was avoided - // because that might use a syscall which could hint the optimizer to reload v_ref's pointer - // regardless of volatile status. Note that we use a longer retry duration for optimized - // builds. - #[cfg(debug_assertions)] - const RETRY_MAX: u64 = 500_000_000; - #[cfg(not(debug_assertions))] - const RETRY_MAX: u64 = 10_000_000_000; - - let mut retry = 0; - while v_ref.load() == 99 && retry < RETRY_MAX { - retry += 1; - } + start_barrier.wait(); + end_barrier.wait(); - assert_ne!(retry, RETRY_MAX, "maximum retry exceeded"); assert_eq!(v_ref.load(), 0); } -- cgit 1.4.1