From 477c53b25d1451e4771968fcf4c48a5f7381b04f Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Fri, 29 Apr 2022 14:53:01 +0200 Subject: [PATCH 1/4] use std::thread::scope instead of scoped_threadpool as it's easier to deal with TSAN false positives in the former API as surfaced in PR 280 the current supression rules don't handle newer versions of the scoped_threadpool crate trying to update the supression rules related to scoped_threadpool in PR #282 revealed that the supression rules are masking (hiding) real data races: https://github.com/japaric/heapless/pull/282#issuecomment-1113173358 std::thread::scope requires less supression rules and does not mask real data races -- for instance, the data race in the linked issue comment is not masked when using std::thread::scope tradeoffs: - pro: one less dev dependency - pro: supressions file is simpler - cons: std::thread::scope is only available on recent nightlies --- Cargo.toml | 3 --- suppressions.txt | 9 ++------- tests/tsan.rs | 32 ++++++++++++++++---------------- 3 files changed, 18 insertions(+), 26 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 41ad73a4..5855a586 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,9 +27,6 @@ mpmc_large = [] # This flag has no version guarantee, the `defmt` dependency can be updated in a patch release defmt-impl = ["defmt"] -[target.'cfg(not(target_os = "none"))'.dev-dependencies] -scoped_threadpool = "0.1.8" - [target.thumbv6m-none-eabi.dependencies] atomic-polyfill = { version = "0.1.2", optional = true } diff --git a/suppressions.txt b/suppressions.txt index bf051e53..e3400670 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -1,7 +1,2 @@ -# false positives in thread::spawn (?) -race:*dealloc -race:*drop_slow* -race:__call_tls_dtors - -# false positives in scoped_threadpool (?) -race:*drop* +race:std::panic::catch_unwind +race:std::thread::scope diff --git a/tests/tsan.rs b/tests/tsan.rs index 22a51958..a207e125 100644 --- a/tests/tsan.rs +++ b/tests/tsan.rs @@ -1,3 +1,4 @@ +#![feature(scoped_threads)] #![deny(rust_2018_compatibility)] #![deny(rust_2018_idioms)] #![deny(warnings)] @@ -5,7 +6,6 @@ use std::{sync::mpsc, thread}; use heapless::{mpmc::Q64, spsc}; -use scoped_threadpool::Pool; #[test] fn once() { @@ -59,12 +59,12 @@ fn scoped() { { let (mut p, mut c) = rb.split(); - Pool::new(2).scoped(move |scope| { - scope.execute(move || { + thread::scope(move |scope| { + scope.spawn(move || { p.enqueue(1).unwrap(); }); - scope.execute(move || { + scope.spawn(move || { c.dequeue().unwrap(); }); }); @@ -83,8 +83,8 @@ fn contention() { { let (mut p, mut c) = rb.split(); - Pool::new(2).scoped(move |scope| { - scope.execute(move || { + thread::scope(move |scope| { + scope.spawn(move || { let mut sum: u32 = 0; for i in 0..(2 * N) { @@ -95,7 +95,7 @@ fn contention() { println!("producer: {}", sum); }); - scope.execute(move || { + scope.spawn(move || { let mut sum: u32 = 0; for _ in 0..(2 * N) { @@ -126,9 +126,9 @@ fn mpmc_contention() { static Q: Q64 = Q64::new(); let (s, r) = mpsc::channel(); - Pool::new(2).scoped(|scope| { + thread::scope(|scope| { let s1 = s.clone(); - scope.execute(move || { + scope.spawn(move || { let mut sum: u32 = 0; for i in 0..(16 * N) { @@ -141,7 +141,7 @@ fn mpmc_contention() { }); let s2 = s.clone(); - scope.execute(move || { + scope.spawn(move || { let mut sum: u32 = 0; for _ in 0..(16 * N) { @@ -178,14 +178,14 @@ fn unchecked() { { let (mut p, mut c) = rb.split(); - Pool::new(2).scoped(move |scope| { - scope.execute(move || { + thread::scope(move |scope| { + scope.spawn(move || { for _ in 0..N / 2 - 1 { p.enqueue(2).unwrap(); } }); - scope.execute(move || { + scope.spawn(move || { let mut sum: usize = 0; for _ in 0..N / 2 - 1 { @@ -246,8 +246,8 @@ fn pool() { A::grow(unsafe { &mut M }); - Pool::new(2).scoped(move |scope| { - scope.execute(move || { + thread::pool(move |scope| { + scope.spawn(move || { for _ in 0..N / 4 { let a = A::alloc().unwrap(); let b = A::alloc().unwrap(); @@ -257,7 +257,7 @@ fn pool() { } }); - scope.execute(move || { + scope.spawn(move || { for _ in 0..N / 2 { let a = A::alloc().unwrap(); let a = a.init([2; 8]); From 3d3277f6d3d3a788ae2c8c661b789bb2429b23fd Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Fri, 29 Apr 2022 15:19:23 +0200 Subject: [PATCH 2/4] only build tests that require thread::scope on nightly --- Cargo.toml | 3 +++ build.rs | 9 +++++++++ tests/tsan.rs | 14 +++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5855a586..1b5404f2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -62,5 +62,8 @@ version = "0.1" version = ">=0.2.0,<0.4" optional = true +[build-dependencies] +rustc_version = "0.4.0" + [package.metadata.docs.rs] all-features = true diff --git a/build.rs b/build.rs index 0840d6fb..1342bd70 100644 --- a/build.rs +++ b/build.rs @@ -2,6 +2,8 @@ use std::{env, error::Error}; +use rustc_version::Channel; + fn main() -> Result<(), Box> { let target = env::var("TARGET")?; @@ -66,5 +68,12 @@ fn main() -> Result<(), Box> { _ => {} } + if !matches!( + rustc_version::version_meta().unwrap().channel, + Channel::Stable | Channel::Beta + ) { + println!("cargo:rustc-cfg=unstable_channel"); + } + Ok(()) } diff --git a/tests/tsan.rs b/tests/tsan.rs index a207e125..658f6aa9 100644 --- a/tests/tsan.rs +++ b/tests/tsan.rs @@ -1,11 +1,11 @@ -#![feature(scoped_threads)] +#![cfg_attr(unstable_channel, feature(scoped_threads))] #![deny(rust_2018_compatibility)] #![deny(rust_2018_idioms)] #![deny(warnings)] -use std::{sync::mpsc, thread}; +use std::thread; -use heapless::{mpmc::Q64, spsc}; +use heapless::spsc; #[test] fn once() { @@ -51,6 +51,7 @@ fn twice() { } #[test] +#[cfg(unstable_channel)] fn scoped() { let mut rb: spsc::Queue = spsc::Queue::new(); @@ -75,6 +76,7 @@ fn scoped() { #[test] #[cfg_attr(miri, ignore)] // too slow +#[cfg(unstable_channel)] fn contention() { const N: usize = 1024; @@ -120,7 +122,12 @@ fn contention() { #[test] #[cfg_attr(miri, ignore)] // too slow +#[cfg(unstable_channel)] fn mpmc_contention() { + use std::sync::mpsc; + + use heapless::mpmc::Q64; + const N: u32 = 64; static Q: Q64 = Q64::new(); @@ -166,6 +173,7 @@ fn mpmc_contention() { #[test] #[cfg_attr(miri, ignore)] // too slow +#[cfg(unstable_channel)] fn unchecked() { const N: usize = 1024; From 1e1d801b07c20f73f3343aae526d9785f9a3b2b3 Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Fri, 29 Apr 2022 15:29:15 +0200 Subject: [PATCH 3/4] fix typo --- tests/tsan.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/tsan.rs b/tests/tsan.rs index 658f6aa9..9e87fb0e 100644 --- a/tests/tsan.rs +++ b/tests/tsan.rs @@ -254,7 +254,7 @@ fn pool() { A::grow(unsafe { &mut M }); - thread::pool(move |scope| { + thread::scope(move |scope| { scope.spawn(move || { for _ in 0..N / 4 { let a = A::alloc().unwrap(); From 57ac20eeb7b774bad2d300572ad5105b97d9dd9b Mon Sep 17 00:00:00 2001 From: Jorge Aparicio Date: Fri, 29 Apr 2022 16:02:48 +0200 Subject: [PATCH 4/4] ubuntu-specific(?) suppressions --- suppressions.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/suppressions.txt b/suppressions.txt index e3400670..1f4eb1ef 100644 --- a/suppressions.txt +++ b/suppressions.txt @@ -1,2 +1,7 @@ race:std::panic::catch_unwind race:std::thread::scope + +# std::thread::spawn false positive; seen on Ubuntu 20.04 but not on Arch Linux (2022-04-29) +race:drop_in_place*JoinHandle +race:alloc::sync::Arc<*>::drop_slow +race:__call_tls_dtors