From e292f12741665706554912eda00f3b30c7c7158e Mon Sep 17 00:00:00 2001 From: Daniel Noland Date: Fri, 15 May 2026 10:57:34 -0600 Subject: [PATCH] refactor(concurrency): absorb dataplane-quiescent `dataplane-quiescent` was a single-file QSBR primitive whose only external dep was `dataplane-concurrency::sync`, and whose own `Slot` publication helper duplicated infrastructure that `concurrency` would benefit from owning anyway. Merge the crate in-tree so the primitive lives next to the sync types it builds on. Moves (no functional change): * `quiescent/src/lib.rs` -> `concurrency/src/quiescent.rs`. Imports rewritten from `concurrency::sync::...` to `crate::sync::...`; module-doc `#[doc = include_str!(...)]` retargeted at the relocated README. * `quiescent/src/slot.rs` -> `concurrency/src/slot.rs`. Visibility bumped from `pub(crate)` to `pub` so callers outside the quiescent module can use it for their own publication patterns. * `quiescent/README.md` -> `concurrency/QUIESCENT.md`. Surfaced as the module-level rustdoc. * `quiescent/tests/{loom,protocol,properties,shuttle}.rs` -> `concurrency/tests/quiescent_*.rs`. Each integration test stays in its own compilation unit; imports rewritten to `dataplane_concurrency::...` (cargo rejects self-deps as dev-dependencies, so the workspace alias `concurrency` isn't available inside the crate's own integration tests). Cargo: * `arc-swap` and `static_assertions` become direct deps of `dataplane-concurrency` (previously they were transitive via `dataplane-quiescent`). * `bolero` becomes a dev-dep for the property and shuttle tests. * `_strict_provenance` feature carried over verbatim: forces the `Mutex>` slot fallback for the miri-strict-provenance CI job, since `arc_swap`'s hazard pointers can't yet expose strict-provenance information. * Workspace `Cargo.toml` drops the `quiescent` member and its `[workspace.metadata.package.quiescent]` block. CI: * `.github/workflows/dev.yml` renames the `quiescent/permissive` and `quiescent/strict` miri steps to `concurrency/permissive` and `concurrency/strict`; both now pass `--package=dataplane-concurrency`. * The `features` matrix entry for `loom` retargets `test_package` from `quiescent` to `concurrency`. Co-Authored-By: Claude Opus 4.7 (1M context) Signed-off-by: Daniel Noland --- .github/workflows/dev.yml | 29 ++++++++++--------- Cargo.lock | 15 ++-------- Cargo.toml | 7 ----- concurrency/Cargo.toml | 13 ++++++++- .../README.md => concurrency/QUIESCENT.md | 0 concurrency/src/lib.rs | 3 ++ .../lib.rs => concurrency/src/quiescent.rs | 19 +++--------- {quiescent => concurrency}/src/slot.rs | 20 ++++++------- .../tests/quiescent_loom.rs | 2 +- .../tests/quiescent_properties.rs | 4 +-- .../tests/quiescent_protocol.rs | 2 +- .../tests/quiescent_shuttle.rs | 8 ++--- justfile | 13 ++++++++- quiescent/Cargo.toml | 29 ------------------- 14 files changed, 68 insertions(+), 96 deletions(-) rename quiescent/README.md => concurrency/QUIESCENT.md (100%) rename quiescent/src/lib.rs => concurrency/src/quiescent.rs (98%) rename {quiescent => concurrency}/src/slot.rs (78%) rename quiescent/tests/loom.rs => concurrency/tests/quiescent_loom.rs (99%) rename quiescent/tests/properties.rs => concurrency/tests/quiescent_properties.rs (98%) rename quiescent/tests/protocol.rs => concurrency/tests/quiescent_protocol.rs (99%) rename quiescent/tests/shuttle.rs => concurrency/tests/quiescent_shuttle.rs (97%) delete mode 100644 quiescent/Cargo.toml diff --git a/.github/workflows/dev.yml b/.github/workflows/dev.yml index 6895e70c91..436adba805 100644 --- a/.github/workflows/dev.yml +++ b/.github/workflows/dev.yml @@ -368,12 +368,15 @@ jobs: miri::cpu=${{ matrix.cpu }} with: recipe: "miri::test" - # quiescent is especially sensitive to concurrency violations and memory model issues so we run it - # again with and without strict provenance enabled and on many different random schedules - # The permissive provenance build runs with real arc-swap and could therefore catch issues which the strict can't. - # We run under extra schedules to make that effect stand out as much as possible (shuttle/loom can't test with the - # arc-swap in place either, so this is the closest we can get to fixing that coverage gap) - - name: "quiescent/permissive" + # The quiescent code is especially sensitive to concurrency violations + # and memory-model issues so we run it again with and without strict + # provenance enabled and on many different random schedules. The + # permissive-provenance build runs with real arc-swap and could + # therefore catch issues which the strict run can't. We run under + # extra schedules to make that effect stand out as much as possible + # (shuttle/loom can't test with arc-swap in place either, so this is + # the closest we can get to fixing that coverage gap). + - name: "concurrency/permissive" uses: *just env: JUST_VARS: >- @@ -382,8 +385,8 @@ jobs: miri::provenance=permissive with: recipe: "miri::test" - recipe_args: "--package=dataplane-quiescent" - - name: "quiescent/strict" + recipe_args: "--package=dataplane-concurrency" + - name: "concurrency/strict" uses: *just env: JUST_VARS: >- @@ -391,7 +394,7 @@ jobs: miri::provenance=strict with: recipe: "miri::test" - recipe_args: "--package=dataplane-quiescent --features=_strict_provenance" + recipe_args: "--package=dataplane-concurrency --features=_strict_provenance" - *tmate wasm: @@ -521,12 +524,12 @@ jobs: # The `loom` feature flips `concurrency::sync` to loom's # primitives workspace-wide, which breaks crates that rely on # `Weak`, `Arc::downgrade`, etc. (those aren't in - # `loom::sync`). Scope the loom build to only the quiescent - # package so workspace feature unification doesn't poison - # unrelated crates. + # `loom::sync`). Scope the loom build to only the concurrency + # package (which hosts the quiescent tests) so workspace + # feature unification doesn't poison unrelated crates. - build: *release-build features: "loom" - test_package: "quiescent" + test_package: "concurrency" - build: *release-build features: "shuttle" test_package: "" diff --git a/Cargo.lock b/Cargo.lock index 0bc0747276..586ce4a4bc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1232,9 +1232,12 @@ dependencies = [ name = "dataplane-concurrency" version = "0.21.0" dependencies = [ + "arc-swap", + "bolero", "dataplane-concurrency-macros", "loom", "shuttle", + "static_assertions", ] [[package]] @@ -1597,18 +1600,6 @@ dependencies = [ "tracing", ] -[[package]] -name = "dataplane-quiescent" -version = "0.21.0" -dependencies = [ - "arc-swap", - "bolero", - "dataplane-concurrency", - "loom", - "shuttle", - "static_assertions", -] - [[package]] name = "dataplane-rekon" version = "0.21.0" diff --git a/Cargo.toml b/Cargo.toml index 5a9ffdbc58..975e34099e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -26,7 +26,6 @@ members = [ "nat", "net", "pipeline", - "quiescent", "rekon", "routing", "stats", @@ -81,7 +80,6 @@ mgmt = { path = "./mgmt", package = "dataplane-mgmt", features = [] } nat = { path = "./nat", package = "dataplane-nat", features = [] } net = { path = "./net", package = "dataplane-net", features = [] } pipeline = { path = "./pipeline", package = "dataplane-pipeline", features = [] } -quiescent = { path = "./quiescent", package = "dataplane-quiescent", features = [] } rekon = { path = "./rekon", package = "dataplane-rekon", features = [] } routing = { path = "./routing", package = "dataplane-routing", features = [] } stats = { path = "./stats", package = "dataplane-stats", features = [] } @@ -339,11 +337,6 @@ package = "dataplane-tracectl" miri = false # hopeless + pointless wasm = false # hopeless + pointless -[workspace.metadata.package.quiescent] -package = "dataplane-quiescent" -miri = true -wasm = false # works but pointless - [workspace.metadata.package.vpcmap] package = "dataplane-vpcmap" miri = true diff --git a/concurrency/Cargo.toml b/concurrency/Cargo.toml index 7d87b814e9..fcec53af33 100644 --- a/concurrency/Cargo.toml +++ b/concurrency/Cargo.toml @@ -9,8 +9,19 @@ version.workspace = true loom = ["dep:loom", "concurrency-macros/loom"] shuttle = ["dep:shuttle", "concurrency-macros/shuttle"] silence_clippy = ["concurrency-macros/silence_clippy"] # Do not manually enable this feature, let --all-features do it for you +# Force the Mutex-based slot fallback even without a model checker. +# Intended for the CI miri job that exercises the fallback slot under +# strict-provenance; arc_swap uses hazard pointers that strict-provenance +# rejects, so the production slot can't be miri-checked under strict +# provenance. +_strict_provenance = [] [dependencies] +arc-swap = { workspace = true } +concurrency-macros = { workspace = true, features = [] } loom = { workspace = true, optional = true, features = [] } shuttle = { workspace = true, optional = true, features = [] } -concurrency-macros = { workspace = true, features = [] } +static_assertions = { workspace = true } + +[dev-dependencies] +bolero = { workspace = true, features = ["std"] } diff --git a/quiescent/README.md b/concurrency/QUIESCENT.md similarity index 100% rename from quiescent/README.md rename to concurrency/QUIESCENT.md diff --git a/concurrency/src/lib.rs b/concurrency/src/lib.rs index 6a9f0c04e8..f3546218bd 100644 --- a/concurrency/src/lib.rs +++ b/concurrency/src/lib.rs @@ -72,3 +72,6 @@ compile_error!("silence_clippy manually enabled, should only be enabled by --all #[allow(unused_imports)] pub use macros::*; + +pub mod quiescent; +pub mod slot; diff --git a/quiescent/src/lib.rs b/concurrency/src/quiescent.rs similarity index 98% rename from quiescent/src/lib.rs rename to concurrency/src/quiescent.rs index a8b63f30e7..b09654603d 100644 --- a/quiescent/src/lib.rs +++ b/concurrency/src/quiescent.rs @@ -1,29 +1,18 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright Open Network Fabric Authors -#![doc = include_str!("../README.md")] -#![forbid(unsafe_code)] -#![deny( - missing_docs, - clippy::pedantic, - clippy::unwrap_used, - clippy::expect_used, - clippy::panic -)] - -mod slot; +#![doc = include_str!("../QUIESCENT.md")] use core::cell::{Cell, RefCell}; use core::marker::PhantomData; use core::num::NonZero; -use concurrency::sync::{ +use crate::slot::Slot; +use crate::sync::{ Arc, Mutex, atomic::{AtomicU64, Ordering}, }; -use crate::slot::Slot; - struct Versioned { /// Monotonic version stamp assigned by the Publisher. version: Version, @@ -102,7 +91,7 @@ impl Domain { // any subsequent `retired.clear()` decrement of the // matching `Versioned` is now ordered after the // Subscriber's prior `cached = None` decrement. - concurrency::sync::atomic::fence(Ordering::Acquire); + crate::sync::atomic::fence(Ordering::Acquire); return false; } let observed = cell.load(); diff --git a/quiescent/src/slot.rs b/concurrency/src/slot.rs similarity index 78% rename from quiescent/src/slot.rs rename to concurrency/src/slot.rs index b2d7809c09..ac73763368 100644 --- a/quiescent/src/slot.rs +++ b/concurrency/src/slot.rs @@ -21,21 +21,21 @@ // fallback implementation. cfg_select! { any(feature = "loom", feature = "shuttle", feature = "_strict_provenance") => { - use concurrency::sync::{Arc, Mutex}; + use crate::sync::{Arc, Mutex}; - pub(crate) struct Slot(Mutex>); + pub struct Slot(Mutex>); impl Slot { - pub(crate) fn from_pointee(value: T) -> Self { + pub fn from_pointee(value: T) -> Self { Self(Mutex::new(Arc::new(value))) } - pub(crate) fn load_full(&self) -> Arc { + pub fn load_full(&self) -> Arc { #[allow(clippy::expect_used)] // poisoned only in unrecoverable cases Arc::clone(&self.0.lock().expect("slot mutex poisoned")) } - pub(crate) fn swap(&self, new: Arc) -> Arc { + pub fn swap(&self, new: Arc) -> Arc { #[allow(clippy::expect_used)] let mut guard = self.0.lock().expect("slot mutex poisoned"); core::mem::replace(&mut *guard, new) @@ -43,24 +43,24 @@ cfg_select! { } } _ => { - use concurrency::sync::Arc; + use crate::sync::Arc; use arc_swap::ArcSwap; - pub(crate) struct Slot(ArcSwap); + pub struct Slot(ArcSwap); impl Slot { #[inline] - pub(crate) fn from_pointee(value: T) -> Self { + pub fn from_pointee(value: T) -> Self { Self(ArcSwap::from_pointee(value)) } #[inline] - pub(crate) fn load_full(&self) -> Arc { + pub fn load_full(&self) -> Arc { self.0.load_full() } #[inline] - pub(crate) fn swap(&self, new: Arc) -> Arc { + pub fn swap(&self, new: Arc) -> Arc { self.0.swap(new) } } diff --git a/quiescent/tests/loom.rs b/concurrency/tests/quiescent_loom.rs similarity index 99% rename from quiescent/tests/loom.rs rename to concurrency/tests/quiescent_loom.rs index a8aef4dfea..ac8fe5038e 100644 --- a/quiescent/tests/loom.rs +++ b/concurrency/tests/quiescent_loom.rs @@ -41,7 +41,7 @@ use loom::thread; -use dataplane_quiescent::{Publisher, channel}; +use dataplane_concurrency::quiescent::{Publisher, channel}; /// Run `body` with a `&'static` reference to a freshly-constructed /// `Publisher`. After `body` returns, recover the `Box` and drop the diff --git a/quiescent/tests/properties.rs b/concurrency/tests/quiescent_properties.rs similarity index 98% rename from quiescent/tests/properties.rs rename to concurrency/tests/quiescent_properties.rs index c8b84e5792..8bc4bf7a7e 100644 --- a/quiescent/tests/properties.rs +++ b/concurrency/tests/quiescent_properties.rs @@ -32,7 +32,7 @@ use std::sync::Arc; use std::sync::atomic::{AtomicUsize, Ordering}; use bolero::TypeGenerator; -use dataplane_quiescent::channel; +use dataplane_concurrency::quiescent::channel; // ---------- ops & state ---------- @@ -171,7 +171,7 @@ fn protocol_invariants() { /// gap on a function that only differs from the auto-derive in name. #[test] fn factory_clone_uses_explicit_impl() { - let publisher = dataplane_quiescent::channel(0u32); + let publisher = dataplane_concurrency::quiescent::channel(0u32); let factory = publisher.factory(); #[allow(clippy::clone_on_copy)] let _factory_clone = factory.clone(); diff --git a/quiescent/tests/protocol.rs b/concurrency/tests/quiescent_protocol.rs similarity index 99% rename from quiescent/tests/protocol.rs rename to concurrency/tests/quiescent_protocol.rs index d90883bb8e..b24a84dad2 100644 --- a/quiescent/tests/protocol.rs +++ b/concurrency/tests/quiescent_protocol.rs @@ -29,7 +29,7 @@ use std::sync::{Arc, Mutex}; use std::thread; use std::time::Duration; -use dataplane_quiescent::channel; +use dataplane_concurrency::quiescent::channel; // ---------- helpers ---------- diff --git a/quiescent/tests/shuttle.rs b/concurrency/tests/quiescent_shuttle.rs similarity index 97% rename from quiescent/tests/shuttle.rs rename to concurrency/tests/quiescent_shuttle.rs index e35575fd0d..f4ff609e2d 100644 --- a/quiescent/tests/shuttle.rs +++ b/concurrency/tests/quiescent_shuttle.rs @@ -23,11 +23,11 @@ use std::panic::RefUnwindSafe; use bolero::TypeGenerator; -use concurrency::sync::Arc; -use concurrency::sync::atomic::{AtomicUsize, Ordering}; -use concurrency::thread; +use dataplane_concurrency::sync::Arc; +use dataplane_concurrency::sync::atomic::{AtomicUsize, Ordering}; +use dataplane_concurrency::thread; -use dataplane_quiescent::{Subscriber, channel}; +use dataplane_concurrency::quiescent::{Subscriber, channel}; // ---------- ops & plan ---------- diff --git a/justfile b/justfile index c575deae70..63450dc071 100644 --- a/justfile +++ b/justfile @@ -51,7 +51,18 @@ _cargo_feature_flags := \ _cargo_profile_flag := if profile == "debug" { "" } else { "--profile " + profile } # filters for nextest -filter := if features == "shuttle" { "shuttle" } else if features == "loom" { "-E 'binary(loom)'" } else { "" } +# +# Under `shuttle`, isolate the bolero x shuttle suite (a `shuttle` +# substring matches both the test binary and the test-name +# convention). +# +# Under `loom`, the legacy filter `-E 'binary(loom)'` matched the +# old `dataplane-quiescent` `loom` test binary. After absorbing +# the crate, the binary is `quiescent_loom` (and later test files +# add more); an empty filter lets nextest walk every archived +# binary. Tests that don't apply under loom are cfg-gated out and +# compile to zero entries. +filter := if features == "shuttle" { "shuttle" } else { "" } # instrumentation mode (none/coverage) instrument := "none" diff --git a/quiescent/Cargo.toml b/quiescent/Cargo.toml deleted file mode 100644 index 882af75022..0000000000 --- a/quiescent/Cargo.toml +++ /dev/null @@ -1,29 +0,0 @@ -[package] -name = "dataplane-quiescent" -edition.workspace = true -license.workspace = true -publish.workspace = true -version.workspace = true - -[features] -# Mutually exclusive with `shuttle`. -loom = ["concurrency/loom", "dep:loom"] -# Shuttle equivalent. Mutually exclusive with `loom`. -shuttle = ["concurrency/shuttle", "dep:shuttle"] -# for miri: enable strict provenance checks -_strict_provenance = [] - -[dependencies] -# internal -concurrency = { workspace = true } - -# external -arc-swap = { workspace = true } -static_assertions = { workspace = true } - -[target.'cfg(not(miri))'.dependencies] -loom = { workspace = true, optional = true } -shuttle = { workspace = true, optional = true } - -[dev-dependencies] -bolero = { workspace = true, features = ["std"] }