From 0396adb66c72367538f4329c8a8939443c9020d4 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 10 Oct 2025 20:30:10 +0200 Subject: [PATCH 01/35] feat(utils): add crate left-right-tlcache This crate implements a thread-local cache of left-right ReadHandles. Signed-off-by: Fredi Raspall --- Cargo.lock | 7 ++ Cargo.toml | 2 + left-right-tlcache/Cargo.toml | 9 +++ left-right-tlcache/src/lib.rs | 137 ++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+) create mode 100644 left-right-tlcache/Cargo.toml create mode 100644 left-right-tlcache/src/lib.rs diff --git a/Cargo.lock b/Cargo.lock index 6712c2c2e..c0d07f5ef 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1108,6 +1108,13 @@ dependencies = [ "tracing", ] +[[package]] +name = "dataplane-left-right-tlcache" +version = "0.1.0" +dependencies = [ + "left-right", +] + [[package]] name = "dataplane-lpm" version = "0.1.0" diff --git a/Cargo.toml b/Cargo.toml index ac7f6635d..fa4c7e757 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -16,6 +16,7 @@ members = [ "id", "init", "interface-manager", + "left-right-tlcache", "mgmt", "nat", "net", @@ -51,6 +52,7 @@ hardware = { path = "./hardware", package = "dataplane-hardware" } id = { path = "./id", package = "dataplane-id" } init = { path = "./init", package = "dataplane-init" } interface-manager = { path = "./interface-manager", package = "dataplane-interface-manager" } +left-right-tlcache = { path = "./left-right-tlcache", package = "dataplane-left-right-tlcache" } lpm = { path = "./lpm", package = "dataplane-lpm" } mgmt = { path = "./mgmt", package = "dataplane-mgmt" } nat = { path = "./nat", package = "dataplane-nat" } diff --git a/left-right-tlcache/Cargo.toml b/left-right-tlcache/Cargo.toml new file mode 100644 index 000000000..85624676c --- /dev/null +++ b/left-right-tlcache/Cargo.toml @@ -0,0 +1,9 @@ +[package] +name = "dataplane-left-right-tlcache" +version = "0.1.0" +edition = "2024" +publish = false +license = "Apache-2.0" + +[dependencies] +left-right = { workspace = true } diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs new file mode 100644 index 000000000..6690a2297 --- /dev/null +++ b/left-right-tlcache/src/lib.rs @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Small crate to provide thread-local storage of left-right "read handles". +//! TL;DR: the `ReadHandle` type that allows concurrent reads over some type T cannot +//! be safely shared among distinct threads. There are cases where multiple threads may need +//! to read multiple T's but the set of T's (and read handles) may not be known upfront and +//! change over time. +//! +//! So, a mechanism is needed to: +//! - ensure that threads discover read handles for multiple instances of T +//! - ensure they are provided with non-shared read handles for those T's +//! +//! This crate provides a type to that end that relies on a thread-local cache of read-handles. +//! Having a thread-local set of read-handles avoids the potentially-expensive creation of those +//! every time an instance of T needs to be read (e.g. a FIB). This type assumes that each of the +//! T instances (or their handles) can be identified by some Key. In order to populate the local +//! cache of read handles a read-handle "provider" is needed. Neither the key types, T or the provider +//! are prescribed here. The provider functionality is implemented as a trait. +//! +//! An alternative approach to the problem would use `thread_local::ThreadLocal`. However, the latter +//! requires Q to be `Send`, which is not necessarily the case of `ReadHandle`. Specifically, +//! `ReadHandle` is Send only if T is Sync. +//! +//! The use of this type requires: +//! - implementing trait `ReadHandleProvider` to provide `ReadHandle` 's based on some key value. +//! - declaring a thread-local `ReadHandleCache` object +//! +//! Note: providers must be Sync since the thread-local caches for distinct threads will poll them. + +use left_right::{ReadHandle, ReadHandleFactory}; +use std::cell::RefCell; +use std::collections::HashMap; +use std::hash::Hash; +use std::rc::Rc; +use std::thread::LocalKey; + +pub trait ReadHandleProvider: Sync { + type Data; + type Key; + + /// A provider should provide per thread `ReadHandles`. However, it is safer if we require providers + /// to give us factories as that shields us from buggy providers returning ReadHandles that are not + /// unique per thread. There should be no performance penalty of offering factories instead of read handles + /// since factory::handle() is identical to rhandle::clone() + fn get_factory(&self, key: &Self::Key) -> Option<&ReadHandleFactory>; +} + +#[derive(Debug)] +pub enum ReadHandleCacheError { + NotFound(K), + NotAccessible(K), +} + +pub struct ReadHandleCache { + handles: RefCell>>>, +} +impl ReadHandleCache +where + K: Hash + Eq + Clone, +{ + #[allow(clippy::new_without_default)] + pub fn new() -> Self { + Self { + handles: RefCell::new(HashMap::new()), + } + } + pub fn get_reader( + thread_local: &'static LocalKey, + key: K, + provider: &impl ReadHandleProvider, + ) -> Result>, ReadHandleCacheError> { + thread_local.with(|local| { + let mut map = local.handles.borrow_mut(); + + // cache has a valid handle for that key + if let Some(rhandle) = map.get(&key) + && !rhandle.was_dropped() + { + return Ok(Rc::clone(rhandle)); + } + + // Either we've found a handle but was invalid or we did not find it. + // In either case, request a fresh handle to the provider, which may: + // 1) fail to provide one with that key + // 2) provide a valid one + // 3) provide an invalid one (e.g. if the `WriteHandle` was dropped). + // We don't require providers to provide good readhandles/factories, nor + // want to return invalid handles to callers, so we validate them too. + // A valid readhandle is one where, at least now, can be 'entered'. + + let result = { + let fresh = provider + .get_factory(&key) + .map(|factory| factory.handle()) + .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; + + if fresh.was_dropped() { + Err(ReadHandleCacheError::NotAccessible(key.clone())) + } else { + let fresh = Rc::new(fresh); + map.entry(key.clone()) + .and_modify(|e| *e = Rc::clone(&fresh)) + .or_insert_with(|| Rc::clone(&fresh)); + + Ok(fresh) + } + }; + if result.is_err() { + // clean-up cache on failure + map.remove(&key); + } + result + }) + } +} + +/// Create a thread-local `ReadHandleCache` with a given name, to access +/// `ReadHandle`'s identified with some type of key. +/// Example: +/// ``` +/// use left_right::{ReadHandle, ReadHandleFactory}; +/// use dataplane_left_right_tlcache::make_thread_local_readhandle_cache; +/// use dataplane_left_right_tlcache::ReadHandleCache; +/// +/// struct LeftRightWrappedType; +/// +/// make_thread_local_readhandle_cache!(MYCACHE, u32, LeftRightWrappedType); +/// ``` +#[macro_export] +macro_rules! make_thread_local_readhandle_cache { + ($name:ident, $key_t:ty, $rhandle_t:ty) => { + thread_local! { + static $name: ReadHandleCache<$key_t, $rhandle_t> = ReadHandleCache::new(); + } + }; +} From 57399020b97d5d1c91c6421c8c135a2dd3201d0c Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 10 Oct 2025 20:42:14 +0200 Subject: [PATCH 02/35] feat(left-right-tlcache): use ahash for local map Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + left-right-tlcache/Cargo.toml | 1 + left-right-tlcache/src/lib.rs | 5 +++-- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c0d07f5ef..6d574ca5f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1112,6 +1112,7 @@ dependencies = [ name = "dataplane-left-right-tlcache" version = "0.1.0" dependencies = [ + "ahash 0.8.10", "left-right", ] diff --git a/left-right-tlcache/Cargo.toml b/left-right-tlcache/Cargo.toml index 85624676c..11597e9b2 100644 --- a/left-right-tlcache/Cargo.toml +++ b/left-right-tlcache/Cargo.toml @@ -7,3 +7,4 @@ license = "Apache-2.0" [dependencies] left-right = { workspace = true } +ahash = { workspace = true } diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index 6690a2297..08fd96865 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -28,6 +28,7 @@ //! //! Note: providers must be Sync since the thread-local caches for distinct threads will poll them. +use ahash::RandomState; use left_right::{ReadHandle, ReadHandleFactory}; use std::cell::RefCell; use std::collections::HashMap; @@ -53,7 +54,7 @@ pub enum ReadHandleCacheError { } pub struct ReadHandleCache { - handles: RefCell>>>, + handles: RefCell>, RandomState>>, } impl ReadHandleCache where @@ -62,7 +63,7 @@ where #[allow(clippy::new_without_default)] pub fn new() -> Self { Self { - handles: RefCell::new(HashMap::new()), + handles: RefCell::new(HashMap::with_hasher(RandomState::with_seed(0))), } } pub fn get_reader( From 67083a84541afcab820fb858d981d48b0846a54e Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Mon, 13 Oct 2025 12:50:09 +0200 Subject: [PATCH 03/35] feat(left-right-tlcache): use thiserror Use thiserror for ReadHandleCacheError to easily integrate cache errors. Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + left-right-tlcache/Cargo.toml | 1 + left-right-tlcache/src/lib.rs | 5 ++++- 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 6d574ca5f..9e120f6ba 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1114,6 +1114,7 @@ version = "0.1.0" dependencies = [ "ahash 0.8.10", "left-right", + "thiserror 2.0.17", ] [[package]] diff --git a/left-right-tlcache/Cargo.toml b/left-right-tlcache/Cargo.toml index 11597e9b2..f6b5bc79f 100644 --- a/left-right-tlcache/Cargo.toml +++ b/left-right-tlcache/Cargo.toml @@ -8,3 +8,4 @@ license = "Apache-2.0" [dependencies] left-right = { workspace = true } ahash = { workspace = true } +thiserror = { workspace = true } diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index 08fd96865..f58d1dbd1 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -35,6 +35,7 @@ use std::collections::HashMap; use std::hash::Hash; use std::rc::Rc; use std::thread::LocalKey; +use thiserror::Error; pub trait ReadHandleProvider: Sync { type Data; @@ -47,9 +48,11 @@ pub trait ReadHandleProvider: Sync { fn get_factory(&self, key: &Self::Key) -> Option<&ReadHandleFactory>; } -#[derive(Debug)] +#[derive(Debug, PartialEq, Error)] pub enum ReadHandleCacheError { + #[error("Reader not found for key {0}")] NotFound(K), + #[error("Reader for key {0} is not accessible")] NotAccessible(K), } From 0e1543f054dc5de7b7e06267860b963c6e4c4856 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Mon, 13 Oct 2025 17:12:06 +0200 Subject: [PATCH 04/35] feat(left-right-tlcache): cache entry invalidation The existing cache relies on a single criteria to invalidate an entry: if the ReadHandle cannot be "entered", then it is invalid and the provider needs to be asked for a refresh; which may fail or not, depending on whether a factory for the key exists or not. This behavior suffices to invalidate entries (read handles) when the wrapped objects are removed or created in the provider. However, providers (and hence caches) may have "alias" keys. An example is a Fib object that can be identified by both an Id (master key) but also a Vni. In such a case, the cache may itself have two cached entries, pointing to the same readhandle. So far so good. However, if such aliases get exchanged without the objects being torn down, they will remain in the cache indefinitely, pointing to the wrong object, because they will not be invalidated. In the example of a Fib, suppose 2 Fibs with ids A and B. A is accessible via vni 10 and B only with its Id. At some point, there is a configuration change such that Fib A no longer uses vni 10, but B is assigned vni 10, and both Fibs continue to exist.Without a mechanism to invalidate the entry with key vni = 10, the cache would return a readhandle for A instead of B. Invalidating such entries (vni 10 -> Rhandle(A)) can be tricky because we don't want to poll the provider for a handle every time (which would defeat the whole point of the cache) and because the provider itself may be wrapped in left-right, meaning that changes may not immediately appear in readers. Some mechanism is needed to be able to tell if a cached entry points to the "right" object, or has to be otherwise invalidated / refreshed. This patch augments the implementation to address that case by: - defining the identity of a T (in ReadHandle) with a trait - augmenting cached entries to store that identity, which may differ from the key used to access them - adding method get_identity() to ReadHandleProvider so that caches can request the latest identity for a key. - invalidating entries by detecting if their identity has changed. This is achieved by asking the provider the identity of a T and comparing it with the cached identity. As a sanity, we enter T and check the real identity of the object. This cache invalidation requires an extra hash lookup when a readhandle exist for a key != identity. Such extra lookups are reduced in the next patch. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 91 ++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 23 deletions(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index f58d1dbd1..fc1a6ebe3 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -24,6 +24,7 @@ //! //! The use of this type requires: //! - implementing trait `ReadHandleProvider` to provide `ReadHandle` 's based on some key value. +//! - implementing trait `Identity` for T in `ReadHandle` //! - declaring a thread-local `ReadHandleCache` object //! //! Note: providers must be Sync since the thread-local caches for distinct threads will poll them. @@ -46,6 +47,18 @@ pub trait ReadHandleProvider: Sync { /// unique per thread. There should be no performance penalty of offering factories instead of read handles /// since factory::handle() is identical to rhandle::clone() fn get_factory(&self, key: &Self::Key) -> Option<&ReadHandleFactory>; + + /// Ask the provider about the identity of T for the `ReadHandle` accessible via some key. + /// This is needed for the cache to be able to invalidate entries that point to `ReadHandle`'s + /// for T's that should no longer be accessed by that key. + fn get_identity(&self, key: &Self::Key) -> Option; +} + +/// Trait to determine the real identity of a `T` wrapped in left-right. That is, +/// the identity of `T` in a `ReadHandle`. This is needed to invalidate cache entries +/// with keys that are alias of their identity. +pub trait Identity { + fn identity(&self) -> K; } #[derive(Debug, PartialEq, Error)] @@ -56,12 +69,46 @@ pub enum ReadHandleCacheError { NotAccessible(K), } -pub struct ReadHandleCache { - handles: RefCell>, RandomState>>, +/// An entry in a thread-local `ReadHandleCache` to hold a `ReadHandle` +/// along with its identity, which may match the key to find it or not. +struct ReadHandleEntry { + rhandle: Rc>, + identity: K, +} +impl, K: PartialEq> ReadHandleEntry { + fn new(identity: K, rhandle: Rc>) -> Self { + Self { rhandle, identity } + } + fn is_valid(&self, key: &K, provider: &impl ReadHandleProvider) -> bool { + if self.rhandle.was_dropped() { + return false; + } + if *key == self.identity { + return true; + } + if self.version == provider.get_version() { + return true; + } + let Some(identity) = provider.get_identity(key) else { + return false; + }; + if self.identity != identity { + return false; + } + match self.rhandle.enter() { + Some(t) => t.identity() == identity, + None => false, + } + } +} + +pub struct ReadHandleCache { + handles: RefCell, RandomState>>, } impl ReadHandleCache where K: Hash + Eq + Clone, + T: Identity, { #[allow(clippy::new_without_default)] pub fn new() -> Self { @@ -78,37 +125,31 @@ where let mut map = local.handles.borrow_mut(); // cache has a valid handle for that key - if let Some(rhandle) = map.get(&key) - && !rhandle.was_dropped() + if let Some(entry) = map.get(&key) + && entry.is_valid(&key, provider) { - return Ok(Rc::clone(rhandle)); + return Ok(Rc::clone(&entry.rhandle)); } - // Either we've found a handle but was invalid or we did not find it. - // In either case, request a fresh handle to the provider, which may: - // 1) fail to provide one with that key - // 2) provide a valid one - // 3) provide an invalid one (e.g. if the `WriteHandle` was dropped). - // We don't require providers to provide good readhandles/factories, nor - // want to return invalid handles to callers, so we validate them too. - // A valid readhandle is one where, at least now, can be 'entered'. - let result = { + // get a factory for the key from the provider and build a fresh handle from it let fresh = provider .get_factory(&key) .map(|factory| factory.handle()) .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; - if fresh.was_dropped() { - Err(ReadHandleCacheError::NotAccessible(key.clone())) - } else { - let fresh = Rc::new(fresh); - map.entry(key.clone()) - .and_modify(|e| *e = Rc::clone(&fresh)) - .or_insert_with(|| Rc::clone(&fresh)); + // get master identity + let identity = fresh + .enter() + .ok_or_else(|| ReadHandleCacheError::NotAccessible(key.clone()))? + .as_ref() + .identity(); - Ok(fresh) - } + // store a new entry locally with a handle, its identity and the key + let rhandle = Rc::new(fresh); + let entry = ReadHandleEntry::new(identity, Rc::clone(&rhandle)); + map.insert(key.clone(), entry); + Ok(rhandle) }; if result.is_err() { // clean-up cache on failure @@ -126,8 +167,12 @@ where /// use left_right::{ReadHandle, ReadHandleFactory}; /// use dataplane_left_right_tlcache::make_thread_local_readhandle_cache; /// use dataplane_left_right_tlcache::ReadHandleCache; +/// use dataplane_left_right_tlcache::Identity; /// /// struct LeftRightWrappedType; +/// impl Identity for LeftRightWrappedType { +/// fn identity(&self) -> u32 {0} +/// } /// /// make_thread_local_readhandle_cache!(MYCACHE, u32, LeftRightWrappedType); /// ``` From 24ef74761b5edbec72e960bfad77d1c78f98efa3 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 13:21:20 +0200 Subject: [PATCH 05/35] feat(left-right-tlcache): reduce provider queries Add a version field and provider trait method to avoid querying the provider unless there are changes in the readhandle collection. This is an optimization that replaces a potentially expensive lookup in the provider by fetching a number (version), on every cache check. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index fc1a6ebe3..bc667b6d8 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -46,12 +46,20 @@ pub trait ReadHandleProvider: Sync { /// to give us factories as that shields us from buggy providers returning ReadHandles that are not /// unique per thread. There should be no performance penalty of offering factories instead of read handles /// since factory::handle() is identical to rhandle::clone() - fn get_factory(&self, key: &Self::Key) -> Option<&ReadHandleFactory>; + #[allow(clippy::type_complexity)] + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)>; /// Ask the provider about the identity of T for the `ReadHandle` accessible via some key. /// This is needed for the cache to be able to invalidate entries that point to `ReadHandle`'s /// for T's that should no longer be accessed by that key. fn get_identity(&self, key: &Self::Key) -> Option; + + /// Get version. Provider should promise to provide a distinct value (e.g. monotonically increasing) + /// anytime there's a change in the collection of read handles / factories it owns. + fn get_version(&self) -> u64; } /// Trait to determine the real identity of a `T` wrapped in left-right. That is, @@ -74,10 +82,15 @@ pub enum ReadHandleCacheError { struct ReadHandleEntry { rhandle: Rc>, identity: K, + version: u64, } impl, K: PartialEq> ReadHandleEntry { - fn new(identity: K, rhandle: Rc>) -> Self { - Self { rhandle, identity } + fn new(identity: K, rhandle: Rc>, version: u64) -> Self { + Self { + rhandle, + identity, + version, + } } fn is_valid(&self, key: &K, provider: &impl ReadHandleProvider) -> bool { if self.rhandle.was_dropped() { @@ -95,6 +108,7 @@ impl, K: PartialEq> ReadHandleEntry { if self.identity != identity { return false; } + // this is just extra sanity match self.rhandle.enter() { Some(t) => t.identity() == identity, None => false, @@ -132,22 +146,25 @@ where } let result = { + // get version (*) + let version = provider.get_version(); + // get a factory for the key from the provider and build a fresh handle from it let fresh = provider .get_factory(&key) .map(|factory| factory.handle()) .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; - // get master identity + // get identity let identity = fresh .enter() .ok_or_else(|| ReadHandleCacheError::NotAccessible(key.clone()))? .as_ref() .identity(); - // store a new entry locally with a handle, its identity and the key + // store a new entry locally with a handle, its identity and version, for the given key let rhandle = Rc::new(fresh); - let entry = ReadHandleEntry::new(identity, Rc::clone(&rhandle)); + let entry = ReadHandleEntry::new(identity, Rc::clone(&rhandle), version); map.insert(key.clone(), entry); Ok(rhandle) }; @@ -158,6 +175,10 @@ where result }) } + // (*) This code does not guarantee that between the call of version() and get_factory(), the underlying collection of + // handles owned by the provider hasn't changed. This is fine if we call get_version() first: we may get the latest handle + // with an older version. The next access to the reader will heal this, at the cost of building a new read-handle. A quick + // solution would be for get_factory to return (identity, version, factory). } /// Create a thread-local `ReadHandleCache` with a given name, to access From 58ef94b0365e577eb51f281cba05f27d021ea969 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 14:04:10 +0200 Subject: [PATCH 06/35] feat(left-right-tlcache): avoid potential race Augment get_factory() trait method to return also the identity and version for a given key. This avoids the potential race that could happen due to the fact that the provider is queried twice on each access to an rhandle and nothing prevents the provider contents from changing between the two. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index bc667b6d8..79b43faba 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -146,26 +146,17 @@ where } let result = { - // get version (*) - let version = provider.get_version(); - - // get a factory for the key from the provider and build a fresh handle from it - let fresh = provider + // get a factory for the key from the provider to build a fresh handle from it + // provider returns identity of object and version for entry invalidation + let (factory, identity, version) = provider .get_factory(&key) - .map(|factory| factory.handle()) .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; - // get identity - let identity = fresh - .enter() - .ok_or_else(|| ReadHandleCacheError::NotAccessible(key.clone()))? - .as_ref() - .identity(); - // store a new entry locally with a handle, its identity and version, for the given key - let rhandle = Rc::new(fresh); - let entry = ReadHandleEntry::new(identity, Rc::clone(&rhandle), version); + let rhandle = Rc::new(factory.handle()); + let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); map.insert(key.clone(), entry); + Ok(rhandle) }; if result.is_err() { @@ -175,10 +166,6 @@ where result }) } - // (*) This code does not guarantee that between the call of version() and get_factory(), the underlying collection of - // handles owned by the provider hasn't changed. This is fine if we call get_version() first: we may get the latest handle - // with an older version. The next access to the reader will heal this, at the cost of building a new read-handle. A quick - // solution would be for get_factory to return (identity, version, factory). } /// Create a thread-local `ReadHandleCache` with a given name, to access From 4905cc10056f63f1ae50a777ec68f58c24d741b2 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 15:38:38 +0200 Subject: [PATCH 07/35] feat(left-right-tlcache): avoid duplicate rhandles The thread-local cache stores ReadHandle's based on some key, where the key may match the identity or be an alias. Currently, we store a distinct readhandle per key, regardless of whether the readhandle allows reading from the same object. Exploit the fact that we learn the identity of a T when querying the provider to let cached entries for distinct keys but the same identity share a single ReadHandle. This should expedite internal operations in case there would be many handles. In the sample use case, we'll cache the same ReadHandle for the two keys it may be queried (vrfId or Vni). Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index 79b43faba..b6c841dea 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -157,6 +157,16 @@ where let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); map.insert(key.clone(), entry); + // if the querying key is not the identity, update entry for key = identity. This helps in consistency + // and avoids having duplicate readhandles for the same T, which should expedite checks with many read handles + // if T's are accessed by multiple keys. + if key != identity { + map.remove(&identity); + map.insert( + identity.clone(), + ReadHandleEntry::new(identity, Rc::clone(&rhandle), version), + ); + } Ok(rhandle) }; if result.is_err() { From 936b9ea01a8f3c0666147ac607c8f8b9a1028139 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 20:16:03 +0200 Subject: [PATCH 08/35] feat(left-right-tlcache): filter bad rhandles Avoid caching a ReadHandle if the corresponding WriteHandle has been dropped. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index b6c841dea..ee8021daa 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -152,8 +152,14 @@ where .get_factory(&key) .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; + // obtain handle but don't store it nor return it if there is no writer / data + let rhandle = factory.handle(); + if rhandle.was_dropped() { + return Err(ReadHandleCacheError::NotAccessible(key.clone())); + } + // store a new entry locally with a handle, its identity and version, for the given key - let rhandle = Rc::new(factory.handle()); + let rhandle = Rc::new(rhandle); let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); map.insert(key.clone(), entry); From ab1aa69aa3fc4f7269050354f7e69a5fdebae6a1 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 22:53:57 +0200 Subject: [PATCH 09/35] feat(left-right-tlcache): bad handle removal Ensure that the cache does not keep bad handles. When detecting that an object is no longer readable (the writer has been dropped), remove not only the entry for the key but all entries pointing to handles with the same identity. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 61 ++++++++++++++++------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index ee8021daa..a65c29f26 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -145,41 +145,36 @@ where return Ok(Rc::clone(&entry.rhandle)); } - let result = { - // get a factory for the key from the provider to build a fresh handle from it - // provider returns identity of object and version for entry invalidation - let (factory, identity, version) = provider - .get_factory(&key) - .ok_or_else(|| ReadHandleCacheError::NotFound(key.clone()))?; - - // obtain handle but don't store it nor return it if there is no writer / data - let rhandle = factory.handle(); - if rhandle.was_dropped() { - return Err(ReadHandleCacheError::NotAccessible(key.clone())); - } - - // store a new entry locally with a handle, its identity and version, for the given key - let rhandle = Rc::new(rhandle); - let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); - map.insert(key.clone(), entry); - - // if the querying key is not the identity, update entry for key = identity. This helps in consistency - // and avoids having duplicate readhandles for the same T, which should expedite checks with many read handles - // if T's are accessed by multiple keys. - if key != identity { - map.remove(&identity); - map.insert( - identity.clone(), - ReadHandleEntry::new(identity, Rc::clone(&rhandle), version), - ); - } - Ok(rhandle) - }; - if result.is_err() { - // clean-up cache on failure + // get a factory for the key from the provider to build a fresh handle from it + // provider returns identity of object and version for entry invalidation + let (factory, identity, version) = provider.get_factory(&key).ok_or_else(|| { map.remove(&key); + ReadHandleCacheError::NotFound(key.clone()) + })?; + + // obtain handle but don't store it nor return it if there is no writer / data + let rhandle = factory.handle(); + if rhandle.was_dropped() { + // can remove element with key, but also all which point to the same identity + map.retain(|_key, entry| entry.identity != identity); + return Err(ReadHandleCacheError::NotAccessible(key.clone())); } - result + + // store a new entry locally with a handle, its identity and version, for the given key + let rhandle = Rc::new(rhandle); + let entry = ReadHandleEntry::new(identity.clone(), Rc::clone(&rhandle), version); + map.insert(key.clone(), entry); + + // if the querying key is not the identity, update entry for key = identity. This helps in consistency + // and avoids having duplicate readhandles for the same T, which should expedite checks with many read handles + // if T's are accessed by multiple keys. + if key != identity { + map.insert( + identity.clone(), + ReadHandleEntry::new(identity, Rc::clone(&rhandle), version), + ); + } + Ok(rhandle) }) } } From 882d0e549d00ab38974e3fca334baee8a8b9a673 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 23:40:16 +0200 Subject: [PATCH 10/35] feat(left-right-tlcache): add cache purge method Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index a65c29f26..fbfb0c06a 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -177,6 +177,12 @@ where Ok(rhandle) }) } + + pub fn purge(thread_local: &'static LocalKey) { + thread_local.with(|local| { + local.handles.borrow_mut().clear(); + }); + } } /// Create a thread-local `ReadHandleCache` with a given name, to access From 88b21ece9ff2a1f011dd4650d67462e793db9f3e Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Tue, 14 Oct 2025 16:17:18 +0200 Subject: [PATCH 11/35] feat(left-right-tlcache): add simple tests Add a test provider and several tests. Tests are serialized because they run in the same thread and access the same cache. That could be avoided using distinct caches,though. Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + left-right-tlcache/Cargo.toml | 3 + left-right-tlcache/src/lib.rs | 365 ++++++++++++++++++++++++++++++++++ 3 files changed, 369 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 9e120f6ba..478f5040b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1114,6 +1114,7 @@ version = "0.1.0" dependencies = [ "ahash 0.8.10", "left-right", + "serial_test", "thiserror 2.0.17", ] diff --git a/left-right-tlcache/Cargo.toml b/left-right-tlcache/Cargo.toml index f6b5bc79f..9a58573fa 100644 --- a/left-right-tlcache/Cargo.toml +++ b/left-right-tlcache/Cargo.toml @@ -9,3 +9,6 @@ license = "Apache-2.0" left-right = { workspace = true } ahash = { workspace = true } thiserror = { workspace = true } + +[dev-dependencies] +serial_test = { workspace = true } diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index fbfb0c06a..2b1efcade 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -209,3 +209,368 @@ macro_rules! make_thread_local_readhandle_cache { } }; } + +#[cfg(test)] +mod tests { + #![allow(clippy::collapsible_if)] + + use super::*; + use left_right::{Absorb, ReadHandleFactory, WriteHandle}; + use serial_test::serial; + use std::sync::Mutex; + // Our left-right protected struct + #[derive(Debug, Clone)] + struct TestStruct { + id: u64, + data: String, + } + impl TestStruct { + fn new(id: u64, data: &str) -> Self { + Self { + id, + data: data.to_string(), + } + } + } + // Implement identity for TestStruct + impl Identity for TestStruct { + fn identity(&self) -> u64 { + self.id + } + } + + // Dummy implementation of Absorb for TestStruct + #[derive(Debug)] + enum TestStructChange { + Update(String), + } + impl Absorb for TestStruct { + fn absorb_first(&mut self, op: &mut TestStructChange, _other: &Self) { + match op { + TestStructChange::Update(data) => { + self.data = data.clone(); + } + } + } + fn sync_with(&mut self, first: &Self) { + *self = first.clone(); + } + } + + // create local cache + make_thread_local_readhandle_cache!(TEST_CACHE, u64, TestStruct); + + // ReadHandle "owner" implementing ReadHandleProvider + #[derive(Debug)] + struct TestProviderEntry, TestStructChange> { + id: u64, + factory: ReadHandleFactory, + // writer owning the TestStruct. We use option to easily drop it + // and Mutex to make the provider Sync. + writer: Option>>, + } + impl TestProviderEntry { + fn new( + id: u64, + factory: ReadHandleFactory, + writer: Option>>, + ) -> Self { + Self { + id, + factory, + writer, + } + } + } + #[derive(Debug)] + struct TestProvider { + data: HashMap>, + version: u64, + } + impl TestProvider { + fn new() -> Self { + Self { + data: HashMap::new(), + version: 0, + } + } + fn add_object(&mut self, key: u64, identity: u64) { + if key != identity { + let entry = self.data.get(&identity).unwrap(); + let new = TestProviderEntry::new(identity, entry.factory.clone(), None); + self.data.insert(key, new); + } else { + let object = TestStruct::new(identity, "unset"); + let (w, r) = left_right::new_from_empty(object); + let entry = TestProviderEntry::new(identity, r.factory(), Some(Mutex::new(w))); + self.data.insert(key, entry); + let stored = self.data.get(&key).unwrap(); + assert_eq!(stored.id, identity); + } + self.version = self.version.wrapping_add(1); + } + fn mod_object(&mut self, key: u64, data: &str) { + if let Some(object) = self.data.get_mut(&key) { + if let Some(writer_lock) = &mut object.writer { + #[allow(clippy::mut_mutex_lock)] // lock exists just to make provider Sync + let mut writer = writer_lock.lock().unwrap(); + writer.append(TestStructChange::Update(data.to_owned())); + writer.publish(); + } + } + } + fn drop_writer(&mut self, key: u64) { + if let Some(object) = self.data.get_mut(&key) { + let x = object.writer.take(); + drop(x); + self.version = self.version.wrapping_add(1); + } + } + } + + // Implement trait ReadHandleProvider + impl ReadHandleProvider for TestProvider { + type Data = TestStruct; + type Key = u64; + fn get_version(&self) -> u64 { + self.version + } + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)> { + self.data + .get(key) + .map(|entry| (&entry.factory, entry.id, self.version)) + } + fn get_identity(&self, key: &Self::Key) -> Option { + self.data.get(key).map(|entry| entry.id) + } + } + + #[serial] + #[test] + fn test_readhandle_cache_basic() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider + let mut provider = TestProvider::new(); + provider.add_object(1, 1); + provider.add_object(2, 2); + provider.mod_object(1, "object-1"); + provider.mod_object(2, "object-2"); + + // add alias for 1 + provider.add_object(6000, 1); + + { + let key = 1; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 1); + assert_eq!(obj.data, "object-1"); + } + + { + let key = 2; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2"); + } + + { + // 6000 is alias for 1: should access object with id 1 + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 1); + assert_eq!(obj.data, "object-1"); + } + + TEST_CACHE.with(|x| { + let x = x.handles.borrow(); + assert!(x.contains_key(&6000)); + assert!(x.contains_key(&1)); + assert!(x.contains_key(&2)); + assert_eq!(x.len(), 3); + println!("Test: cache contains entries"); + }); + + println!("Change: Let 6000 be a key for 2 instead of 1"); + provider.add_object(6000, 2); + provider.mod_object(2, "object-2-modified"); + { + let key = 6000; + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2-modified"); + } + { + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, 2); + assert_eq!(obj.data, "object-2-modified"); + } + + println!("Change: drop data for object 1. It should not be accessible"); + provider.drop_writer(1); + { + let key = 1; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + + println!("Change: drop data for object 2: should not be accessible by keys 2 and 6000"); + provider.drop_writer(2); + { + let key = 2; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + { + let key = 6000; + println!("Test: Access to object with key {key}"); + let h = ReadHandleCache::get_reader(&TEST_CACHE, key, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(key))); + } + + // ensure cache is clean + TEST_CACHE.with(|x| { + let x = x.handles.borrow(); + assert!(!x.contains_key(&6000)); + assert!(!x.contains_key(&1)); + assert!(!x.contains_key(&2)); + assert!(x.is_empty()); + println!("Test: cache is empty"); + }); + } + + #[serial] + #[test] + fn test_readhandle_cache_multi_invalidation() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + const NUM_ALIASES: u64 = 10; + + // build provider + let mut provider = TestProvider::new(); + provider.add_object(1, 1); + provider.mod_object(1, "object-1"); + + // add aliases + for k in 1..=NUM_ALIASES { + let alias = 100 + k; + provider.add_object(alias, 1); + } + + // query for identity and all aliases + ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider).unwrap(); + for k in 1..=NUM_ALIASES { + let alias = 100 + k; + ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + } + + // cache should contain NUM_ALIASES + 1 entries + TEST_CACHE + .with(|cache| assert_eq!(cache.handles.borrow().len() as u64, (NUM_ALIASES + 1u64))); + + provider.drop_writer(1); + + // do single query for identity + let h = ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(1))); + + // all entries should have been invalidated + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + + // querying again with key = identity should fail + let h = ReadHandleCache::get_reader(&TEST_CACHE, 1, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(1))); + + // querying again with aliases should fail too. Aliases are 100 + k k=1..=NUM_ALIASES + let alias = 100 + 1; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(alias))); + } + + #[serial] + #[test] + fn test_readhandle_cache() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider and populate it + const NUM_HANDLES: u64 = 1000; + let mut provider = TestProvider::new(); + for id in 0..=NUM_HANDLES { + provider.add_object(id, id); + provider.mod_object(id, format!("object-id-{id}").as_ref()); + provider.add_object(id + NUM_HANDLES + 1, id); + } + + // access all objects by id + for id in 0..=NUM_HANDLES { + let h = ReadHandleCache::get_reader(&TEST_CACHE, id, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + } + + // access all objects by alias + for id in 0..=NUM_HANDLES { + let alias = id + NUM_HANDLES + 1; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + } + + // modify all objects and replace all aliases + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + provider.mod_object(id, format!("modified-id-{id}").as_ref()); + provider.add_object(alias, id); + } + + // access objects with re-assigned aliases + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider).unwrap(); + let x = h.enter().unwrap(); + let obj = x.as_ref(); + assert_eq!(obj.id, id); + assert_eq!(obj.data, format!("modified-id-{id}")); + } + + // invalidate all writers + for id in 0..=NUM_HANDLES { + provider.drop_writer(id); + } + + // access objects from alias: none should be accessible + for id in 0..=NUM_HANDLES { + let alias = 2 * NUM_HANDLES + 1 - id; + let h = ReadHandleCache::get_reader(&TEST_CACHE, alias, &provider); + assert!(h.is_err_and(|e| e == ReadHandleCacheError::NotAccessible(alias))); + } + + // all handles from cache should have been removed as we looked up them all + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + } +} From 5eb0fe22b1157462e4a70d60e875bec3e462ab5f Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Sun, 12 Oct 2025 09:59:22 +0200 Subject: [PATCH 12/35] feat(routing): add FibReaderFactory type A FibReaderFactory object allows creating FibReaders. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index dd56e6052..7af1ecad2 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -5,7 +5,8 @@ #![allow(clippy::collapsible_if)] -use left_right::{Absorb, ReadGuard, ReadHandle, WriteHandle}; +use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; + use std::cell::Ref; use std::net::IpAddr; @@ -380,4 +381,17 @@ impl FibReader { pub fn get_id(&self) -> Option { self.0.enter().map(|fib| fib.get_id()) } + #[must_use] + pub fn factory(&self) -> FibReaderFactory { + FibReaderFactory(self.0.factory()) + } +} + +pub struct FibReaderFactory(ReadHandleFactory); + +impl FibReaderFactory { + #[must_use] + pub fn handle(&self) -> FibReader { + FibReader(self.0.handle()) + } } From 1039017e4497f51b100434669ef60bee46d44520 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Sun, 12 Oct 2025 10:35:53 +0200 Subject: [PATCH 13/35] feat(routing): refactor FibTable & rename FibId This commit contains 3 changes: 1) Rename FibId -> FibKey and clarify usage. The old FibId is used in two ways: one, to identify a Fib; another, to look up fibs in the Fibtable. Rename it to FibKey since, while still used as an Id, the main purpose is to act as a key. Also, change some internal APIs to make sure that Fibs are identified by a single FibKey (based on the vrf id). 2) Restructure FibTable: FibReaders (which wrap ReadHandles) cannot be shared among threads. Instead of storing a FibReader for each Fib, store a FibReaderFactory from which readers can be obtained on a per-thread basis. In this patch, every time a Fib needs to be accessed, a new FibReader is created, which is very inefficient due to the need for locking when creating ReadHandles. Subsequent commits will fix this. We also keep the Id of a Fib along with its factory. This is needed to be able to efficiently implement the traits needed for the readhandle cache. On this, note that: - the fibtable could keep fibreaders as it did, since new can be obtained via cloning. However, keeping instead factories protects us from errorneously handing off a reference to a FibReader to more than a thread. - if we would store fibreaders (and clone them on demand), we would not need to store the Id (since it could be obtained entering the fib via the readhandle), but the number of readers would be increased. 3) Elements in the new FibTable are stored within an Arc. This is to ensure that the compiler automatically determines that FibTable is Sync. 4) Avoid needing to derive/implement Clone() for FibTable by adding a dummy sync_from() Absorb implementation, by publishing on FibTable creation. Signed-off-by: Fredi Raspall --- dataplane/src/packet_processor/ipforward.rs | 18 +-- routing/src/display.rs | 14 +-- routing/src/fib/fibtable.rs | 133 +++++++++++--------- routing/src/fib/fibtype.rs | 52 ++++---- routing/src/interfaces/iftable.rs | 6 +- routing/src/interfaces/iftablerw.rs | 9 +- routing/src/interfaces/interface.rs | 6 +- routing/src/interfaces/mod.rs | 4 +- routing/src/rib/vrf.rs | 6 +- routing/src/rib/vrftable.rs | 67 +++++----- 10 files changed, 176 insertions(+), 139 deletions(-) diff --git a/dataplane/src/packet_processor/ipforward.rs b/dataplane/src/packet_processor/ipforward.rs index 1e4bf4e3d..117f75e69 100644 --- a/dataplane/src/packet_processor/ipforward.rs +++ b/dataplane/src/packet_processor/ipforward.rs @@ -16,7 +16,7 @@ use tracing::{debug, error, trace, warn}; use routing::fib::fibobjects::{EgressObject, FibEntry, PktInstruction}; use routing::fib::fibtable::FibTable; use routing::fib::fibtable::FibTableReader; -use routing::fib::fibtype::FibId; +use routing::fib::fibtype::FibKey; use routing::evpn::Vtep; use routing::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; @@ -56,11 +56,11 @@ impl IpForwarder { /// Forward a [`Packet`] fn forward_packet(&self, packet: &mut Packet, vrfid: VrfId) { let nfi = &self.name; - let fibid = if let Some(dst_vpcd) = packet.get_meta().dst_vpcd { + let fibkey = if let Some(dst_vpcd) = packet.get_meta().dst_vpcd { let VpcDiscriminant::VNI(dst_vni) = dst_vpcd; - FibId::from_vni(dst_vni) + FibKey::from_vni(dst_vni) } else { - FibId::from_vrfid(vrfid) + FibKey::from_vrfid(vrfid) }; /* get destination ip address */ @@ -84,21 +84,21 @@ impl IpForwarder { return; }; /* Lookup the fib which needs to be consulted */ - let Some(fibr) = fibtr.get_fib(&fibid) else { - warn!("{nfi}: Unable to find fib with id {fibid} for vrf {vrfid}"); + let Some(fibr) = fibtr.get_fib(&fibkey) else { + warn!("{nfi}: Unable to find fib with id {fibkey} for vrf {vrfid}"); packet.done(DoneReason::InternalFailure); return; }; /* Read-only access to fib */ let Some(fib) = fibr.enter() else { - warn!("{nfi}: Unable to read from fib {fibid}"); + warn!("{nfi}: Unable to read from fib {fibkey}"); packet.done(DoneReason::InternalFailure); return; }; /* Perform lookup in the fib */ let (prefix, fibentry) = fib.lpm_entry_prefix(packet); if let Some(fibentry) = &fibentry { - debug!("{nfi}: Packet hits prefix {prefix} in fib {fibid}"); + debug!("{nfi}: Packet hits prefix {prefix} in fib {fibkey}"); debug!("{nfi}: Entry is:\n{fibentry}"); /* decrement packet TTL, unless the packet is for us */ @@ -134,7 +134,7 @@ impl IpForwarder { let vni = vxlan.vni(); debug!("{nfi}: DECAPSULATED vxlan packet:\n {packet}"); debug!("{nfi}: Packet comes with vni {vni}"); - let fibid = FibId::from_vni(vni); + let fibid = FibKey::from_vni(vni); let Some(fib) = fibtable.get_fib(&fibid) else { error!("{nfi}: Failed to find fib {fibid} associated to vni {vni}"); packet.done(DoneReason::Unroutable); diff --git a/routing/src/display.rs b/routing/src/display.rs index c5d769fb9..d04e9b444 100644 --- a/routing/src/display.rs +++ b/routing/src/display.rs @@ -8,7 +8,7 @@ use crate::cpi::{CpiStats, CpiStatus, StatsRow}; use crate::fib::fibgroupstore::FibRoute; use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; use crate::fib::fibtable::FibTable; -use crate::fib::fibtype::{Fib, FibId}; +use crate::fib::fibtype::{Fib, FibKey}; use crate::frr::frrmi::{FrrAppliedConfig, Frrmi, FrrmiStats}; use crate::rib::VrfTable; @@ -638,12 +638,12 @@ impl Display for AdjacencyTable { } //========================= Fib ================================// -impl Display for FibId { +impl Display for FibKey { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { match self { - FibId::Id(vrfid) => write!(f, "vrfid: {vrfid}")?, - FibId::Vni(vni) => write!(f, "vni: {vni:?}")?, - FibId::Unset => write!(f, "Unset!")?, + FibKey::Id(vrfid) => write!(f, "vrfid: {vrfid}")?, + FibKey::Vni(vni) => write!(f, "vni: {vni:?}")?, + FibKey::Unset => write!(f, "Unset!")?, } Ok(()) } @@ -700,7 +700,7 @@ impl Display for FibRoute { fn fmt_fib_trie bool>( f: &mut std::fmt::Formatter<'_>, - fibid: FibId, + fibid: FibKey, show_string: &str, trie: &PrefixMapTrie, group_filter: F, @@ -726,7 +726,7 @@ impl Display for Fib { impl Display for FibTable { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { Heading(format!(" Fib Table ({} fibs)", self.len())).fmt(f)?; - for (_fibid, fibr) in self.iter() { + for fibr in self.values().map(|f| f.handle()) { if let Some(fib) = fibr.enter() { write!(f, "{}", *fib)?; } diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 9bdc6e938..f7e67e0e3 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -3,93 +3,111 @@ //! The Fib table, which allows accessing all FIBs -use crate::fib::fibtype::{FibId, FibReader, FibWriter}; +use crate::fib::fibtype::{FibKey, FibReader, FibReaderFactory, FibWriter}; +use crate::rib::vrf::VrfId; + use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use net::vxlan::Vni; use std::collections::BTreeMap; use std::sync::Arc; -use tracing::{debug, error}; +#[allow(unused)] +use tracing::{debug, error, info}; -#[derive(Clone, Default, Debug)] -pub struct FibTable(BTreeMap>); +#[derive(Debug)] +struct FibTableEntry { + id: FibKey, + factory: FibReaderFactory, +} +impl FibTableEntry { + const fn new(id: FibKey, factory: FibReaderFactory) -> Self { + Self { id, factory } + } +} + +#[derive(Default, Debug)] +pub struct FibTable(BTreeMap>); impl FibTable { - /// Add a new Fib ([`FibReader`]) - pub fn add_fib(&mut self, id: FibId, fibr: Arc) { - debug!("Creating FIB with id {id}"); - self.0.insert(id, fibr); - } - /// Del a Fib ([`FibReader`]) - pub fn del_fib(&mut self, id: &FibId) { - debug!("Deleting FIB reference with id {id}"); + /// Register a `Fib` by adding a `FibReaderFactory` for it + fn add_fib(&mut self, id: FibKey, factory: FibReaderFactory) { + info!("Registering Fib with id {id} in the FibTable"); + self.0.insert(id, Arc::new(FibTableEntry::new(id, factory))); + } + /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it + fn del_fib(&mut self, id: &FibKey) { + info!("Unregistering Fib id {id} from the FibTable"); self.0.remove(id); } - /// Register a Fib ([`FibReader`]) with a given [`Vni`] - /// This allows finding the Fib from the [`Vni`] - pub fn register_by_vni(&mut self, id: &FibId, vni: &Vni) { - if let Some(fibr) = self.get_fib(id) { - self.0.insert(FibId::Vni(*vni), fibr.clone()); - debug!("Registered Fib {id} with vni {vni}"); + /// Register an existing `Fib` with a given [`Vni`]. + /// This allows looking up a Fib (`FibReaderFactory`) from a [`Vni`] + fn register_by_vni(&mut self, id: &FibKey, vni: Vni) { + if let Some(entry) = self.get_entry(id) { + self.0.insert(FibKey::from_vni(vni), Arc::clone(entry)); + info!("Registering Fib with id {id} with new vni {vni} in FibTable"); } else { - error!("Failed to register Fib {id} with vni {vni}: no fib"); + error!("Failed to register Fib {id} with vni {vni}: no fib with id {id} found"); } } - - /// Remove any entry referring to the given Vni - pub fn unregister_vni(&mut self, vni: &Vni) { - let id = FibId::Vni(*vni); - debug!("Deleting FIB reference with id {id}"); - self.0.remove(&id); + /// Remove any entry keyed by a [`Vni`] + fn unregister_vni(&mut self, vni: Vni) { + let key = FibKey::from_vni(vni); + info!("Unregistered Fib with vni {vni} from the FibTable"); + self.0.remove(&key); } - /// Get the [`FibReader`] for the fib with the given [`FibId`] + /// Get the entry for the fib with the given [`FibKey`] #[must_use] - pub fn get_fib(&self, id: &FibId) -> Option<&Arc> { - self.0.get(id) + fn get_entry(&self, key: &FibKey) -> Option<&Arc> { + self.0.get(key) } - /// Number of [`FibReader`]s in the fib table + /// Get a [`FibReader`] for the fib with the given [`FibKey`]. The call of this + /// method to handle packets is ===TEMPORARY=== as it creates a new `FibReader` every time. #[must_use] - pub fn len(&self) -> usize { + pub fn get_fib(&self, key: &FibKey) -> Option { + self.get_entry(key).map(|entry| entry.factory.handle()) + } + + #[must_use] + pub(crate) fn len(&self) -> usize { self.0.len() } /// Tell if fib table is empty #[must_use] - pub fn is_empty(&self) -> bool { + #[allow(unused)] + pub(crate) fn is_empty(&self) -> bool { self.0.is_empty() } - /// Provide iterator - pub fn iter(&self) -> impl Iterator)> { - self.0.iter() + /// Provide an iterator of [`FibReaderFactory`]s + pub(crate) fn values(&self) -> impl Iterator { + self.0.values().map(|e| &e.factory) } } enum FibTableChange { - Add((FibId, Arc)), - Del(FibId), - RegisterByVni((FibId, Vni)), + Add((FibKey, FibReaderFactory)), + Del(FibKey), + RegisterByVni((FibKey, Vni)), UnRegisterVni(Vni), } impl Absorb for FibTable { fn absorb_first(&mut self, change: &mut FibTableChange, _: &Self) { match change { - FibTableChange::Add((id, fibr)) => self.add_fib(*id, fibr.clone()), + FibTableChange::Add((id, factory)) => self.add_fib(*id, factory.clone()), FibTableChange::Del(id) => self.del_fib(id), - FibTableChange::RegisterByVni((id, vni)) => self.register_by_vni(id, vni), - FibTableChange::UnRegisterVni(vni) => self.unregister_vni(vni), + FibTableChange::RegisterByVni((id, vni)) => self.register_by_vni(id, *vni), + FibTableChange::UnRegisterVni(vni) => self.unregister_vni(*vni), } } - fn drop_first(self: Box) {} - fn sync_with(&mut self, first: &Self) { - *self = first.clone(); - } + fn sync_with(&mut self, _first: &Self) {} } pub struct FibTableWriter(WriteHandle); impl FibTableWriter { #[must_use] pub fn new() -> (FibTableWriter, FibTableReader) { - let (write, read) = left_right::new::(); + let (mut write, read) = left_right::new::(); + write.publish(); /* avoid needing to impl sync_with() so that no need to impl Clone */ (FibTableWriter(write), FibTableReader(read)) } pub fn enter(&self) -> Option> { @@ -97,26 +115,28 @@ impl FibTableWriter { } #[allow(clippy::arc_with_non_send_sync)] #[must_use] - pub fn add_fib(&mut self, id: FibId, vni: Option) -> (FibWriter, Arc) { - let (fibw, fibr) = FibWriter::new(id); - let fibr_arc = Arc::new(fibr); - self.0.append(FibTableChange::Add((id, fibr_arc.clone()))); + pub fn add_fib(&mut self, vrfid: VrfId, vni: Option) -> FibWriter { + let fibid = FibKey::from_vrfid(vrfid); + let (fibw, fibr) = FibWriter::new(fibid); + self.0.append(FibTableChange::Add((fibid, fibr.factory()))); if let Some(vni) = vni { - self.0.append(FibTableChange::RegisterByVni((id, vni))); + self.0.append(FibTableChange::RegisterByVni((fibid, vni))); } self.0.publish(); - (fibw, fibr_arc) + fibw } - pub fn register_fib_by_vni(&mut self, id: FibId, vni: Vni) { - self.0.append(FibTableChange::RegisterByVni((id, vni))); + pub fn register_fib_by_vni(&mut self, vrfid: VrfId, vni: Vni) { + let fibid = FibKey::from_vrfid(vrfid); + self.0.append(FibTableChange::RegisterByVni((fibid, vni))); self.0.publish(); } pub fn unregister_vni(&mut self, vni: Vni) { self.0.append(FibTableChange::UnRegisterVni(vni)); self.0.publish(); } - pub fn del_fib(&mut self, id: &FibId, vni: Option) { - self.0.append(FibTableChange::Del(*id)); + pub fn del_fib(&mut self, vrfid: VrfId, vni: Option) { + let fibid = FibKey::from_vrfid(vrfid); + self.0.append(FibTableChange::Del(fibid)); if let Some(vni) = vni { self.0.append(FibTableChange::UnRegisterVni(vni)); } @@ -136,10 +156,11 @@ impl FibTableReaderFactory { #[derive(Clone, Debug)] pub struct FibTableReader(ReadHandle); impl FibTableReader { - /// Access the fib table from its reader + #[must_use] pub fn enter(&self) -> Option> { self.0.enter() } + #[must_use] pub fn factory(&self) -> FibTableReaderFactory { FibTableReaderFactory(self.0.factory()) } diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 7af1ecad2..dd7335727 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -26,33 +26,35 @@ use crate::rib::vrf::VrfId; use tracing::{debug, error, info, warn}; #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] -/// An id we use to idenfify a FIB -pub enum FibId { + +/// A type used to access a [`Fib`] or to identify it. +/// As an identifier, only the variant `FibKey::Id` is allowed. +pub enum FibKey { Unset, Id(VrfId), Vni(Vni), } -impl FibId { +impl FibKey { #[must_use] pub fn from_vrfid(vrfid: VrfId) -> Self { - FibId::Id(vrfid) + FibKey::Id(vrfid) } #[must_use] pub fn from_vni(vni: Vni) -> Self { - FibId::Vni(vni) + FibKey::Vni(vni) } #[must_use] pub fn as_u32(&self) -> u32 { match self { - FibId::Id(value) => *value, - FibId::Vni(value) => value.as_u32(), - FibId::Unset => unreachable!(), + FibKey::Id(value) => *value, + FibKey::Vni(value) => value.as_u32(), + FibKey::Unset => unreachable!(), } } } pub struct Fib { - id: FibId, + id: FibKey, routesv4: PrefixMapTrie, routesv6: PrefixMapTrie, groupstore: FibGroupStore, @@ -62,7 +64,7 @@ pub struct Fib { impl Default for Fib { fn default() -> Self { let mut fib = Self { - id: FibId::Unset, + id: FibKey::Unset, routesv4: PrefixMapTrie::create(), routesv6: PrefixMapTrie::create(), groupstore: FibGroupStore::new(), @@ -80,16 +82,20 @@ pub type FibRouteV4Filter = Box bool>; pub type FibRouteV6Filter = Box bool>; impl Fib { - /// Set the [`FibId`] for this [`Fib`] - fn set_id(&mut self, id: FibId) { + /// Set the id for this [`Fib`] + fn set_id(&mut self, id: FibKey) { + if !matches!(id, FibKey::Id(_)) { + panic!("Attempting to set invalid Id of {id} to fib"); + } self.id = id; } #[must_use] - /// Get the [`FibId`] for this [`Fib`] - pub fn get_id(&self) -> FibId { - if self.id == FibId::Unset { - panic!("Hit fib with unset FibId!!"); + /// Get the id for this [`Fib`] + pub fn get_id(&self) -> FibKey { + if !matches!(self.id, FibKey::Id(_)) { + error!("Hit fib with invalid Id {}", self.id); + unreachable!() } self.id } @@ -295,9 +301,8 @@ impl Absorb for Fib { FibChange::SetVtep(vtep) => self.set_vtep(vtep), } } - fn drop_first(self: Box) {} fn sync_with(&mut self, first: &Self) { - assert!(self.id != FibId::Unset); + assert!(self.id != FibKey::Unset); assert_eq!(self.id, first.id); debug!("Internal LR state for fib {} is now synced", self.id); } @@ -307,9 +312,9 @@ pub struct FibWriter(WriteHandle); impl FibWriter { /// create a fib, providing a writer and a reader #[must_use] - pub fn new(id: FibId) -> (FibWriter, FibReader) { + pub fn new(id: FibKey) -> (FibWriter, FibReader) { let (mut w, r) = left_right::new::(); - // Set the FibId in the read and write copies, created Fib::default() that sets it to FibId::Unset. + // Set the Id in the read and write copies, created Fib::default() that sets it to FibKey::Unset. unsafe { // It is safe to call raw_handle() and raw_write_handle() here let fib_rcopy = r.raw_handle().unwrap_or_else(|| unreachable!()).as_mut(); @@ -319,13 +324,14 @@ impl FibWriter { // this is needed to avoid needing to clone the fib w.publish(); } + info!("Created Fib with id {id}"); (FibWriter(w), FibReader(r)) } pub fn enter(&self) -> Option> { self.0.enter() } #[must_use] - pub fn get_id(&self) -> Option { + pub fn get_id(&self) -> Option { self.0.enter().map(|fib| fib.get_id()) } pub fn register_fibgroup(&mut self, key: &NhopKey, fibgroup: &FibGroup, publish: bool) { @@ -378,7 +384,7 @@ impl FibReader { pub fn enter(&self) -> Option> { self.0.enter() } - pub fn get_id(&self) -> Option { + pub fn get_id(&self) -> Option { self.0.enter().map(|fib| fib.get_id()) } #[must_use] @@ -386,7 +392,7 @@ impl FibReader { FibReaderFactory(self.0.factory()) } } - +#[derive(Debug, Clone)] pub struct FibReaderFactory(ReadHandleFactory); impl FibReaderFactory { diff --git a/routing/src/interfaces/iftable.rs b/routing/src/interfaces/iftable.rs index 5d4ca7245..19573d7eb 100644 --- a/routing/src/interfaces/iftable.rs +++ b/routing/src/interfaces/iftable.rs @@ -4,7 +4,7 @@ //! A table of interfaces use crate::errors::RouterError; -use crate::fib::fibtype::{FibId, FibReader}; +use crate::fib::fibtype::{FibKey, FibReader}; use crate::interfaces::interface::{IfAddress, IfState, Interface, RouterInterfaceConfig}; use ahash::RandomState; use std::collections::HashMap; @@ -151,9 +151,9 @@ impl IfTable { } ////////////////////////////////////////////////////////////////////// - /// Detach all interfaces attached to the Vrf whose fib has id `FibId` + /// Detach all interfaces attached to the Vrf whose fib has the given Id ////////////////////////////////////////////////////////////////////// - pub fn detach_interfaces_from_vrf(&mut self, fibid: FibId) { + pub fn detach_interfaces_from_vrf(&mut self, fibid: FibKey) { for iface in self.by_index.values_mut() { iface.detach_from_fib(fibid); } diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index cb22287c0..77bb8fb53 100644 --- a/routing/src/interfaces/iftablerw.rs +++ b/routing/src/interfaces/iftablerw.rs @@ -4,7 +4,7 @@ //! Interface to the interfaces module use crate::errors::RouterError; -use crate::fib::fibtype::FibId; +use crate::fib::fibtype::FibKey; use crate::fib::fibtype::FibReader; use crate::interfaces::iftable::IfTable; use crate::interfaces::interface::{IfAddress, IfState, RouterInterfaceConfig}; @@ -20,7 +20,7 @@ enum IfTableChange { Del(InterfaceIndex), Attach((InterfaceIndex, FibReader)), Detach(InterfaceIndex), - DetachFromVrf(FibId), + DetachFromVrf(FibKey), AddIpAddress((InterfaceIndex, IfAddress)), DelIpAddress((InterfaceIndex, IfAddress)), UpdateOpState((InterfaceIndex, IfState)), @@ -168,8 +168,9 @@ impl IfTableWriter { self.0.append(IfTableChange::Detach(ifindex)); self.0.publish(); } - pub fn detach_interfaces_from_vrf(&mut self, fibid: FibId) { - self.0.append(IfTableChange::DetachFromVrf(fibid)); + pub fn detach_interfaces_from_vrf(&mut self, vrfid: VrfId) { + self.0 + .append(IfTableChange::DetachFromVrf(FibKey::Id(vrfid))); self.0.publish(); } } diff --git a/routing/src/interfaces/interface.rs b/routing/src/interfaces/interface.rs index 16dd63ebe..ac03bf71e 100644 --- a/routing/src/interfaces/interface.rs +++ b/routing/src/interfaces/interface.rs @@ -5,7 +5,7 @@ #![allow(clippy::collapsible_if)] -use crate::fib::fibtype::{FibId, FibReader}; +use crate::fib::fibtype::{FibKey, FibReader}; use crate::rib::vrf::VrfId; use net::eth::mac::Mac; use net::interface::{InterfaceIndex, Mtu}; @@ -210,7 +210,7 @@ impl Interface { /// Tell if an [`Interface`] is attached to a Fib with the given Id ////////////////////////////////////////////////////////////////// #[must_use] - pub fn is_attached_to_fib(&self, fibid: FibId) -> bool { + pub fn is_attached_to_fib(&self, fibid: FibKey) -> bool { if let Some(Attachment::VRF(fibr)) = &self.attachment { fibr.get_id() == Some(fibid) } else { @@ -221,7 +221,7 @@ impl Interface { ////////////////////////////////////////////////////////////////// /// Detach an [`Interface`] from VRF if the associated fib has the given id ////////////////////////////////////////////////////////////////// - pub fn detach_from_fib(&mut self, fibid: FibId) { + pub fn detach_from_fib(&mut self, fibid: FibKey) { self.attachment.take_if(|attachment| { if let Attachment::VRF(fibr) = &attachment { if fibr.get_id() == Some(fibid) { diff --git a/routing/src/interfaces/mod.rs b/routing/src/interfaces/mod.rs index 70208789d..7929f0b20 100644 --- a/routing/src/interfaces/mod.rs +++ b/routing/src/interfaces/mod.rs @@ -10,7 +10,7 @@ pub mod interface; #[cfg(test)] pub mod tests { use crate::RouterError; - use crate::fib::fibtype::{FibId, FibWriter}; + use crate::fib::fibtype::{FibKey, FibWriter}; use crate::interfaces::iftable::IfTable; use crate::interfaces::iftablerw::{IfTableReader, IfTableWriter}; use crate::interfaces::interface::{ @@ -113,7 +113,7 @@ pub mod tests { let mut iftable = build_test_iftable(); /* Create a fib for the vrf created next */ - let (fibw, _fibr) = FibWriter::new(FibId::Id(0)); + let (fibw, _fibr) = FibWriter::new(FibKey::Id(0)); /* Create a VRF for that fib */ let vrf_cfg = RouterVrfConfig::new(0, "default"); diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index da6905f17..936270ec1 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -15,7 +15,7 @@ use crate::pretty_utils::Frame; use super::nexthop::{FwAction, Nhop, NhopKey, NhopStore}; use crate::evpn::{RmacStore, Vtep}; -use crate::fib::fibtype::{FibId, FibReader, FibWriter}; +use crate::fib::fibtype::{FibKey, FibReader, FibWriter}; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; use net::route::RouteTableId; @@ -256,9 +256,9 @@ impl Vrf { } //////////////////////////////////////////////////////////////////////// - /// Get the `FibId` of the Fib associated to this [`Vrf`] + /// Get the id (`FibKey`) of the Fib associated to this [`Vrf`] ///////////////////////////////////////////////////////////////////////// - pub fn get_vrf_fibid(&self) -> Option { + pub fn get_vrf_fibid(&self) -> Option { self.get_vrf_fibr()?.get_id() } diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index c992e8106..9c03c5678 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -7,7 +7,7 @@ use crate::RouterError; use crate::evpn::RmacStore; use crate::fib::fibtable::FibTableWriter; -use crate::fib::fibtype::FibId; +use crate::fib::fibtype::FibKey; use crate::interfaces::iftablerw::IfTableWriter; use crate::rib::vrf::{RouterVrfConfig, Vrf, VrfId}; @@ -72,7 +72,7 @@ impl VrfTable { } /* create fib */ - let (fibw, _) = self.fibtablew.add_fib(FibId::Id(vrf.vrfid), vrf.vni); + let fibw = self.fibtablew.add_fib(vrf.vrfid, vrf.vni); vrf.set_fibw(fibw); /* store */ @@ -107,9 +107,8 @@ impl VrfTable { /* register vni */ self.by_vni.insert(vni, vrfid); - /* register fib */ - self.fibtablew - .register_fib_by_vni(FibId::from_vrfid(vrfid), vni); + /* make fib accessible from vni */ + self.fibtablew.register_fib_by_vni(vrfid, vni); Ok(()) } @@ -135,33 +134,44 @@ impl VrfTable { /// a [`Vni`] configured or it does but the internal state is not the expected. /////////////////////////////////////////////////////////////////////////////////// pub fn check_vni(&self, vrfid: VrfId) -> Result<(), RouterError> { + // lookup vrf let vrf = self.get_vrf(vrfid)?; + + // Vrf must have a vni configured let Some(vni) = &vrf.vni else { return Err(RouterError::Internal("No vni found")); }; + + // must be able to look it up by vni let found = self.get_vrfid_by_vni(*vni)?; if found != vrfid { error!("Vni {vni} refers to vrfid {found} and not {vrfid}"); return Err(RouterError::Internal("Inconsistent vni mapping")); } - // look up fib -- from fibtable + + // access fib fibtable let fibtable = self .fibtablew .enter() .ok_or(RouterError::Internal("Failed to access fib table"))?; - let fib = fibtable - .get_fib(&FibId::Vni(*vni)) + + // lookup fib from vni must succeed + let fibr = fibtable + .get_fib(&FibKey::Vni(*vni)) .ok_or(RouterError::Internal("No fib for vni found"))?; - let fib = fib + + // access the fib + let fib = fibr .enter() .ok_or(RouterError::Internal("Unable to read fib"))?; + let found_fibid = fib.get_id(); - // look up fib - direct (TODO: make fib mandatory for VRF) if let Some(fibw) = &vrf.fibw { let fib = fibw .enter() .ok_or(RouterError::Internal("Unable to access Fib for vrf"))?; + let fibid = fib.get_id(); if fibid != found_fibid { error!("Expected: {found_fibid} found: {fibid}"); @@ -186,10 +196,9 @@ impl VrfTable { }; // delete the corresponding fib if vrf.fibw.is_some() { - let fib_id = FibId::Id(vrfid); - debug!("Requesting deletion of vrf {vrfid} FIB. Id is '{fib_id}'"); - self.fibtablew.del_fib(&fib_id, vrf.vni); - iftablew.detach_interfaces_from_vrf(fib_id); + debug!("Requesting deletion of vrf {vrfid} FIB"); + self.fibtablew.del_fib(vrfid, vrf.vni); + iftablew.detach_interfaces_from_vrf(vrfid); } // if the VRF had a vni assigned, unregister it @@ -420,7 +429,7 @@ impl VrfTable { mod tests { use super::*; use crate::fib::fibobjects::{EgressObject, PktInstruction}; - use crate::fib::fibtype::FibId; + use crate::fib::fibtype::FibKey; use crate::interfaces::tests::build_test_iftable_left_right; use crate::pretty_utils::Frame; use crate::rib::encapsulation::Encapsulation; @@ -522,7 +531,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth0 = ift.get_interface(idx2).expect("Should find interface"); - assert!(eth0.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth0.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -534,7 +543,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth1 = ift.get_interface(idx3).expect("Should find interface"); - assert!(eth1.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth1.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -546,7 +555,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let eth2 = ift.get_interface(idx4).expect("Should find interface"); - assert!(eth2.is_attached_to_fib(FibId::Id(vrfid))); + assert!(eth2.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -558,7 +567,7 @@ mod tests { .expect("Should succeed"); let ift = iftr.enter().unwrap(); let iface = ift.get_interface(idx5).expect("Should find interface"); - assert!(iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(iface.is_attached_to_fib(FibKey::Id(vrfid))); println!("{}", *ift); drop(ift); @@ -576,10 +585,10 @@ mod tests { println!("{vrftable}"); let ift = iftr.enter().unwrap(); let iface = ift.get_interface(idx4).expect("Should be there"); - assert!(!iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!iface.is_attached_to_fib(FibKey::Id(vrfid))); assert!(iface.attachment.is_none()); let iface = ift.get_interface(idx5).expect("Should be there"); - assert!(!iface.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!iface.is_attached_to_fib(FibKey::Id(vrfid))); assert!(iface.attachment.is_none()); println!("{}", *ift); drop(ift); @@ -608,10 +617,10 @@ mod tests { ); let ift = iftr.enter().unwrap(); let eth0 = ift.get_interface(idx2).expect("Should be there"); - assert!(!eth0.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!eth0.is_attached_to_fib(FibKey::Id(vrfid))); assert!(eth0.attachment.is_none()); let eth1 = ift.get_interface(idx3).expect("Should be there"); - assert!(!eth1.is_attached_to_fib(FibId::Id(vrfid))); + assert!(!eth1.is_attached_to_fib(FibKey::Id(vrfid))); assert!(eth1.attachment.is_none()); println!("{}", *ift); drop(ift); @@ -665,9 +674,9 @@ mod tests { assert_eq!(id, vrfid); debug!("\n{vrftable}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); + let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); + let fib = fibtable.get_fib(&FibKey::from_vni(vni)); assert!(fib.is_some()); } @@ -681,9 +690,9 @@ mod tests { assert!((id.is_err_and(|e| e == RouterError::NoSuchVrf))); debug!("\n{vrftable}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); + let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); + let fib = fibtable.get_fib(&FibKey::from_vni(vni)); assert!(fib.is_none()); } } @@ -740,9 +749,9 @@ mod tests { // check fib table if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibId::from_vrfid(vrfid)); + let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); assert!(fib.is_none()); - let fib = fibtable.get_fib(&FibId::from_vni(vni)); + let fib = fibtable.get_fib(&FibKey::from_vni(vni)); assert!(fib.is_none()); assert_eq!(fibtable.len(), 1); } From 7949efb5edf436d52eeffef9dc2e243d606bb7e8 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Sun, 12 Oct 2025 19:00:04 +0200 Subject: [PATCH 14/35] feat(routing): impl Hash for Fib Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index dd7335727..f8c89c97e 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -8,6 +8,7 @@ use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use std::cell::Ref; +use std::hash::Hash; use std::net::IpAddr; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; @@ -25,8 +26,7 @@ use crate::rib::vrf::VrfId; #[allow(unused)] use tracing::{debug, error, info, warn}; -#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq)] - +#[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] /// A type used to access a [`Fib`] or to identify it. /// As an identifier, only the variant `FibKey::Id` is allowed. pub enum FibKey { @@ -60,7 +60,15 @@ pub struct Fib { groupstore: FibGroupStore, vtep: Vtep, } - +impl Hash for Fib { + // We implement explicitly `std::hash::Hash` for `Fib` instead of deriving it because: + // - this avoids the need to implement/derive it for all internal components + // - it is actually not possible to do so since some types are defined externally (prefixes) + // - the Id suffices to identify them and the implementation is possibly faster. + fn hash(&self, state: &mut H) { + self.id.hash(state); + } +} impl Default for Fib { fn default() -> Self { let mut fib = Self { @@ -375,6 +383,7 @@ impl FibWriter { } #[derive(Clone, Debug)] +#[repr(transparent)] pub struct FibReader(ReadHandle); impl FibReader { #[must_use] @@ -393,7 +402,7 @@ impl FibReader { } } #[derive(Debug, Clone)] -pub struct FibReaderFactory(ReadHandleFactory); +pub struct FibReaderFactory(pub(crate) ReadHandleFactory); impl FibReaderFactory { #[must_use] From 6b5c07b5e252c8350b401124d576c4aca1360a86 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Mon, 13 Oct 2025 11:17:29 +0200 Subject: [PATCH 15/35] feat(routing): impl AsRef for ReadHandle The thread-local cache of prior commit stores ReadHandle. However, the rest of the code uses transparent wrapper FibReader, with additional methods. Implement AsRef so that we can have FibReaders with zero cost. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index f8c89c97e..1458c5ba7 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -401,6 +401,16 @@ impl FibReader { FibReaderFactory(self.0.factory()) } } + +// make FibReader a zero-cost wrap of ReadHandle +impl AsRef for ReadHandle { + #[inline] + fn as_ref(&self) -> &FibReader { + // safe because FibReader is repr(transparent) wrapper of ReadHandle + unsafe { &*(self as *const ReadHandle as *const FibReader) } + } +} + #[derive(Debug, Clone)] pub struct FibReaderFactory(pub(crate) ReadHandleFactory); From a10c3915fb841c30745c31c01b6a11f4e887079b Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Mon, 13 Oct 2025 19:04:36 +0200 Subject: [PATCH 16/35] feat(routing): impl Identity for Fib Add left-right-tlcache as dependency to implement the fibtable cache. Implement trait Identity for Fib so that the thread-local cache of ReadHandles can determine if a read handle points to the right object or the entry needs to be invalidated. Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + routing/Cargo.toml | 1 + routing/src/fib/fibtype.rs | 6 ++++++ 3 files changed, 8 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index 478f5040b..2469cd528 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1280,6 +1280,7 @@ dependencies = [ "chrono", "dataplane-cli", "dataplane-config", + "dataplane-left-right-tlcache", "dataplane-lpm", "dataplane-net", "dataplane-tracectl", diff --git a/routing/Cargo.toml b/routing/Cargo.toml index 7cde0afd5..f984ccce8 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -13,6 +13,7 @@ testing = [] cli = { workspace = true } config = { workspace = true } dplane-rpc = { workspace = true } +left-right-tlcache = { workspace = true } lpm = { workspace = true } net = { workspace = true } tracectl = { workspace = true } diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 1458c5ba7..71c620b16 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -6,6 +6,7 @@ #![allow(clippy::collapsible_if)] use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; +use left_right_tlcache::Identity; use std::cell::Ref; use std::hash::Hash; @@ -69,6 +70,11 @@ impl Hash for Fib { self.id.hash(state); } } +impl Identity for Fib { + fn identity(&self) -> FibKey { + self.get_id() + } +} impl Default for Fib { fn default() -> Self { let mut fib = Self { From aaa6137699c063477133babf4399069d6209d3c0 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 13:31:13 +0200 Subject: [PATCH 17/35] feat(routing): add version to FibTable Versioning the fibtable will allow us to determine the validity of cached readhandles without needing to ask the provider, which would add an extra lookup (e.g. hash or similar), on every cache lookup. This patch adds the version for the FibTable and the logic to monotonically increment its value anytime it is updated. Note that version is u64 instead of AtomicU64 since we don't need to synchronize multiple threads on its value. Signed-off-by: Fredi Raspall --- routing/src/display.rs | 7 ++++++- routing/src/fib/fibtable.rs | 28 +++++++++++++++++++--------- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/routing/src/display.rs b/routing/src/display.rs index d04e9b444..aac612f76 100644 --- a/routing/src/display.rs +++ b/routing/src/display.rs @@ -725,7 +725,12 @@ impl Display for Fib { } impl Display for FibTable { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - Heading(format!(" Fib Table ({} fibs)", self.len())).fmt(f)?; + Heading(format!( + " Fib Table ({} fibs, version: {})", + self.len(), + self.version() + )) + .fmt(f)?; for fibr in self.values().map(|f| f.handle()) { if let Some(fib) = fibr.enter() { write!(f, "{}", *fib)?; diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index f7e67e0e3..c309a94ca 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -25,24 +25,29 @@ impl FibTableEntry { } #[derive(Default, Debug)] -pub struct FibTable(BTreeMap>); +pub struct FibTable { + version: u64, + entries: BTreeMap>, +} impl FibTable { /// Register a `Fib` by adding a `FibReaderFactory` for it fn add_fib(&mut self, id: FibKey, factory: FibReaderFactory) { info!("Registering Fib with id {id} in the FibTable"); - self.0.insert(id, Arc::new(FibTableEntry::new(id, factory))); + self.entries + .insert(id, Arc::new(FibTableEntry::new(id, factory))); } /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it fn del_fib(&mut self, id: &FibKey) { info!("Unregistering Fib id {id} from the FibTable"); - self.0.remove(id); + self.entries.remove(id); } /// Register an existing `Fib` with a given [`Vni`]. /// This allows looking up a Fib (`FibReaderFactory`) from a [`Vni`] fn register_by_vni(&mut self, id: &FibKey, vni: Vni) { if let Some(entry) = self.get_entry(id) { - self.0.insert(FibKey::from_vni(vni), Arc::clone(entry)); + self.entries + .insert(FibKey::from_vni(vni), Arc::clone(entry)); info!("Registering Fib with id {id} with new vni {vni} in FibTable"); } else { error!("Failed to register Fib {id} with vni {vni}: no fib with id {id} found"); @@ -52,13 +57,13 @@ impl FibTable { fn unregister_vni(&mut self, vni: Vni) { let key = FibKey::from_vni(vni); info!("Unregistered Fib with vni {vni} from the FibTable"); - self.0.remove(&key); + self.entries.remove(&key); } /// Get the entry for the fib with the given [`FibKey`] #[must_use] fn get_entry(&self, key: &FibKey) -> Option<&Arc> { - self.0.get(key) + self.entries.get(key) } /// Get a [`FibReader`] for the fib with the given [`FibKey`]. The call of this /// method to handle packets is ===TEMPORARY=== as it creates a new `FibReader` every time. @@ -69,17 +74,21 @@ impl FibTable { #[must_use] pub(crate) fn len(&self) -> usize { - self.0.len() + self.entries.len() } /// Tell if fib table is empty #[must_use] #[allow(unused)] pub(crate) fn is_empty(&self) -> bool { - self.0.is_empty() + self.entries.is_empty() } /// Provide an iterator of [`FibReaderFactory`]s pub(crate) fn values(&self) -> impl Iterator { - self.0.values().map(|e| &e.factory) + self.entries.values().map(|e| &e.factory) + } + /// Tell version of this [`FibTable`] + pub(crate) fn version(&self) -> u64 { + self.version } } @@ -92,6 +101,7 @@ enum FibTableChange { impl Absorb for FibTable { fn absorb_first(&mut self, change: &mut FibTableChange, _: &Self) { + self.version = self.version.wrapping_add(1); match change { FibTableChange::Add((id, factory)) => self.add_fib(*id, factory.clone()), FibTableChange::Del(id) => self.del_fib(id), From 46251e69b8e4c64378e4967ac501bc681fc345b6 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 13:49:54 +0200 Subject: [PATCH 18/35] feat(routing): AsRef for FibReaderFactory Implement AsRef> for FibReaderFactory so that it can be treated as a ReadHandleFactory. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 71c620b16..8f01367d4 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -418,8 +418,17 @@ impl AsRef for ReadHandle { } #[derive(Debug, Clone)] +#[repr(transparent)] pub struct FibReaderFactory(pub(crate) ReadHandleFactory); +// make FibReaderFactory a zero-cost wrap of ReadHandleFactory +impl AsRef> for FibReaderFactory { + #[inline] + fn as_ref(&self) -> &ReadHandleFactory { + &self.0 + } +} + impl FibReaderFactory { #[must_use] pub fn handle(&self) -> FibReader { From 3edf9f853d34258d5a1aebd7dfbee64dab660ca7 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 13:51:46 +0200 Subject: [PATCH 19/35] feat(routing): implement fibtable cache Declare thread-local ReadHandleCache `FIBTABLE_CACHE`. Implement trait ReadHandleProvider for FibTable so that it can be polled by the cache, per thread. We could implement ReadHandleProvider for FibTableReader instead. However, implementing it for the FibTable is more general in that it would also work if the FibTable itself was not wrapped in left- right. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtable.rs | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index c309a94ca..a063222e1 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -178,3 +178,32 @@ impl FibTableReader { #[allow(unsafe_code)] unsafe impl Send for FibTableWriter {} + +/* + * Thread-local cache or readhandles for the fibtable + */ + +// declare thread-local cache for fibtable +use crate::fib::fibtype::Fib; +use left_right_tlcache::make_thread_local_readhandle_cache; +use left_right_tlcache::{ReadHandleCache, ReadHandleProvider}; +make_thread_local_readhandle_cache!(FIBTABLE_CACHE, FibKey, Fib); + +impl ReadHandleProvider for FibTable { + type Data = Fib; + type Key = FibKey; + fn get_factory( + &self, + key: &Self::Key, + ) -> Option<(&ReadHandleFactory, Self::Key, u64)> { + let entry = self.get_entry(key)?.as_ref(); + let factory = entry.factory.as_ref(); + Some((factory, entry.id, self.version)) + } + fn get_version(&self) -> u64 { + self.version + } + fn get_identity(&self, key: &Self::Key) -> Option { + self.get_entry(key).map(|entry| entry.id) + } +} From 0142fae276a0a3e8baac52ec3c87802eb1ef2dfb Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 16:21:46 +0200 Subject: [PATCH 20/35] feat(routing): method to get Rc ... from Rc>. The cache stores ReadHandle's for some T. In the case of the FibTable, T=Fib and we want to hand off FibReaders (a thin wrapper of ReadHandle) to the threads using the cache. Add a method to perform such a conversion. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 8f01367d4..4ebe46e14 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -11,6 +11,7 @@ use left_right_tlcache::Identity; use std::cell::Ref; use std::hash::Hash; use std::net::IpAddr; +use std::rc::Rc; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; @@ -399,6 +400,17 @@ impl FibReader { pub fn enter(&self) -> Option> { self.0.enter() } + #[inline(always)] + /// Convert Rc> -> FibReader + /// We need this conversion because the cache of read-handles stores ReadHandle, + /// but the FibTable provides FibReader. + pub(crate) fn rc_from_rc_rhandle(rc: Rc>) -> Rc { + unsafe { + // the conversion is safe because FibReader is a transparent wrapper of ReadHandle + let ptr = Rc::into_raw(rc) as *const Self; + Rc::from_raw(ptr) + } + } pub fn get_id(&self) -> Option { self.0.enter().map(|fib| fib.get_id()) } @@ -416,7 +428,6 @@ impl AsRef for ReadHandle { unsafe { &*(self as *const ReadHandle as *const FibReader) } } } - #[derive(Debug, Clone)] #[repr(transparent)] pub struct FibReaderFactory(pub(crate) ReadHandleFactory); From 48c8fb3d07a18ece6a86b8e9d4ae99ec87c1a448 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 16:25:27 +0200 Subject: [PATCH 21/35] feat(routing): method to get FibReaders from cache Add the main method to lookup FibReaders from a FibKey from the thread-local cache and a FibTableReader. The method will check the local cache to return FibReaders owned by the calling thread. On failure, it will pull data from provider (fibtable) accessible from the same FibTableReader. Signed-off-by: Fredi Raspall --- routing/src/errors.rs | 7 +++++++ routing/src/fib/fibtable.rs | 23 +++++++++++++++++++++-- 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/routing/src/errors.rs b/routing/src/errors.rs index 34e3759ba..f23894183 100644 --- a/routing/src/errors.rs +++ b/routing/src/errors.rs @@ -3,6 +3,7 @@ //! The error results used by this library. +use crate::fib::fibtype::FibKey; use net::interface::InterfaceIndex; use thiserror::Error; @@ -40,4 +41,10 @@ pub enum RouterError { #[error("Invalid configuration: {0}")] InvalidConfig(&'static str), + + #[error("Fibtable is not accessible")] + FibTableError, + + #[error("Fib error: {0}")] + FibError(#[from] left_right_tlcache::ReadHandleCacheError), } diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index a063222e1..9aef74973 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -3,15 +3,19 @@ //! The Fib table, which allows accessing all FIBs -use crate::fib::fibtype::{FibKey, FibReader, FibReaderFactory, FibWriter}; use crate::rib::vrf::VrfId; +use crate::{ + RouterError, + fib::fibtype::{FibKey, FibReader, FibReaderFactory, FibWriter}, +}; use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use net::vxlan::Vni; use std::collections::BTreeMap; +use std::rc::Rc; use std::sync::Arc; #[allow(unused)] -use tracing::{debug, error, info}; +use tracing::{debug, error, info, warn}; #[derive(Debug)] struct FibTableEntry { @@ -207,3 +211,18 @@ impl ReadHandleProvider for FibTable { self.get_entry(key).map(|entry| entry.id) } } + +impl FibTableReader { + /// Main method for threads to get a reference to a FibReader from their thread-local cache. + /// Note 1: the cache stores `ReadHandle`'s. This method returns `FibReader` for convenience. This is zero cost + /// Note 2: we make this a method of [`FibTableReader`], as each thread is assumed to have its own read handle to the `FibTable`. + /// Note 3: we map ReadHandleCacheError to RouterError + pub fn get_fib_reader(&self, id: FibKey) -> Result, RouterError> { + let Some(fibtable) = self.enter() else { + warn!("Unable to access fib table!"); + return Err(RouterError::FibTableError); + }; + let rhandle = ReadHandleCache::get_reader(&FIBTABLE_CACHE, id, &*fibtable)?; + Ok(FibReader::rc_from_rc_rhandle(rhandle)) + } +} From b8144fadc50e4ea1b6c7a7603292b8af09b12073 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 17:11:21 +0200 Subject: [PATCH 22/35] feat(dataplane): lookup fibreader from cache Let the ipforward NF look up FibReader from thread-local cache. Signed-off-by: Fredi Raspall --- dataplane/src/packet_processor/ipforward.rs | 46 ++++++++------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/dataplane/src/packet_processor/ipforward.rs b/dataplane/src/packet_processor/ipforward.rs index 117f75e69..95819850b 100644 --- a/dataplane/src/packet_processor/ipforward.rs +++ b/dataplane/src/packet_processor/ipforward.rs @@ -14,7 +14,6 @@ use std::net::IpAddr; use tracing::{debug, error, trace, warn}; use routing::fib::fibobjects::{EgressObject, FibEntry, PktInstruction}; -use routing::fib::fibtable::FibTable; use routing::fib::fibtable::FibTableReader; use routing::fib::fibtype::FibKey; @@ -71,30 +70,18 @@ impl IpForwarder { }; debug!("{nfi}: processing packet to {dst} with vrf {vrfid}"); - /* Get the fib to use: this lookup could be avoided since - we know the interface the packet came from and it has to be - attached to a certain fib if the vrf metadata value was set. - This extra lookup is a side effect of splitting into stages. - */ - - /* Read-only access to the fib table */ - let Some(fibtr) = self.fibtr.enter() else { - error!("{nfi}: Unable to lookup fib for vrf {vrfid}"); - packet.done(DoneReason::InternalFailure); - return; - }; - /* Lookup the fib which needs to be consulted */ - let Some(fibr) = fibtr.get_fib(&fibkey) else { - warn!("{nfi}: Unable to find fib with id {fibkey} for vrf {vrfid}"); + /* access fib, by fetching FibReader from cache */ + let Ok(fibr) = &self.fibtr.get_fib_reader(fibkey) else { + warn!("{nfi}: Unable to read fib. Key={fibkey}"); packet.done(DoneReason::InternalFailure); return; }; - /* Read-only access to fib */ let Some(fib) = fibr.enter() else { - warn!("{nfi}: Unable to read from fib {fibkey}"); + warn!("{nfi}: Unable to read from fib. Key={fibkey}"); packet.done(DoneReason::InternalFailure); return; }; + /* Perform lookup in the fib */ let (prefix, fibentry) = fib.lpm_entry_prefix(packet); if let Some(fibentry) = &fibentry { @@ -110,7 +97,7 @@ impl IpForwarder { } } /* execute instructions according to FIB */ - self.packet_exec_instructions(&fibtr, packet, fibentry, fib.get_vtep()); + self.packet_exec_instructions(packet, fibentry, fib.get_vtep()); } else { debug!("Could not get fib group for {prefix}. Will drop packet..."); packet.done(DoneReason::InternalFailure); @@ -121,7 +108,6 @@ impl IpForwarder { fn packet_exec_instruction_local( &self, packet: &mut Packet, - fibtable: &FibTable, _ifindex: InterfaceIndex, /* we get it from metadata */ ) { let nfi = &self.name; @@ -134,14 +120,18 @@ impl IpForwarder { let vni = vxlan.vni(); debug!("{nfi}: DECAPSULATED vxlan packet:\n {packet}"); debug!("{nfi}: Packet comes with vni {vni}"); - let fibid = FibKey::from_vni(vni); - let Some(fib) = fibtable.get_fib(&fibid) else { - error!("{nfi}: Failed to find fib {fibid} associated to vni {vni}"); + + // access fib for Vni vni + let fibkey = FibKey::from_vni(vni); + let Ok(fibr) = self.fibtr.get_fib_reader(fibkey) else { + error!("{nfi}: Failed to find fib associated to vni {vni}. Fib key = {fibkey}"); packet.done(DoneReason::Unroutable); return; }; - let Some(next_vrf) = fib.get_id().map(|id| id.as_u32()) else { - debug!("{nfi}: Failed to access fib {fibid} to determine vrf"); + let Some(next_vrf) = fibr.get_id().map(|id| id.as_u32()) else { + debug!( + "{nfi}: Failed to access fib {fibkey} to determine vrf. Fib Key={fibkey}" + ); packet.done(DoneReason::InternalFailure); return; }; @@ -331,7 +321,6 @@ impl IpForwarder { /// Execute a [`PktInstruction`] on the packet fn packet_exec_instruction( &self, - fibtable: &FibTable, vtep: &Vtep, packet: &mut Packet, instruction: &PktInstruction, @@ -339,7 +328,7 @@ impl IpForwarder { match instruction { PktInstruction::Drop => self.packet_exec_instruction_drop(packet), PktInstruction::Local(ifindex) => { - self.packet_exec_instruction_local(packet, fibtable, *ifindex); + self.packet_exec_instruction_local(packet, *ifindex); } PktInstruction::Encap(encap) => self.packet_exec_instruction_encap(packet, encap, vtep), PktInstruction::Egress(egress) => self.packet_exec_instruction_egress(packet, egress), @@ -349,13 +338,12 @@ impl IpForwarder { /// Execute all of the [`PktInstruction`]s indicated by the given [`FibEntry`] on the packet fn packet_exec_instructions( &self, - fibtable: &FibTable, packet: &mut Packet, fibentry: &FibEntry, vtep: &Vtep, ) { for inst in fibentry.iter() { - self.packet_exec_instruction(fibtable, vtep, packet, inst); + self.packet_exec_instruction(vtep, packet, inst); if packet.is_done() { return; } From 70b7598a7dd4840a8bc3d998a156e5eb27172efb Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 17:54:39 +0200 Subject: [PATCH 23/35] feat(routing): enforce fibtable cache usage Signed-off-by: Fredi Raspall --- routing/src/fib/fibtable.rs | 10 ++++++-- routing/src/rib/vrftable.rs | 46 ++++++++++++------------------------- 2 files changed, 23 insertions(+), 33 deletions(-) diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 9aef74973..8635c1a3a 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -69,9 +69,11 @@ impl FibTable { fn get_entry(&self, key: &FibKey) -> Option<&Arc> { self.entries.get(key) } - /// Get a [`FibReader`] for the fib with the given [`FibKey`]. The call of this - /// method to handle packets is ===TEMPORARY=== as it creates a new `FibReader` every time. + + /// Get a [`FibReader`] for the fib with the given [`FibKey`]. This method should only + /// be called in the existing tests, as it creates a new `FibReader` on every call. #[must_use] + #[cfg(test)] pub fn get_fib(&self, key: &FibKey) -> Option { self.get_entry(key).map(|entry| entry.factory.handle()) } @@ -127,6 +129,10 @@ impl FibTableWriter { pub fn enter(&self) -> Option> { self.0.enter() } + #[must_use] + pub fn as_fibtable_reader(&self) -> FibTableReader { + FibTableReader(self.0.clone()) + } #[allow(clippy::arc_with_non_send_sync)] #[must_use] pub fn add_fib(&mut self, vrfid: VrfId, vni: Option) -> FibWriter { diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index 9c03c5678..7a7b5a95f 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -134,49 +134,33 @@ impl VrfTable { /// a [`Vni`] configured or it does but the internal state is not the expected. /////////////////////////////////////////////////////////////////////////////////// pub fn check_vni(&self, vrfid: VrfId) -> Result<(), RouterError> { - // lookup vrf + // lookup vrf: should be found let vrf = self.get_vrf(vrfid)?; - // Vrf must have a vni configured + // Vrf must have a vni configured: should succeed let Some(vni) = &vrf.vni else { return Err(RouterError::Internal("No vni found")); }; - // must be able to look it up by vni + // must be able to look up [`Vrf`] by vni and we must find a [`Vrf`] with same [`VrfId`] let found = self.get_vrfid_by_vni(*vni)?; if found != vrfid { error!("Vni {vni} refers to vrfid {found} and not {vrfid}"); return Err(RouterError::Internal("Inconsistent vni mapping")); } - // access fib fibtable - let fibtable = self - .fibtablew - .enter() - .ok_or(RouterError::Internal("Failed to access fib table"))?; - - // lookup fib from vni must succeed - let fibr = fibtable - .get_fib(&FibKey::Vni(*vni)) - .ok_or(RouterError::Internal("No fib for vni found"))?; - - // access the fib - let fib = fibr - .enter() - .ok_or(RouterError::Internal("Unable to read fib"))?; - - let found_fibid = fib.get_id(); - - if let Some(fibw) = &vrf.fibw { - let fib = fibw - .enter() - .ok_or(RouterError::Internal("Unable to access Fib for vrf"))?; - - let fibid = fib.get_id(); - if fibid != found_fibid { - error!("Expected: {found_fibid} found: {fibid}"); - return Err(RouterError::Internal("Inconsistent fib id found!")); - } + // access fib table: it should always be possible for us to enter the fib table since + // the vrf table owns the `FibTableWriter`. Also, as a reader, we should be able to see the latest + // changes since we published and would otherwise got blocked. To test for correctness, we check + // the two keys via which this vrf should be accessible and from our thread-local cache. + let fibtabler = self.fibtablew.as_fibtable_reader(); + fibtabler.get_fib_reader(FibKey::from_vrfid(vrfid))?; + + let fibid = FibKey::from_vrfid(vrfid); // The id it should have + if let Some(key) = fibtabler.get_fib_reader(FibKey::from_vni(*vni))?.get_id() + && key != fibid + { + return Err(RouterError::Internal("Inconsistent fib id found!")); } Ok(()) } From 7231d8f2e1fecc968d79c237d8d848f05a3e8b68 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 15 Oct 2025 22:49:17 +0200 Subject: [PATCH 24/35] feat(left-right-tlcache): Add refresh and iterator Extend the thread-local cache with full refresh logic and iterator. This requires adding a new method to trait ReadHandleProvider. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 131 ++++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index 2b1efcade..a6697648e 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -60,6 +60,16 @@ pub trait ReadHandleProvider: Sync { /// Get version. Provider should promise to provide a distinct value (e.g. monotonically increasing) /// anytime there's a change in the collection of read handles / factories it owns. fn get_version(&self) -> u64; + + /// Method for a provider to produce an iterator over the read handles (factories) + /// This returns (version, iterator) + #[allow(clippy::type_complexity)] + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, Self::Key)>, + ); } /// Trait to determine the real identity of a `T` wrapped in left-right. That is, @@ -183,6 +193,71 @@ where local.handles.borrow_mut().clear(); }); } + + // Do a full refresh of the cache + pub fn refresh( + thread_local: &'static LocalKey, + provider: &impl ReadHandleProvider, + ) { + // get all readers (factories) from the provider + let (version, iterator) = provider.get_iter(); + + // filter out all unusable readers + let iterator = iterator.filter(|(_key, factory, _id)| { + let rhandle = factory.handle(); + !rhandle.was_dropped() + }); + + // update local cache, consuming the iterator + thread_local.with(|local| { + let mut handles = local.handles.borrow_mut(); + + // purge all unusable readers + handles.retain(|_key, entry| !entry.rhandle.was_dropped()); + + // update cache from iterator if: 1) cache does not have them or 2) version changed + // FIXME: here aliases store an extra handle instead of reusing the one for key == identity + // Will fix that later. + for (key, factory, id) in iterator { + handles + .entry(key.clone()) + .and_modify(|e| { + if e.version != version { + *e = + ReadHandleEntry::new(id.clone(), Rc::new(factory.handle()), version) + } + }) + .or_insert_with(|| { + ReadHandleEntry::new(id.clone(), Rc::new(factory.handle()), version) + }); + } + }); + } + + /// Get an iterator of read handles from the cache. If refresh is true, the cache will be refreshed first. + /// This function is mostly useful if we want to iterate over the objects that the cache represents, optionally + /// refreshing it first. This is useful when caller does not know _what_ objects are there. + /// Since `ReadHandleProvider`s must be Sync, threads could simply call `ReadHandleProvider::get_iter()` + /// directly. However, that would not refresh the cache and would create a new reader for every item returned + /// by the provider, on each call. + pub fn iter( + thread_local: &'static LocalKey, + provider: &impl ReadHandleProvider, + refresh: bool, + ) -> impl Iterator>)> { + if refresh { + ReadHandleCache::refresh(thread_local, provider); + } + thread_local.with(|local| { + let vector: Vec<(K, Rc>)> = local + .handles + .borrow() + .iter() + .map(|(key, e)| (key.clone(), Rc::clone(&e.rhandle))) + .collect(); + vector.into_iter() + }) + } } /// Create a thread-local `ReadHandleCache` with a given name, to access @@ -346,6 +421,19 @@ mod tests { fn get_identity(&self, key: &Self::Key) -> Option { self.data.get(key).map(|entry| entry.id) } + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, Self::Key)>, + ) { + let iterator = self + .data + .iter() + .map(|(key, entry)| (*key, &entry.factory, entry.id)); + + (self.version, iterator) + } } #[serial] @@ -573,4 +661,47 @@ mod tests { // all handles from cache should have been removed as we looked up them all TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); } + + #[serial] + #[test] + fn test_readhandle_cache_iter() { + // start fresh + ReadHandleCache::purge(&TEST_CACHE); + + // build provider and populate it + const NUM_HANDLES: u64 = 20; + let mut provider = TestProvider::new(); + for id in 1..=NUM_HANDLES { + let alias = NUM_HANDLES + id; + provider.add_object(id, id); + provider.mod_object(id, format!("object-{id}").as_ref()); + provider.add_object(alias, id); + } + + // should be empty + TEST_CACHE.with(|cache| assert!(cache.handles.borrow().is_empty())); + + // request iterator without refresh. Nothing should be returned since cache is empty. + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, false); + assert_eq!(iterator.count(), 0); + + // request iterator with refresh + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + + // consume it + let mut count = 0; + for (_key, rhandle) in iterator { + count += 1; + let _guard = rhandle.enter().unwrap(); + } + + // we got all + assert_eq!(count, NUM_HANDLES * 2); + + // test that refresh/iter filters out invalid handles (need refresh) 2 should have been invalidated + provider.drop_writer(1); + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + let vec: Vec<(u64, Rc>)> = iterator.collect(); + assert_eq!(vec.len() as u64, (NUM_HANDLES - 1) * 2); + } } From 7b24fd07df0f9e90f0e15bba79c139ce8427526c Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Thu, 16 Oct 2025 21:49:20 +0200 Subject: [PATCH 25/35] feat(routing): add provider method for fibtable impl ReadHandleProvider get_iter() for FibTable. This is needed to allow the thread handling the cli to safely iterate over the fibs. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtable.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 8635c1a3a..05359aed7 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -216,6 +216,18 @@ impl ReadHandleProvider for FibTable { fn get_identity(&self, key: &Self::Key) -> Option { self.get_entry(key).map(|entry| entry.id) } + fn get_iter( + &self, + ) -> ( + u64, + impl Iterator, FibKey)>, + ) { + let iter = self + .entries + .iter() + .map(|(key, entry)| (*key, &*entry.factory.as_ref(), entry.as_ref().id)); + (self.version, iter) + } } impl FibTableReader { From 5065d66fcf78ac3c7286abe0c2584f5a4481a2b1 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Thu, 16 Oct 2025 22:31:02 +0200 Subject: [PATCH 26/35] feat(left-right-tlcache): optimization for iter The existing iterator implementation is subobtimal in that each call to get_iter() needs to request the provider a full iterator for the whole set of read handle factories, unless refresh arg is set to false, and then refresh the local cache with read handles built from each factory. This is inefficient in case the set of objects in the provider rarely changes (which may be the case in most setups). Avoid pulling an iterator from the provider if the version did not change, as this means that no objects have been added or removed. If anything, we remove unusable readers. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 52 ++++++++++++++++++++++++++++++++++- 1 file changed, 51 insertions(+), 1 deletion(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index a6697648e..a88817dda 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -128,6 +128,7 @@ impl, K: PartialEq> ReadHandleEntry { pub struct ReadHandleCache { handles: RefCell, RandomState>>, + refresh_version: RefCell, // version when last refresh mas made } impl ReadHandleCache where @@ -138,6 +139,7 @@ where pub fn new() -> Self { Self { handles: RefCell::new(HashMap::with_hasher(RandomState::with_seed(0))), + refresh_version: RefCell::new(0), } } pub fn get_reader( @@ -191,6 +193,15 @@ where pub fn purge(thread_local: &'static LocalKey) { thread_local.with(|local| { local.handles.borrow_mut().clear(); + *local.refresh_version.borrow_mut() = 0; + }); + } + + #[allow(unused)] + fn purge_unreadable(thread_local: &'static LocalKey) { + thread_local.with(|local| { + let mut handles = local.handles.borrow_mut(); + handles.retain(|_, e| !e.rhandle.was_dropped()); }); } @@ -199,9 +210,25 @@ where thread_local: &'static LocalKey, provider: &impl ReadHandleProvider, ) { + // skip refresh if the version has not changed + let cache_refresh_version = thread_local.with(|local| *local.refresh_version.borrow()); + let provider_version = provider.get_version(); + if cache_refresh_version == provider_version { + // this should not be needed + Self::purge_unreadable(thread_local); + return; + } + // get all readers (factories) from the provider let (version, iterator) = provider.get_iter(); + // theoretically, it could happen that while we call get_version() and get_iter(), the underlying collection + // has changed and both differ + if version != provider_version { + Self::refresh(thread_local, provider); + return; + } + // filter out all unusable readers let iterator = iterator.filter(|(_key, factory, _id)| { let rhandle = factory.handle(); @@ -231,12 +258,13 @@ where ReadHandleEntry::new(id.clone(), Rc::new(factory.handle()), version) }); } + *local.refresh_version.borrow_mut() = version; }); } /// Get an iterator of read handles from the cache. If refresh is true, the cache will be refreshed first. /// This function is mostly useful if we want to iterate over the objects that the cache represents, optionally - /// refreshing it first. This is useful when caller does not know _what_ objects are there. + /// refreshing it first. This is useful when the caller does not know _what_ objects are there. /// Since `ReadHandleProvider`s must be Sync, threads could simply call `ReadHandleProvider::get_iter()` /// directly. However, that would not refresh the cache and would create a new reader for every item returned /// by the provider, on each call. @@ -698,6 +726,28 @@ mod tests { // we got all assert_eq!(count, NUM_HANDLES * 2); + // test that if the version of the provider does not change, then the cache is not unnecessarily refreshed. + // Our test provider does not change version unless we add/drop elements and we don't now. + { + // empty cache for testing purposes. Since this will reset iter_version, we save it first + let saved_iter_version = TEST_CACHE.with(|local| *local.refresh_version.borrow()); + ReadHandleCache::purge(&TEST_CACHE); + TEST_CACHE.with(|local| *local.refresh_version.borrow_mut() = saved_iter_version); + } + // since version did not change, we should not get anything after iterating. + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + assert_eq!(iterator.count(), 0); + assert_eq!(TEST_CACHE.with(|local| local.handles.borrow().len()), 0); + + // if we reset the version, then the iterator should refresh the cache. + TEST_CACHE.with(|local| *local.refresh_version.borrow_mut() = 0); + let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); + assert_eq!(iterator.count() as u64, 2 * NUM_HANDLES); + assert_eq!( + TEST_CACHE.with(|local| local.handles.borrow().len() as u64), + 2 * NUM_HANDLES + ); + // test that refresh/iter filters out invalid handles (need refresh) 2 should have been invalidated provider.drop_writer(1); let iterator = ReadHandleCache::iter(&TEST_CACHE, &provider, true); From 6897da7e375e88510c3a06aa35db2c956e726078 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 17 Oct 2025 09:47:02 +0200 Subject: [PATCH 27/35] feat(left-right-tlcache): minimize num of rhandles Fix the code so that a cache keeps the minimal number of rhandles after a refresh. The previous code would keep an independent rhandle for aliases, meaning that if there are N objects each with one allias, 2 x N rhandles would exist after a refresh. Solve this by letting aliases use the same rhandle as their primary. Signed-off-by: Fredi Raspall --- left-right-tlcache/src/lib.rs | 38 +++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/left-right-tlcache/src/lib.rs b/left-right-tlcache/src/lib.rs index a88817dda..6476c3217 100644 --- a/left-right-tlcache/src/lib.rs +++ b/left-right-tlcache/src/lib.rs @@ -229,12 +229,15 @@ where return; } - // filter out all unusable readers + // filter out all unusable readers from iterator let iterator = iterator.filter(|(_key, factory, _id)| { let rhandle = factory.handle(); !rhandle.was_dropped() }); + // split the iterator in two: primaries and aliases + let (primaries, aliases): (Vec<_>, Vec<_>) = iterator.partition(|(key, _, id)| key == id); + // update local cache, consuming the iterator thread_local.with(|local| { let mut handles = local.handles.borrow_mut(); @@ -242,22 +245,41 @@ where // purge all unusable readers handles.retain(|_key, entry| !entry.rhandle.was_dropped()); - // update cache from iterator if: 1) cache does not have them or 2) version changed - // FIXME: here aliases store an extra handle instead of reusing the one for key == identity - // Will fix that later. - for (key, factory, id) in iterator { + // update primaries first and store an Rc of the latest rhandles in a temporary map + let mut temporary = HashMap::new(); + for (key, factory, id) in primaries { handles .entry(key.clone()) .and_modify(|e| { if e.version != version { - *e = - ReadHandleEntry::new(id.clone(), Rc::new(factory.handle()), version) + *e = ReadHandleEntry::new( + id.clone(), + Rc::new(factory.handle()), + version, + ); } + temporary.insert(id.clone(), Rc::clone(&e.rhandle)); }) .or_insert_with(|| { - ReadHandleEntry::new(id.clone(), Rc::new(factory.handle()), version) + let rhandle = Rc::new(factory.handle()); + temporary.insert(key, Rc::clone(&rhandle)); + ReadHandleEntry::new(id, rhandle, version) }); } + // update entries for aliases to reuse primaries' handles, using the temporary map + for (key, _factory, id) in aliases { + if let Some(rhandle) = temporary.get(&id) { + handles.insert( + key.clone(), + ReadHandleEntry::new(id, Rc::clone(rhandle), version), + ); + } else { + // we should only get here if we got a key (alias) and could not find + // the primary object. This would be a provider bug. + // TODO: determine what to do here + } + } + *local.refresh_version.borrow_mut() = version; }); } From 7f32131e0886c7bc464d022ceee78a9e9ac5feb9 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 17 Oct 2025 10:53:06 +0200 Subject: [PATCH 28/35] feat(routing): removed unused code Signed-off-by: Fredi Raspall --- routing/src/display.rs | 28 ++++++++++------------------ routing/src/fib/fibtable.rs | 16 ++-------------- 2 files changed, 12 insertions(+), 32 deletions(-) diff --git a/routing/src/display.rs b/routing/src/display.rs index aac612f76..4d4d7e5be 100644 --- a/routing/src/display.rs +++ b/routing/src/display.rs @@ -2,12 +2,20 @@ // Copyright Open Network Fabric Authors //! Module that implements Display for routing objects - +//! +//! Note: most of the objects for which Display is implemented here belong to +//! the routing database which is fully and only owned by the routing thread. +//! This includes the Fib contents since fibs belong to vrfs. +//! So: +/// - it is Okay to call any of this from the routing thread (cli) +/// - other threads may not be able to call Display's for routing objects. +/// - Display for Fib objects visible from dataplane workers can be safely called. +/// - Cli thread does not need a read handle cache to inspect Fib contents +/// - Still, FIXME(fredi): make that distinction clearer use crate::atable::adjacency::{Adjacency, AdjacencyTable}; use crate::cpi::{CpiStats, CpiStatus, StatsRow}; use crate::fib::fibgroupstore::FibRoute; use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; -use crate::fib::fibtable::FibTable; use crate::fib::fibtype::{Fib, FibKey}; use crate::frr::frrmi::{FrrAppliedConfig, Frrmi, FrrmiStats}; @@ -723,22 +731,6 @@ impl Display for Fib { Ok(()) } } -impl Display for FibTable { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { - Heading(format!( - " Fib Table ({} fibs, version: {})", - self.len(), - self.version() - )) - .fmt(f)?; - for fibr in self.values().map(|f| f.handle()) { - if let Some(fib) = fibr.enter() { - write!(f, "{}", *fib)?; - } - } - Ok(()) - } -} pub struct FibViewV4<'a, F> where diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index 05359aed7..bf41ce9b1 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -78,24 +78,12 @@ impl FibTable { self.get_entry(key).map(|entry| entry.factory.handle()) } + /// Number of entries in this table #[must_use] + #[cfg(test)] pub(crate) fn len(&self) -> usize { self.entries.len() } - /// Tell if fib table is empty - #[must_use] - #[allow(unused)] - pub(crate) fn is_empty(&self) -> bool { - self.entries.is_empty() - } - /// Provide an iterator of [`FibReaderFactory`]s - pub(crate) fn values(&self) -> impl Iterator { - self.entries.values().map(|e| &e.factory) - } - /// Tell version of this [`FibTable`] - pub(crate) fn version(&self) -> u64 { - self.version - } } enum FibTableChange { From 90499f77f77aeb26367497f303b3e3c3c83c85d4 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 17 Oct 2025 11:32:32 +0200 Subject: [PATCH 29/35] feat(routing): cleanup, improve logs & tests Signed-off-by: Fredi Raspall --- routing/src/fib/fibtable.rs | 6 +- routing/src/interfaces/iftablerw.rs | 3 + routing/src/rib/vrf.rs | 2 +- routing/src/rib/vrftable.rs | 109 ++++++++++++++++++++-------- 4 files changed, 86 insertions(+), 34 deletions(-) diff --git a/routing/src/fib/fibtable.rs b/routing/src/fib/fibtable.rs index bf41ce9b1..7913fd646 100644 --- a/routing/src/fib/fibtable.rs +++ b/routing/src/fib/fibtable.rs @@ -43,7 +43,7 @@ impl FibTable { } /// Delete a `Fib`, by unregistering a `FibReaderFactory` for it fn del_fib(&mut self, id: &FibKey) { - info!("Unregistering Fib id {id} from the FibTable"); + info!("Unregistering Fib with id {id} from the FibTable"); self.entries.remove(id); } /// Register an existing `Fib` with a given [`Vni`]. @@ -52,7 +52,7 @@ impl FibTable { if let Some(entry) = self.get_entry(id) { self.entries .insert(FibKey::from_vni(vni), Arc::clone(entry)); - info!("Registering Fib with id {id} with new vni {vni} in FibTable"); + info!("Registering Fib with id {id} in the FibTable with vni {vni}"); } else { error!("Failed to register Fib {id} with vni {vni}: no fib with id {id} found"); } @@ -60,7 +60,7 @@ impl FibTable { /// Remove any entry keyed by a [`Vni`] fn unregister_vni(&mut self, vni: Vni) { let key = FibKey::from_vni(vni); - info!("Unregistered Fib with vni {vni} from the FibTable"); + info!("Unregistered key = {key} from the FibTable"); self.entries.remove(&key); } diff --git a/routing/src/interfaces/iftablerw.rs b/routing/src/interfaces/iftablerw.rs index 77bb8fb53..417b24e66 100644 --- a/routing/src/interfaces/iftablerw.rs +++ b/routing/src/interfaces/iftablerw.rs @@ -14,6 +14,8 @@ use left_right::ReadHandleFactory; use left_right::{Absorb, ReadGuard, ReadHandle, WriteHandle}; use net::interface::InterfaceIndex; +use tracing::debug; + enum IfTableChange { Add(RouterInterfaceConfig), Mod(RouterInterfaceConfig), @@ -169,6 +171,7 @@ impl IfTableWriter { self.0.publish(); } pub fn detach_interfaces_from_vrf(&mut self, vrfid: VrfId) { + debug!("Scheduling detach of interfaces from vrf {vrfid}"); self.0 .append(IfTableChange::DetachFromVrf(FibKey::Id(vrfid))); self.0.publish(); diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 936270ec1..0db143d87 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -267,7 +267,7 @@ impl Vrf { ///////////////////////////////////////////////////////////////////////// pub fn set_vni(&mut self, vni: Vni) { self.vni = Some(vni); - debug!("Associated vni {vni} to Vrf '{}'", self.name); + debug!("Set vni {vni} to Vrf {} ({})", self.vrfid, self.name); } ///////////////////////////////////////////////////////////////////////// diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index 7a7b5a95f..b99627ed2 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -95,19 +95,20 @@ impl VrfTable { return Ok(()); /* vrf already has that vni */ } // No vrf has the requested vni, including the vrf with id vrfId. - // However the vrf with id VrfId may have another vni associated, - - /* remove vni from vrf if it has one */ + // However the vrf with id VrfId may have another vni associated. + // So, unset any vni that the vrf may have associated first. self.unset_vni(vrfid)?; - /* set the vni to the vrf */ + /* lookup vrf */ let vrf = self.get_vrf_mut(vrfid)?; + + /* set the vni to the vrf */ vrf.set_vni(vni); - /* register vni */ + /* register vni as key for vrf vrfid in the vrf table */ self.by_vni.insert(vni, vrfid); - /* make fib accessible from vni */ + /* make fib accessible from vni in the fib table */ self.fibtablew.register_fib_by_vni(vrfid, vni); Ok(()) } @@ -117,13 +118,18 @@ impl VrfTable { /// removes it from the `by_vni` map. It also unindexes the vrf's FIB by the vni. /////////////////////////////////////////////////////////////////////////////////// pub fn unset_vni(&mut self, vrfid: VrfId) -> Result<(), RouterError> { + debug!("Unsetting any vni configuration for vrf {vrfid}.."); let vrf = self.get_vrf_mut(vrfid)?; + + // check if vrf has vni configured if let Some(vni) = vrf.vni { - debug!("Removing vni {vni} from vrf {vrfid}..."); + debug!("Vrf {vrfid} has vni {vni} associated. Removing..."); vrf.vni.take(); self.by_vni.remove(&vni); self.fibtablew.unregister_vni(vni); - debug!("Vrf with Id {vrfid} no longer has a VNI associated"); + debug!("Vrf with Id {vrfid} no longer has a vni {vni} associated"); + } else { + debug!("Vrf {vrfid} has no vni configured"); } Ok(()) } @@ -173,14 +179,15 @@ impl VrfTable { vrfid: VrfId, iftablew: &mut IfTableWriter, ) -> Result<(), RouterError> { - debug!("Removing VRF with vrfid {vrfid}..."); + // remove the vrf from the vrf table + debug!("Removing VRF {vrfid}..."); let Some(vrf) = self.by_id.remove(&vrfid) else { error!("No vrf with id {vrfid} exists"); return Err(RouterError::NoSuchVrf); }; // delete the corresponding fib if vrf.fibw.is_some() { - debug!("Requesting deletion of vrf {vrfid} FIB"); + debug!("Deleting Fib for vrf {vrfid} from the FibTable"); self.fibtablew.del_fib(vrfid, vrf.vni); iftablew.detach_interfaces_from_vrf(vrfid); } @@ -190,6 +197,7 @@ impl VrfTable { debug!("Unregistering vni {vni}"); self.by_vni.remove(&vni); } + debug!("Vrf {vrfid} has been removed"); Ok(()) } @@ -216,6 +224,7 @@ impl VrfTable { /// Remove all of the VRFs with status `Deleted` ////////////////////////////////////////////////////////////////// pub fn remove_deleted_vrfs(&mut self, iftablew: &mut IfTableWriter) { + debug!("Removing deletable vrfs..."); self.remove_vrfs(Vrf::can_be_deleted, iftablew); } @@ -223,6 +232,7 @@ impl VrfTable { /// Remove all of the VRFs with status `Deleting` ////////////////////////////////////////////////////////////////// pub fn remove_deleting_vrfs(&mut self, iftablew: &mut IfTableWriter) { + debug!("Removing vrfs with deleting status..."); self.remove_vrfs(Vrf::is_deleting, iftablew); } @@ -657,11 +667,10 @@ mod tests { .expect("Should find vrfid by vni"); assert_eq!(id, vrfid); debug!("\n{vrftable}"); + if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); - assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibKey::from_vni(vni)); - assert!(fib.is_some()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_some()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_some()); } debug!("━━━━Test: Unset vni {vni} from the vrf"); @@ -674,36 +683,72 @@ mod tests { assert!((id.is_err_and(|e| e == RouterError::NoSuchVrf))); debug!("\n{vrftable}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); - assert!(fib.is_some()); - let fib = fibtable.get_fib(&FibKey::from_vni(vni)); - assert!(fib.is_none()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_some()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_none()); } } #[traced_test] #[test] fn vrf_table_deletions() { + debug!("━━━━Test: Create testing interface table"); + let (mut iftw, iftr) = build_test_iftable_left_right(); + debug!("━━━━Test: Create vrf table"); let (fibtw, fibtr) = FibTableWriter::new(); - let (mut iftw, iftr) = build_test_iftable_left_right(); let mut vrftable = VrfTable::new(fibtw); + debug!("━━━━Test: Check fib access for vrf default"); + if let Some(fibtable) = fibtr.enter() { + let fibkey = FibKey::from_vrfid(0); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + } + let vrfid = 999; let vni = mk_vni(3000); - debug!("━━━━Test: Add a VRF without Vni"); + debug!("━━━━Test: Add a VRF with id {vrfid} but no Vni"); let vrf_cfg = RouterVrfConfig::new(vrfid, "VPC-1"); vrftable.add_vrf(&vrf_cfg).expect("Should be created"); + assert_eq!(vrftable.len(), 2); // default is always there + debug!("\n{vrftable}"); + + debug!("━━━━Test: Check fib access for vrf {vrfid}"); + if let Some(fibtable) = fibtr.enter() { + // check that fib is accessible from vrfid + let fibkey = FibKey::from_vrfid(vrfid); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + assert_eq!(fibtable.as_ref().len(), 2); + } - debug!("━━━━Test: Associate VNI {vni}"); + debug!("━━━━Test: Associate vni {vni} to vrf {vrfid}"); vrftable.set_vni(vrfid, vni).expect("Should succeed"); - assert_eq!(vrftable.len(), 2); // default is always there debug!("\n{vrftable}"); + debug!("━━━━Test: Check fib access for vrf {vrfid} and vni {vni}"); + if let Some(fibtable) = fibtr.enter() { + // check that fib continues to be accessible via vrfid + let fibkey = FibKey::from_vrfid(vrfid); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), fibkey); + + // check that fib is accessible from vni + let fibkey = FibKey::from_vni(vni); + let fibr = fibtable.get_fib(&fibkey).unwrap(); + let fib = fibr.enter().unwrap(); + assert_eq!(fib.as_ref().get_id(), FibKey::from_vrfid(vrfid)); + } + debug!("━━━━Test: deleting removed VRFs: nothing should be removed"); vrftable.remove_deleted_vrfs(&mut iftw); assert_eq!(vrftable.len(), 2); // default is always there + assert_eq!(fibtr.enter().unwrap().len(), 3); + debug!("\n{vrftable}"); debug!("━━━━Test: Get interface from iftable"); let idx = InterfaceIndex::try_new(2).unwrap(); @@ -713,7 +758,7 @@ mod tests { debug!("\n{}", *iftable); } - debug!("━━━━Test: Attach interface to vrf"); + debug!("━━━━Test: Attach interface to vrf {vrfid}"); iftw.attach_interface_to_vrf(idx, vrfid, &vrftable) .expect("Should succeed"); if let Some(iftable) = iftr.enter() { @@ -727,18 +772,22 @@ mod tests { vrf.set_status(VrfStatus::Deleted); debug!("\n{vrftable}"); - debug!("━━━━Test: remove vrfs marked as deleted again - VPC-1 vrf should be gone"); + debug!("━━━━Test: remove vrfs marked as deleted: VPC-1 vrf should be gone"); vrftable.remove_deleted_vrfs(&mut iftw); assert_eq!(vrftable.len(), 1, "should be gone"); + assert_eq!( + fibtr.enter().unwrap().len(), + 1, + "only default fib should remain" + ); - // check fib table + debug!("━━━━Test: Check that no fib is accessible vrfid:{vrfid} nor vni:{vni}"); if let Some(fibtable) = fibtr.enter() { - let fib = fibtable.get_fib(&FibKey::from_vrfid(vrfid)); - assert!(fib.is_none()); - let fib = fibtable.get_fib(&FibKey::from_vni(vni)); - assert!(fib.is_none()); - assert_eq!(fibtable.len(), 1); + assert!(fibtable.get_fib(&FibKey::from_vrfid(vrfid)).is_none()); + assert!(fibtable.get_fib(&FibKey::from_vni(vni)).is_none()); + assert!(fibtable.get_fib(&FibKey::from_vrfid(0)).is_some()); } + debug!("━━━━Test: Interface {idx} should no longer be attached"); if let Some(iftable) = iftr.enter() { let iface = iftable.get_interface(idx).expect("Should be there"); assert!(iface.attachment.is_none(), "Should have been detached"); From 132338161f97fa0a3039ce5b40653d795a1a6511 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 3 Oct 2025 11:56:03 +0200 Subject: [PATCH 30/35] feat(routing): misc cleanups Misc small cleanups: - adjust method visibility - use more reasonable tracing levels - remove unused methods - clean-up tests - move fibgroup drop builder Signed-off-by: Fredi Raspall --- routing/src/fib/fibgroupstore.rs | 61 +++++++++++++++----------------- routing/src/fib/fibobjects.rs | 5 +++ routing/src/fib/fibtype.rs | 4 +-- routing/src/rib/rib2fib.rs | 14 ++++---- routing/src/rib/vrf.rs | 4 +-- 5 files changed, 44 insertions(+), 44 deletions(-) diff --git a/routing/src/fib/fibgroupstore.rs b/routing/src/fib/fibgroupstore.rs index fbf742e68..906117ed7 100644 --- a/routing/src/fib/fibgroupstore.rs +++ b/routing/src/fib/fibgroupstore.rs @@ -20,8 +20,6 @@ //! must be done while holding a `left_right::ReadGuard` or `left_right::WriteGuard` to //! the `Fib` that owns it. -#![allow(unused)] - use crate::fib::fibobjects::{FibEntry, FibGroup}; use crate::rib::nexthop::NhopKey; use ahash::RandomState; @@ -39,7 +37,7 @@ pub enum FibError { NoFibGroup(NhopKey), } -#[derive(Default, Debug)] +#[derive(Debug, Default)] pub(crate) struct FibGroupStore(HashMap>, RandomState>); impl FibGroupStore { @@ -49,7 +47,7 @@ impl FibGroupStore { unsafe { // This is safe because we have exclusive access to the store we just created // so no one else can have a reference to any Rc's it contains - store.add_mod_group(&NhopKey::with_drop(), Self::drop_fibgroup()); + store.add_mod_group(&NhopKey::with_drop(), FibGroup::drop_fibgroup()); } store } @@ -59,10 +57,6 @@ impl FibGroupStore { self.0.len() } #[must_use] - fn drop_fibgroup() -> FibGroup { - FibGroup::with_entry(FibEntry::drop_fibentry()) - } - #[must_use] /// get an Rc for the drop `Fibgroup`. The drop fibgroup is unique. pub fn get_drop_fibgroup_ref(&self) -> Rc> { self.get_ref(&NhopKey::with_drop()) @@ -99,6 +93,7 @@ impl FibGroupStore { fn get_ref(&self, key: &NhopKey) -> Option>> { self.0.get(key).map(|group| Rc::clone(group)) } + #[cfg(test)] #[must_use] pub(crate) fn get(&self, key: &NhopKey) -> Option> { self.0.get(key).map(|group| group.borrow()) @@ -141,9 +136,11 @@ impl FibGroupStore { pub(crate) fn values(&self) -> impl Iterator> { self.0.values().map(|group| group.borrow()) } + //////////////////////////////////////////////////////////////////////////////////////////////////// /// Iterate over the `FibGroups` in the store //////////////////////////////////////////////////////////////////////////////////////////////////// + #[cfg(test)] pub(crate) fn iter(&self) -> impl Iterator)> { self.0.iter().map(|(key, group)| (key, group.borrow())) } @@ -160,20 +157,12 @@ impl FibRoute { pub fn with_fibgroup(fg_ref: Rc>) -> Self { Self(vec![fg_ref]) } + + #[cfg(test)] /// Add a reference to a `FibGroup` to a `FibRoute` pub(crate) fn add_fibgroup_ref(&mut self, fg_ref: Rc>) { self.0.push(fg_ref); } - #[must_use] - #[cfg(test)] - /// Get a reference to the `FibGroup` at the group-level index. This is only for testing. - pub(crate) fn get_fibgroup(&self, index: usize) -> Option> { - if index < self.len() { - unsafe { Some(self.0[index].borrow()) } - } else { - None - } - } ///////////////////////////////////////////////////////////////////////////////////////////////// /// Tells the total number of `FibEntry`s in a `FibRoute`, as the sum of the lengths of its `FibGroup`s @@ -272,7 +261,7 @@ pub mod tests { )); FibEntry::with_inst(inst) } - // builds fibgroup with a single entry + // builds a fibgroup with several entries fn build_fibgroup(entries: &[FibEntry]) -> FibGroup { let mut fibgroup = FibGroup::new(); fibgroup.entries.extend_from_slice(entries); @@ -298,11 +287,12 @@ pub mod tests { { // retrieve the fibgrop from the store from its key + // (scoped since we mutate the store later on) let stored = store.get(&nhkey).unwrap(); assert_eq!(stored.len(), 1); assert_eq!(stored.entries[0].len(), 1); assert_eq!(stored.entries[0], entry); - println!("{stored:#?}"); + println!("{stored}"); } // create fibroute to have a shared ref to that fibgroup @@ -311,6 +301,7 @@ pub mod tests { assert_eq!(fibroute.0.len(), 1); assert_eq!(fibroute.len(), 1); + // create a second fibroute object sharing the same fibgroup let mut fibroute2 = FibRoute::new(); fibroute2.add_fibgroup_ref(store.get_ref(&nhkey).unwrap()); @@ -326,11 +317,12 @@ pub mod tests { let found2 = fibroute2.get_fibentry(0).unwrap(); assert_eq!(*found1, *found2); assert_eq!(*found1, entry2); - println!("hit:\n{found1}"); + + println!("updated fibgroup entry:\n{found1}"); } #[test] - fn test_multi_fibgroup_route_entry_selection() { + fn test_fibgroup_multigroup_route_entry_selection() { // create multiple fib entries let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); @@ -347,7 +339,7 @@ pub mod tests { let g3 = build_fibgroup(&[e7.clone()]); let g4 = build_fibgroup(&[e8.clone()]); - // create a dummy nhop key whose contents do not matter other than for storing each fibgroup + // create dummy nhop keys whose contents do not matter other than for storing each fibgroup let key1 = NhopKey::with_address(&IpAddr::from_str("8.0.0.1").unwrap()); let key2 = NhopKey::with_address(&IpAddr::from_str("8.0.0.2").unwrap()); let key3 = NhopKey::with_address(&IpAddr::from_str("8.0.0.3").unwrap()); @@ -379,14 +371,16 @@ pub mod tests { assert_eq!(fibroute.len(), g1.len() + g2.len() + g3.len() + g4.len()); // select one entry in the fibroute by index and check that it is correct - assert_eq!(fibroute.get_fibentry(0).map(|e| e.clone()), Some(e1)); - assert_eq!(fibroute.get_fibentry(1).map(|e| e.clone()), Some(e2)); - assert_eq!(fibroute.get_fibentry(2).map(|e| e.clone()), Some(e3)); - assert_eq!(fibroute.get_fibentry(3).map(|e| e.clone()), Some(e4)); - assert_eq!(fibroute.get_fibentry(4).map(|e| e.clone()), Some(e5)); - assert_eq!(fibroute.get_fibentry(5).map(|e| e.clone()), Some(e6)); - assert_eq!(fibroute.get_fibentry(6).map(|e| e.clone()), Some(e7)); - assert_eq!(fibroute.get_fibentry(7).map(|e| e.clone()), Some(e8)); + // this route has all of the fibgroups + assert_eq!(&*fibroute.get_fibentry(0).unwrap(), &e1); + assert_eq!(&*fibroute.get_fibentry(0).unwrap(), &e1); + assert_eq!(&*fibroute.get_fibentry(1).unwrap(), &e2); + assert_eq!(&*fibroute.get_fibentry(2).unwrap(), &e3); + assert_eq!(&*fibroute.get_fibentry(3).unwrap(), &e4); + assert_eq!(&*fibroute.get_fibentry(4).unwrap(), &e5); + assert_eq!(&*fibroute.get_fibentry(5).unwrap(), &e6); + assert_eq!(&*fibroute.get_fibentry(6).unwrap(), &e7); + assert_eq!(&*fibroute.get_fibentry(7).unwrap(), &e8); assert!(fibroute.get_fibentry(8).is_none()); // attempt to remove fibgroups: none should be removed from the store @@ -466,8 +460,9 @@ pub mod tests { // Attempt to create a Fibroute with a key for which no fibgroup exists: should fail let key = NhopKey::with_address(&IpAddr::from_str("9.0.0.1").unwrap()); - let fibroute = FibRoute::from_nhopkeys(&store, &[key1, key2, key]); - assert!(matches!(fibroute, Err(FibError::NoFibGroup(ref key)))); + println!("Creating fibroute from non-registered nhop key {key}..."); + let fibroute = FibRoute::from_nhopkeys(&store, &[key1, key2, key.clone()]); println!("{fibroute:#?}"); + assert!(fibroute.is_err_and(|e| e == FibError::NoFibGroup(key))); } } diff --git a/routing/src/fib/fibobjects.rs b/routing/src/fib/fibobjects.rs index 8234836d0..788025b07 100644 --- a/routing/src/fib/fibobjects.rs +++ b/routing/src/fib/fibobjects.rs @@ -84,6 +84,11 @@ impl FibGroup { entries: vec![entry], } } + #[must_use] + pub(crate) fn drop_fibgroup() -> FibGroup { + FibGroup::with_entry(FibEntry::drop_fibentry()) + } + /// Add a [`FibEntry`] to a [`FibGroup`] pub fn add(&mut self, entry: FibEntry) { self.entries.push(entry); diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 4ebe46e14..a4d5d6f25 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -26,7 +26,7 @@ use crate::rib::nexthop::NhopKey; use crate::rib::vrf::VrfId; #[allow(unused)] -use tracing::{debug, error, info, warn}; +use tracing::{debug, error, info, trace, warn}; #[derive(Copy, Clone, Debug, Hash, Ord, PartialOrd, Eq, PartialEq)] /// A type used to access a [`Fib`] or to identify it. @@ -282,7 +282,7 @@ impl Fib { 1 => (prefix, route.get_fibentry(0)), k => { let entry_index = packet.packet_hash_ecmp(0, (k - 1) as u8); - debug!("Selected FibEntry {entry_index}/{k} to forward packet"); + trace!("Selected FibEntry {entry_index}/{k} to forward packet"); (prefix, route.get_fibentry(entry_index as usize)) } } diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index e04982932..3cca2ab7e 100644 --- a/routing/src/rib/rib2fib.rs +++ b/routing/src/rib/rib2fib.rs @@ -4,7 +4,7 @@ //! Rib to fib route processor #[allow(unused)] -use tracing::{debug, warn}; +use tracing::{debug, trace, warn}; use crate::evpn::RmacStore; use crate::rib::encapsulation::{Encapsulation, VxlanEncapsulation}; @@ -108,18 +108,18 @@ impl Nhop { pub(crate) fn set_fibgroup(&self, rstore: &RmacStore) -> bool { // determine nhop pkt instructions. This is independent of the routing table self.build_nhop_instructions(rstore); + // build the fibgroup for a next-hop. This requires the nhop to be resolved // and its resolvers too, and that these have packet instructions up to date let fibgroup = self.build_nhop_fibgroup(); - let changed = fibgroup != *self.fibgroup.borrow(); + let changed = fibgroup != *(self.fibgroup.borrow()); if changed { - // FIXME(fredi): we need a way of enabling these logs at runtime - //debug!("Fibgroup for nhop {self}\nchanged!"); - //debug!("\nold:\n{}", self.fibgroup.borrow()); - //debug!("\nnew:\n{}", fibgroup); + trace!("Fibgroup for nhop {self}\nchanged. Will replace.."); + trace!("\nold:\n{}", self.fibgroup.borrow()); + trace!("\nnew:\n{}", fibgroup); self.fibgroup.replace(fibgroup); } else { - //debug!("Fibgroup for nhop {self} did NOT change"); + trace!("Fibgroup for nhop {self} did NOT change"); } changed } diff --git a/routing/src/rib/vrf.rs b/routing/src/rib/vrf.rs index 0db143d87..6b70dc24b 100644 --- a/routing/src/rib/vrf.rs +++ b/routing/src/rib/vrf.rs @@ -438,9 +438,9 @@ impl Vrf { if let Some(fibw) = &mut self.fibw { let mut count = 0; for nhop in changed { - let fibgroup = &*nhop.fibgroup.borrow(); + let fibgroup = &nhop.fibgroup.borrow(); debug!("Updating fib group for nhop {}...", nhop.key); - fibw.register_fibgroup(&nhop.as_ref().key, fibgroup, false); + fibw.register_fibgroup(&nhop.key, fibgroup, false); count += 1; } if count > 0 { From 56abb27f3627559dba6311d9a3ed6e1d68b5ebee Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Fri, 17 Oct 2025 16:58:07 +0200 Subject: [PATCH 31/35] feat(routing): revert to UnsafeCell in fibgroups This is a manual revert of 6ca1bfea7362e36a4712e8fc25cfd6ace15f1b65. RefCell cannot be used with more than one thread, even if threads would only ever borrow immutably the types within an Arc since the state kept within the RefCell to check borrows is not thread-safe and mutates, making it unusable. Signed-off-by: Fredi Raspall --- routing/src/fib/fibgroupstore.rs | 72 +++++++++++++++++--------------- routing/src/fib/fibtype.rs | 11 ++--- 2 files changed, 41 insertions(+), 42 deletions(-) diff --git a/routing/src/fib/fibgroupstore.rs b/routing/src/fib/fibgroupstore.rs index 906117ed7..465c78abf 100644 --- a/routing/src/fib/fibgroupstore.rs +++ b/routing/src/fib/fibgroupstore.rs @@ -9,21 +9,21 @@ //! Fibgroups when routing changes occur for all the affected routes without needing to explicitly //! change any of the routes. This allows, upon routing changes, updating the FIB in O(Nh) instead //! of O(Routes), potentially reducing the number of updates by several orders of magnitude. -//! This is achieved by means of an `RefCell`, which allows us to mutate fibgroups. +//! This is achieved by means of an `UnsafeCell`, which allows us to mutate fibgroups. //! This data structure is, therefore, NOT thread-safe. Thread-safety is achieved by wrapping //! this structure in left-right. //! -//! Any modification of the FibGroupStore or the `Rc>`s it contains +//! Any modification of the FibGroupStore or the `Rc>`s it contains //! must be done while holding a `left_right::WriteGuard` to the `Fib` that owns it. //! -//! Any use of the `FibGroupStore` or the `Rc>`s it contains +//! Any use of the `FibGroupStore` or the `Rc>`s it contains //! must be done while holding a `left_right::ReadGuard` or `left_right::WriteGuard` to //! the `Fib` that owns it. use crate::fib::fibobjects::{FibEntry, FibGroup}; use crate::rib::nexthop::NhopKey; use ahash::RandomState; -use std::cell::{Ref, RefCell}; +use std::cell::UnsafeCell; use std::collections::HashMap; use std::rc::Rc; use thiserror::Error; @@ -38,7 +38,7 @@ pub enum FibError { } #[derive(Debug, Default)] -pub(crate) struct FibGroupStore(HashMap>, RandomState>); +pub(crate) struct FibGroupStore(HashMap>, RandomState>); impl FibGroupStore { #[must_use] @@ -46,7 +46,7 @@ impl FibGroupStore { let mut store = Self(HashMap::with_hasher(RandomState::with_seed(0))); unsafe { // This is safe because we have exclusive access to the store we just created - // so no one else can have a reference to any Rc's it contains + // so no one else can see it. store.add_mod_group(&NhopKey::with_drop(), FibGroup::drop_fibgroup()); } store @@ -58,7 +58,7 @@ impl FibGroupStore { } #[must_use] /// get an Rc for the drop `Fibgroup`. The drop fibgroup is unique. - pub fn get_drop_fibgroup_ref(&self) -> Rc> { + pub fn get_drop_fibgroup_ref(&self) -> Rc> { self.get_ref(&NhopKey::with_drop()) .unwrap_or_else(|| unreachable!()) } @@ -73,15 +73,17 @@ impl FibGroupStore { /// /// In our actual usage, this is the case because we call this function /// when everything is protected by the `left_right::WriteGuard` of the `Fib` - /// Any use of the `Rc>` entries in the `Fib` is protected by + /// Any use of the `Rc>` entries in the `Fib` is protected by /// `left_right::ReadGuard` or `left_right::WriteGuard` to the `Fib` that owns it. /// //////////////////////////////////////////////////////////////////////////////// pub(super) unsafe fn add_mod_group(&mut self, key: &NhopKey, fibgroup: FibGroup) { if let Some(group) = self.0.get(key) { - *group.borrow_mut() = fibgroup; + unsafe { + *group.get() = fibgroup; + } } else { - let fg = Rc::new(RefCell::new(fibgroup)); + let fg = Rc::new(UnsafeCell::new(fibgroup)); self.0.insert(key.clone(), fg); } } @@ -90,14 +92,16 @@ impl FibGroupStore { /// used by `FibRoutes` to point to the current `FibGroups`. //////////////////////////////////////////////////////////////////////////////// #[must_use] - fn get_ref(&self, key: &NhopKey) -> Option>> { + fn get_ref(&self, key: &NhopKey) -> Option>> { self.0.get(key).map(|group| Rc::clone(group)) } + #[cfg(test)] #[must_use] - pub(crate) fn get(&self, key: &NhopKey) -> Option> { - self.0.get(key).map(|group| group.borrow()) + pub(crate) fn get(&self, key: &NhopKey) -> Option<&FibGroup> { + self.0.get(key).map(|group| unsafe { &*group.get() }) } + pub(crate) fn del(&mut self, key: &NhopKey) { if key == &NhopKey::with_drop() { return; @@ -133,34 +137,34 @@ impl FibGroupStore { //////////////////////////////////////////////////////////////////////////////////////////////////// /// Iterate over the `FibGroups` in the store //////////////////////////////////////////////////////////////////////////////////////////////////// - pub(crate) fn values(&self) -> impl Iterator> { - self.0.values().map(|group| group.borrow()) + pub(crate) fn values(&self) -> impl Iterator { + unsafe { self.0.values().map(|group| &*group.get()) } } //////////////////////////////////////////////////////////////////////////////////////////////////// /// Iterate over the `FibGroups` in the store //////////////////////////////////////////////////////////////////////////////////////////////////// #[cfg(test)] - pub(crate) fn iter(&self) -> impl Iterator)> { - self.0.iter().map(|(key, group)| (key, group.borrow())) + pub(crate) fn iter(&self) -> impl Iterator { + unsafe { self.0.iter().map(|(key, group)| (key, &*group.get())) } } } #[derive(Debug, Clone)] -pub struct FibRoute(Vec>>); +pub struct FibRoute(Vec>>); impl FibRoute { #[must_use] pub(crate) fn new() -> Self { Self(vec![]) } #[must_use] - pub fn with_fibgroup(fg_ref: Rc>) -> Self { + pub fn with_fibgroup(fg_ref: Rc>) -> Self { Self(vec![fg_ref]) } #[cfg(test)] /// Add a reference to a `FibGroup` to a `FibRoute` - pub(crate) fn add_fibgroup_ref(&mut self, fg_ref: Rc>) { + pub(crate) fn add_fibgroup_ref(&mut self, fg_ref: Rc>) { self.0.push(fg_ref); } @@ -169,7 +173,9 @@ impl FibRoute { ///////////////////////////////////////////////////////////////////////////////////////////////// #[must_use] pub(crate) fn len(&self) -> usize { - self.0.iter().fold(0, |val, g| val + g.borrow().len()) + self.0 + .iter() + .fold(0, |val, g| unsafe { val + (&*g.get()).len() }) } ///////////////////////////////////////////////////////////////////////////////////////////////// @@ -198,12 +204,12 @@ impl FibRoute { /// 0 1 2 3 4 | 0 1 2 | 0 1 | 0 1 2 | real entry indices within each group. //////////////////////////////////////////////////////////////////////////////////////////////// #[must_use] - pub(crate) fn get_fibentry(&self, index: usize) -> Option> { + pub(crate) fn get_fibentry(&self, index: usize) -> Option<&FibEntry> { let mut index = index; for g in self.0.iter() { - let group = g.borrow(); + let group = unsafe { &*g.get() }; if index < group.len() { - return Some(Ref::map(group, |g| &g.entries[index])); + return Some(&group.entries[index]); } index -= group.len(); } @@ -213,8 +219,8 @@ impl FibRoute { //////////////////////////////////////////////////////////////////////////////////////////////// /// Provide iterator over the `FibGroups` that a `Fibroute` refers to //////////////////////////////////////////////////////////////////////////////////////////////// - pub(crate) fn iter(&self) -> impl Iterator> { - self.0.iter().map(|group| group.borrow()) + pub(crate) fn iter(&self) -> impl Iterator { + unsafe { self.0.iter().map(|group| &*group.get()) } } } @@ -241,10 +247,7 @@ pub mod tests { use net::interface::InterfaceIndex; use crate::fib::fibgroupstore::{FibError, FibGroupStore, FibRoute}; - use crate::fib::fibobjects::EgressObject; - use crate::fib::fibobjects::FibEntry; - use crate::fib::fibobjects::FibGroup; - use crate::fib::fibobjects::PktInstruction; + use crate::fib::fibobjects::{EgressObject, FibEntry, FibGroup, PktInstruction}; use crate::rib::nexthop::NhopKey; use std::net::IpAddr; @@ -452,10 +455,11 @@ pub mod tests { .unwrap(); assert_eq!(fibroute.0.len(), 4); - assert_eq!(*fibroute.0[0].borrow(), *store.get(&key1).unwrap()); - assert_eq!(*fibroute.0[1].borrow(), *store.get(&key2).unwrap()); - assert_eq!(*fibroute.0[2].borrow(), *store.get(&key3).unwrap()); - assert_eq!(*fibroute.0[3].borrow(), *store.get(&key4).unwrap()); + assert_eq!(unsafe { &*fibroute.0[0].get() }, store.get(&key1).unwrap()); + assert_eq!(unsafe { &*fibroute.0[1].get() }, store.get(&key2).unwrap()); + assert_eq!(unsafe { &*fibroute.0[2].get() }, store.get(&key3).unwrap()); + assert_eq!(unsafe { &*fibroute.0[3].get() }, store.get(&key4).unwrap()); + println!("{fibroute:#?}"); // Attempt to create a Fibroute with a key for which no fibgroup exists: should fail diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index a4d5d6f25..8d58b968b 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -7,8 +7,6 @@ use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use left_right_tlcache::Identity; - -use std::cell::Ref; use std::hash::Hash; use std::net::IpAddr; use std::rc::Rc; @@ -214,7 +212,7 @@ impl Fib { } /// Iterate over [`FibGroup`]s - pub fn group_iter(&self) -> impl Iterator> { + pub fn group_iter(&self) -> impl Iterator { self.groupstore.values() } @@ -258,10 +256,7 @@ impl Fib { /// computing a hash on the invariant header fields of the IP and L4 headers. #[must_use] #[allow(clippy::cast_possible_truncation)] - pub fn lpm_entry( - &self, - packet: &Packet, - ) -> Option> { + pub fn lpm_entry(&self, packet: &Packet) -> Option<&FibEntry> { let (_, entry) = self.lpm_entry_prefix(packet); entry } @@ -271,7 +266,7 @@ impl Fib { pub fn lpm_entry_prefix( &self, packet: &Packet, - ) -> (Prefix, Option>) { + ) -> (Prefix, Option<&FibEntry>) { if let Some(destination) = packet.ip_destination() { let (prefix, route) = self.lpm_with_prefix(&destination); match route.len() { From d4034c8dc646562af8f99ba2908b6435e2bd0f38 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 8 Oct 2025 14:51:46 +0200 Subject: [PATCH 32/35] feat(routing): misc fixes in Fib - clarify documentation. - ensure that installed fibgroups always have at least 1 fibentry. - ensure that fibroutes refer at least to one fibgroup. - Add guarded methods for lpm, that keep a guarded reference to the fib entry to use to forward a packet. These are NOT yet used and actually bring no extra safety, but allow performing lpm ops without explicitly "entering" the left-right protected fib. Signed-off-by: Fredi Raspall --- dataplane/src/packet_processor/ipforward.rs | 29 ++--- routing/src/fib/fibgroupstore.rs | 130 ++++++++++---------- routing/src/fib/fibtype.rs | 99 +++++++++++---- routing/src/rib/rib2fib.rs | 3 +- 4 files changed, 154 insertions(+), 107 deletions(-) diff --git a/dataplane/src/packet_processor/ipforward.rs b/dataplane/src/packet_processor/ipforward.rs index 95819850b..274ef9db0 100644 --- a/dataplane/src/packet_processor/ipforward.rs +++ b/dataplane/src/packet_processor/ipforward.rs @@ -82,26 +82,21 @@ impl IpForwarder { return; }; - /* Perform lookup in the fib */ + /* Perform lookup in the fib. This always returns a FibEntry */ let (prefix, fibentry) = fib.lpm_entry_prefix(packet); - if let Some(fibentry) = &fibentry { - debug!("{nfi}: Packet hits prefix {prefix} in fib {fibkey}"); - debug!("{nfi}: Entry is:\n{fibentry}"); - - /* decrement packet TTL, unless the packet is for us */ - if !fibentry.is_iplocal() { - Self::decrement_ttl(packet, dst); - if packet.is_done() { - debug!("TTL/Hop-count limit exceeded!"); - return; - } + debug!("{nfi}: Packet hits prefix {prefix} in fib {fibkey}"); + debug!("{nfi}: Entry is:\n{fibentry}"); + + /* decrement packet TTL, unless the packet is for us */ + if !fibentry.is_iplocal() { + Self::decrement_ttl(packet, dst); + if packet.is_done() { + debug!("TTL/Hop-count limit exceeded!"); + return; } - /* execute instructions according to FIB */ - self.packet_exec_instructions(packet, fibentry, fib.get_vtep()); - } else { - debug!("Could not get fib group for {prefix}. Will drop packet..."); - packet.done(DoneReason::InternalFailure); } + /* execute instructions according to FIB */ + self.packet_exec_instructions(packet, fibentry, fib.get_vtep()); } /// Execute a local packet instruction diff --git a/routing/src/fib/fibgroupstore.rs b/routing/src/fib/fibgroupstore.rs index 465c78abf..1d3fd9c34 100644 --- a/routing/src/fib/fibgroupstore.rs +++ b/routing/src/fib/fibgroupstore.rs @@ -10,15 +10,16 @@ //! change any of the routes. This allows, upon routing changes, updating the FIB in O(Nh) instead //! of O(Routes), potentially reducing the number of updates by several orders of magnitude. //! This is achieved by means of an `UnsafeCell`, which allows us to mutate fibgroups. -//! This data structure is, therefore, NOT thread-safe. Thread-safety is achieved by wrapping -//! this structure in left-right. +//! This data structure is, therefore, **NOT** thread-safe. //! -//! Any modification of the FibGroupStore or the `Rc>`s it contains -//! must be done while holding a `left_right::WriteGuard` to the `Fib` that owns it. +//! Safety is achieved by wrapping the structure in left-right. Safety for readers is granted +//! by the fact that the fib will not be modifed as long as readers have a read guard to it. +//! It is also guaranteed despite the use of Rc because readers never get to see those Rc's +//! (they are internal) and only the left-right writer deals with cloning them. +//! An implementation without the protection given by left-right would require, instead, +//! something like `Arc>`. //! -//! Any use of the `FibGroupStore` or the `Rc>`s it contains -//! must be done while holding a `left_right::ReadGuard` or `left_right::WriteGuard` to -//! the `Fib` that owns it. +//! **NOTE**: Throughout the documentation it is assumed that the structure is wrapped in left-right. use crate::fib::fibobjects::{FibEntry, FibGroup}; use crate::rib::nexthop::NhopKey; @@ -29,11 +30,11 @@ use std::rc::Rc; use thiserror::Error; #[allow(unused)] -use tracing::{debug, warn}; +use tracing::{debug, error, warn}; #[derive(Error, Debug, PartialEq)] pub enum FibError { - #[error("Failed to find fibgroup for nh {0}")] + #[error("Failed to find fibgroup for nh key {0:?}")] NoFibGroup(NhopKey), } @@ -44,11 +45,7 @@ impl FibGroupStore { #[must_use] pub(crate) fn new() -> Self { let mut store = Self(HashMap::with_hasher(RandomState::with_seed(0))); - unsafe { - // This is safe because we have exclusive access to the store we just created - // so no one else can see it. - store.add_mod_group(&NhopKey::with_drop(), FibGroup::drop_fibgroup()); - } + store.add_mod_group(&NhopKey::with_drop(), FibGroup::drop_fibgroup()); store } #[must_use] @@ -65,19 +62,13 @@ impl FibGroupStore { //////////////////////////////////////////////////////////////////////////////// /// Add a `FibGroup` for a given `NhopKey` or replace it if it exists. - /// - /// Safety: - /// - /// This function is only safe if there are no references to the `Rc` - /// indexed by `key`, as returned by `get_ref`. - /// - /// In our actual usage, this is the case because we call this function - /// when everything is protected by the `left_right::WriteGuard` of the `Fib` - /// Any use of the `Rc>` entries in the `Fib` is protected by - /// `left_right::ReadGuard` or `left_right::WriteGuard` to the `Fib` that owns it. - /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////// - pub(super) unsafe fn add_mod_group(&mut self, key: &NhopKey, fibgroup: FibGroup) { + pub(super) fn add_mod_group(&mut self, key: &NhopKey, fibgroup: FibGroup) { + if fibgroup.is_empty() { + error!("Refusing to add fibgroup without entries for key {key:?}"); + return; + } if let Some(group) = self.0.get(key) { unsafe { *group.get() = fibgroup; @@ -90,6 +81,8 @@ impl FibGroupStore { //////////////////////////////////////////////////////////////////////////////// /// Get a refcounted reference to the `Fibgroup` for a given `NhopKey`. This is /// used by `FibRoutes` to point to the current `FibGroups`. + /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////// #[must_use] fn get_ref(&self, key: &NhopKey) -> Option>> { @@ -102,6 +95,11 @@ impl FibGroupStore { self.0.get(key).map(|group| unsafe { &*group.get() }) } + //////////////////////////////////////////////////////////////////////////////// + /// Remove the fibgroup with the provided key. + /// + /// Safety: Only the left-right writer should use this method. + //////////////////////////////////////////////////////////////////////////////// pub(crate) fn del(&mut self, key: &NhopKey) { if key == &NhopKey::with_drop() { return; @@ -121,6 +119,8 @@ impl FibGroupStore { /// Remove unused `FibGroup`s. This should not be needed, but we may use it to expedite N deletions /// in batch, as it avoids lookups (at the expense of traversal). /// Returns the number of groups removed. + /// + /// Safety: Only the left-right writer should use this method. //////////////////////////////////////////////////////////////////////////////////////////////////// pub fn purge(&mut self) -> usize { let len = self.len(); @@ -178,6 +178,13 @@ impl FibRoute { .fold(0, |val, g| unsafe { val + (&*g.get()).len() }) } + #[allow(unused)] + #[must_use] + pub(crate) fn has_entries(&self) -> bool { + // empty vector returns false + self.0.iter().any(|g| unsafe { !(&*g.get()).is_empty() }) + } + ///////////////////////////////////////////////////////////////////////////////////////////////// /// Tells the number of `FibGroup`s that a `FibRoute` has ///////////////////////////////////////////////////////////////////////////////////////////////// @@ -202,18 +209,25 @@ impl FibRoute { /// to a real one, in the corresponding fibgroup, as shown in the next example for 4 groups. /// 0 1 2 3 4 | 5 6 7 | 8 9 | 10 11 12 | virtual entry indices /// 0 1 2 3 4 | 0 1 2 | 0 1 | 0 1 2 | real entry indices within each group. + /// + /// Panics: + /// This method will panic if index is >= FibRoute::len(). This is so to keep this method + /// infallible, which simplifies its use in several cases. //////////////////////////////////////////////////////////////////////////////////////////////// #[must_use] - pub(crate) fn get_fibentry(&self, index: usize) -> Option<&FibEntry> { + pub(crate) fn get_fibentry(&self, index: usize) -> &FibEntry { let mut index = index; for g in self.0.iter() { let group = unsafe { &*g.get() }; if index < group.len() { - return Some(&group.entries[index]); + return &group.entries[index]; } index -= group.len(); } - None + // There cannot exist a route without fib groups. + // Fibgroups must have at least one entry. + // Developer error: did not check FibRoute::len() + unreachable!() } //////////////////////////////////////////////////////////////////////////////////////////////// @@ -222,9 +236,7 @@ impl FibRoute { pub(crate) fn iter(&self) -> impl Iterator { unsafe { self.0.iter().map(|group| &*group.get()) } } -} -impl FibRoute { #[must_use] /////////////////////////////////////////////////////////////////////////////////// /// Creates a `FibRoute` with the `FibGroups` corresponding to a set of `NhopKey`s. @@ -254,7 +266,7 @@ pub mod tests { use std::str::FromStr; // builds fib entry with single egress instruction - fn build_fib_entry_egress(ifindex: u32, address: &str, ifname: &str) -> FibEntry { + pub(crate) fn build_fib_entry_egress(ifindex: u32, address: &str, ifname: &str) -> FibEntry { let addr = Some(IpAddr::from_str(address).unwrap()); let ifname = Some(ifname.to_string()); let inst = PktInstruction::Egress(EgressObject::new( @@ -265,7 +277,7 @@ pub mod tests { FibEntry::with_inst(inst) } // builds a fibgroup with several entries - fn build_fibgroup(entries: &[FibEntry]) -> FibGroup { + pub(crate) fn build_fibgroup(entries: &[FibEntry]) -> FibGroup { let mut fibgroup = FibGroup::new(); fibgroup.entries.extend_from_slice(entries); fibgroup @@ -284,9 +296,7 @@ pub mod tests { let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); // store the fibgroup - unsafe { - store.add_mod_group(&nhkey, fibgroup); - } + store.add_mod_group(&nhkey, fibgroup); { // retrieve the fibgrop from the store from its key @@ -311,13 +321,11 @@ pub mod tests { // mutate/replace fibgroup, without modifying fibroute let entry2 = build_fib_entry_egress(100, "10.0.2.2", "eth1"); let fibgroup = FibGroup::with_entry(entry2.clone()); - unsafe { - store.add_mod_group(&nhkey, fibgroup); - } + store.add_mod_group(&nhkey, fibgroup); // check that the fibroute has been internally modified - let found1 = fibroute.get_fibentry(0).unwrap(); - let found2 = fibroute2.get_fibentry(0).unwrap(); + let found1 = fibroute.get_fibentry(0); + let found2 = fibroute2.get_fibentry(0); assert_eq!(*found1, *found2); assert_eq!(*found1, entry2); @@ -350,12 +358,11 @@ pub mod tests { // create a fibgroup store and store the fibgroups with the respective nhop keys let mut store = FibGroupStore::new(); - unsafe { - store.add_mod_group(&key1, g1.clone()); - store.add_mod_group(&key2, g2.clone()); - store.add_mod_group(&key3, g3.clone()); - store.add_mod_group(&key4, g4.clone()); - } + store.add_mod_group(&key1, g1.clone()); + store.add_mod_group(&key2, g2.clone()); + store.add_mod_group(&key3, g3.clone()); + store.add_mod_group(&key4, g4.clone()); + assert_eq!(store.len(), 4 + 1); // +1 is for drop group println!("{store:#?}"); @@ -375,16 +382,15 @@ pub mod tests { // select one entry in the fibroute by index and check that it is correct // this route has all of the fibgroups - assert_eq!(&*fibroute.get_fibentry(0).unwrap(), &e1); - assert_eq!(&*fibroute.get_fibentry(0).unwrap(), &e1); - assert_eq!(&*fibroute.get_fibentry(1).unwrap(), &e2); - assert_eq!(&*fibroute.get_fibentry(2).unwrap(), &e3); - assert_eq!(&*fibroute.get_fibentry(3).unwrap(), &e4); - assert_eq!(&*fibroute.get_fibentry(4).unwrap(), &e5); - assert_eq!(&*fibroute.get_fibentry(5).unwrap(), &e6); - assert_eq!(&*fibroute.get_fibentry(6).unwrap(), &e7); - assert_eq!(&*fibroute.get_fibentry(7).unwrap(), &e8); - assert!(fibroute.get_fibentry(8).is_none()); + assert_eq!(&*fibroute.get_fibentry(0), &e1); + assert_eq!(&*fibroute.get_fibentry(0), &e1); + assert_eq!(&*fibroute.get_fibentry(1), &e2); + assert_eq!(&*fibroute.get_fibentry(2), &e3); + assert_eq!(&*fibroute.get_fibentry(3), &e4); + assert_eq!(&*fibroute.get_fibentry(4), &e5); + assert_eq!(&*fibroute.get_fibentry(5), &e6); + assert_eq!(&*fibroute.get_fibentry(6), &e7); + assert_eq!(&*fibroute.get_fibentry(7), &e8); // attempt to remove fibgroups: none should be removed from the store store.del(&key1); @@ -439,12 +445,10 @@ pub mod tests { // create a fibgroup store and store the fibgroups with the respective nhop keys let mut store = FibGroupStore::new(); - unsafe { - store.add_mod_group(&key1, g1.clone()); - store.add_mod_group(&key2, g2.clone()); - store.add_mod_group(&key3, g3.clone()); - store.add_mod_group(&key4, g4.clone()); - } + store.add_mod_group(&key1, g1.clone()); + store.add_mod_group(&key2, g2.clone()); + store.add_mod_group(&key3, g3.clone()); + store.add_mod_group(&key4, g4.clone()); assert_eq!(store.len(), 4 + 1); // +1 is for drop group // create a FibRoute from the keys diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index 8d58b968b..cd4fb4d07 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -123,11 +123,17 @@ impl Fib { /// Add a [`FibRoute`] fn build_add_fibroute(&mut self, prefix: Prefix, keys: &[NhopKey]) { - let Ok(route) = FibRoute::from_nhopkeys(&self.groupstore, keys) else { - error!("Failed to build fibroute for keys {keys:#?}"); + if keys.is_empty() { + error!("Rejecting fibroute creation: no keys provided"); return; - }; - self.add_fibroute(prefix, route); + } + match FibRoute::from_nhopkeys(&self.groupstore, keys) { + Ok(route) => { + debug_assert!(route.len() > 0); + self.add_fibroute(prefix, route); + } + Err(e) => error!("Failed to build fibroute for keys {keys:#?}: {e}"), + } } /// Delete the [`FibRoute`] for a prefix @@ -254,33 +260,24 @@ impl Fib { /// However, instead of returning the entire [`FibRoute`], returns a single [`FibEntry`] out of /// those in the `FibGroup`s that make up the [`FibRoute`]. The entry selected is chosen by /// computing a hash on the invariant header fields of the IP and L4 headers. - #[must_use] - #[allow(clippy::cast_possible_truncation)] - pub fn lpm_entry(&self, packet: &Packet) -> Option<&FibEntry> { - let (_, entry) = self.lpm_entry_prefix(packet); - entry - } - - /// Same as `lpm_entry` but reporting prefix #[allow(clippy::cast_possible_truncation)] pub fn lpm_entry_prefix( &self, packet: &Packet, - ) -> (Prefix, Option<&FibEntry>) { + ) -> (Prefix, &FibEntry) { if let Some(destination) = packet.ip_destination() { let (prefix, route) = self.lpm_with_prefix(&destination); - match route.len() { - 0 => { - warn!("Can't forward packet: no route to {prefix}. This is a bug."); - (prefix, None) - } - 1 => (prefix, route.get_fibentry(0)), - k => { - let entry_index = packet.packet_hash_ecmp(0, (k - 1) as u8); - trace!("Selected FibEntry {entry_index}/{k} to forward packet"); - (prefix, route.get_fibentry(entry_index as usize)) - } + let num_entries = route.len(); + if num_entries == 0 { + let bad = "Warning, hit route without fibgroups/entries. This is a bug."; + warn!("{bad}"); + panic!("{bad}"); + } + let mut entry_index = 0; + if num_entries > 1 { + entry_index = packet.packet_hash_ecmp(0, (num_entries - 1) as u8); } + (prefix, route.get_fibentry(entry_index as usize)) } else { error!("Failed to get destination IP address!"); unreachable!() @@ -300,9 +297,9 @@ enum FibChange { impl Absorb for Fib { fn absorb_first(&mut self, change: &mut FibChange, _: &Self) { match change { - FibChange::RegisterFibGroup((key, fibgroup)) => unsafe { + FibChange::RegisterFibGroup((key, fibgroup)) => { self.groupstore.add_mod_group(key, fibgroup.clone()); - }, + } FibChange::UnregisterFibGroup(key) => { self.groupstore.del(key); } @@ -358,6 +355,10 @@ impl FibWriter { } } pub fn add_fibroute(&mut self, prefix: Prefix, keys: Vec, publish: bool) { + if keys.is_empty() { + error!("Rejected route to prefix {prefix}: no next-hop keys provided"); + return; + } self.0.append(FibChange::AddFibRoute((prefix, keys))); if publish { self.0.publish(); @@ -413,6 +414,52 @@ impl FibReader { pub fn factory(&self) -> FibReaderFactory { FibReaderFactory(self.0.factory()) } + + /// Get a reference to the `FibRoute` best matching address `destination` + /// Return value: this method may only return None if the `FibReader` cannot + /// be entered, which should only occur if the `FibWriter` has been dropped. + /// + /// Safety: the `FibRoute` reference returned will remain valid and immutable + /// as long as the returned `ReadGuard` is alive. + pub fn lpm_route(&self, destination: IpAddr) -> Option> { + self.enter() + .map(|guard| ReadGuard::map(guard, |fib| fib.lpm(&destination))) + } + + /// Same as `FibReader::lpm_route`, but reporting the longest `Prefix` matched. + /// Notes: no prefix will be returned if we fail to "enter" in the fib. + pub fn lpm_route_with_prefix( + &self, + destination: IpAddr, + ) -> Option<(Prefix, ReadGuard<'_, FibRoute>)> { + let mut prefix = Prefix::root_v4(); + let guarded_route = self.enter().map(|guard| { + ReadGuard::map(guard, |fib| { + let (hit, route) = fib.lpm_with_prefix(&destination); + prefix = hit; + route + }) + }); + guarded_route.map(|guarded_route| (prefix, guarded_route)) + } + + /// Similar to `FibReader::lpm_route_with_prefix()`, but receing a `Packet` and selecting + /// a `FibEntry` based on a hash of the packet if more than one FibEntries could be used to + /// forward the packet + pub fn lpm_entry_prefix( + &self, + packet: &Packet, + ) -> Option<(Prefix, ReadGuard<'_, FibEntry>)> { + let mut prefix = Prefix::root_v4(); + let guarded_entry = self.0.enter().map(|guard| { + ReadGuard::map(guard, |fib| { + let (hit, entry) = fib.lpm_entry_prefix(packet); + prefix = hit; + entry + }) + }); + guarded_entry.map(|guarded_entry| (prefix, guarded_entry)) + } } // make FibReader a zero-cost wrap of ReadHandle diff --git a/routing/src/rib/rib2fib.rs b/routing/src/rib/rib2fib.rs index 3cca2ab7e..4f3e01b58 100644 --- a/routing/src/rib/rib2fib.rs +++ b/routing/src/rib/rib2fib.rs @@ -103,7 +103,8 @@ impl Nhop { } ////////////////////////////////////////////////////////////////////// - /// Determine instructions for a next-hop and build its fibgroup + /// Determine instructions for a next-hop and build its `FibGroup`. + /// Returns true if the `Fibgroup` associated to a next-hop changed. ////////////////////////////////////////////////////////////////////// pub(crate) fn set_fibgroup(&self, rstore: &RmacStore) -> bool { // determine nhop pkt instructions. This is independent of the routing table From 912c7599450e984604e4a19b0c5b7b4a1ccd8455 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Wed, 8 Oct 2025 16:41:42 +0200 Subject: [PATCH 33/35] feat(routing): add fib concurrency tests Two tests are added: - one to validate new methods that provide guarded fibentries. - one where multiple workers repeatedly do concurrent lookups on the same fib and for the same destination, while another thread adds/removes/modifies the route that best matches that destination. Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + routing/Cargo.toml | 1 + routing/src/fib/fibgroupstore.rs | 3 +- routing/src/fib/fibobjects.rs | 4 + routing/src/fib/mod.rs | 1 + routing/src/fib/test.rs | 283 +++++++++++++++++++++++++++++++ 6 files changed, 292 insertions(+), 1 deletion(-) create mode 100644 routing/src/fib/test.rs diff --git a/Cargo.lock b/Cargo.lock index 2469cd528..4571cd463 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1293,6 +1293,7 @@ dependencies = [ "mio", "netdev", "procfs", + "rand 0.9.2", "serde", "thiserror 2.0.17", "tokio", diff --git a/routing/Cargo.toml b/routing/Cargo.toml index f984ccce8..113fbd971 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -41,5 +41,6 @@ netdev = { workspace = true } [dev-dependencies] bolero = { workspace = true, default-features = false } lpm = { workspace = true, features = ["testing"] } +rand = { workspace = true, default-features = false, features = ["thread_rng"] } tracing-test = { workspace = true, features = [] } diff --git a/routing/src/fib/fibgroupstore.rs b/routing/src/fib/fibgroupstore.rs index 1d3fd9c34..1705dc16f 100644 --- a/routing/src/fib/fibgroupstore.rs +++ b/routing/src/fib/fibgroupstore.rs @@ -127,7 +127,8 @@ impl FibGroupStore { self.0.retain(|key, group| { let keep = Rc::strong_count(group) > 1 || key == &NhopKey::with_drop(); if !keep { - debug!("Will purge fibgroup for nhop '{key}'"); + #[cfg(not(test))] + debug!("Purging fibgroup for nhop '{key}'"); } keep }); diff --git a/routing/src/fib/fibobjects.rs b/routing/src/fib/fibobjects.rs index 788025b07..0371b2629 100644 --- a/routing/src/fib/fibobjects.rs +++ b/routing/src/fib/fibobjects.rs @@ -10,6 +10,7 @@ use net::interface::InterfaceIndex; use std::net::IpAddr; #[derive(Debug, Default, Clone, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] /// An `EgressObject` indicates the interface over which a packet /// has to be sent and, optionally, a next-hop ip address. If /// no address is provided, ND/ARP is required. @@ -129,6 +130,8 @@ impl FibGroup { } #[derive(Debug, Default, Clone, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] + /// A Fib entry is made of a sequence of [`PktInstruction`] s to be executed for an IP packet /// in order to forward it. pub struct FibEntry { @@ -218,6 +221,7 @@ impl FibEntry { } #[derive(Clone, Default, Debug, Ord, PartialOrd, Eq, PartialEq)] +#[cfg_attr(test, derive(Hash))] #[allow(unused)] /// A `PktInstruction` represents an action to be performed by the packet processor on a packet. pub enum PktInstruction { diff --git a/routing/src/fib/mod.rs b/routing/src/fib/mod.rs index 9e7549480..da56a8854 100644 --- a/routing/src/fib/mod.rs +++ b/routing/src/fib/mod.rs @@ -7,6 +7,7 @@ pub mod fibgroupstore; pub mod fibobjects; pub mod fibtable; pub mod fibtype; +mod test; use tracectl::trace_target; trace_target!("fib", LevelFilter::WARN, &["pipeline"]); diff --git a/routing/src/fib/test.rs b/routing/src/fib/test.rs new file mode 100644 index 000000000..a1497e2bd --- /dev/null +++ b/routing/src/fib/test.rs @@ -0,0 +1,283 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Open Network Fabric Authors + +//! Fib module tests + +#[cfg(test)] +pub mod tests { + use crate::fib::fibobjects::FibEntry; + use crate::fib::fibobjects::FibGroup; + use crate::fib::fibobjects::PktInstruction; + use net::ip::NextHeader; + use net::packet::Packet; + use net::packet::test_utils::build_test_ipv4_packet_with_transport; + use net::udp::UdpPort; + use net::{buffer::TestBuffer, interface::InterfaceIndex}; + + use crate::fib::fibtype::FibKey; + use crate::fib::fibtype::FibWriter; + use crate::rib::nexthop::NhopKey; + use lpm::prefix::IpAddr; + use lpm::prefix::Prefix; + + use rand::Rng; + use rand::rngs::ThreadRng; + use std::str::FromStr; + use std::sync::Arc; + use std::sync::atomic::AtomicU16; + use std::thread::Builder; + use std::{collections::HashMap, collections::HashSet, sync::atomic::Ordering}; + + use crate::fib::fibgroupstore::tests::build_fib_entry_egress; + use crate::fib::fibgroupstore::tests::build_fibgroup; + + /// An object that contains a preset collection of entries and fibgroups. + /// This is shared by fuzzer and workers. Fuzzer uses it to randomly select fibgroups to update the fib. + /// Workers use it to check what they read. + struct RandomRouter { + entries: HashSet, + fibgroups: HashMap, + } + impl RandomRouter { + fn load() -> Self { + let mut entries = HashSet::new(); + let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); + let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); + let e3 = build_fib_entry_egress(3, "10.0.3.1", "eth3"); + let e4 = build_fib_entry_egress(4, "10.0.3.4", "eth4"); + let e5 = build_fib_entry_egress(5, "10.0.3.5", "eth5"); + let e6 = build_fib_entry_egress(6, "10.0.3.6", "eth6"); + let e7 = build_fib_entry_egress(7, "10.0.3.7", "eth7"); + let e8 = build_fib_entry_egress(8, "10.0.3.8", "eth8"); + + entries.insert(e1.clone()); + entries.insert(e2.clone()); + entries.insert(e3.clone()); + entries.insert(e4.clone()); + entries.insert(e5.clone()); + entries.insert(e6.clone()); + entries.insert(e7.clone()); + entries.insert(e8.clone()); + + let mut fibgroups = HashMap::new(); + fibgroups.insert(1, build_fibgroup(&[e1.clone(), e2.clone()])); + fibgroups.insert(2, build_fibgroup(&[e4.clone()])); + fibgroups.insert(3, build_fibgroup(&[e7.clone()])); + fibgroups.insert(4, build_fibgroup(&[e8.clone()])); + fibgroups.insert(5, build_fibgroup(&[e5.clone()])); + fibgroups.insert(6, build_fibgroup(&[e5.clone(), e6.clone()])); + fibgroups.insert(7, build_fibgroup(&[e3.clone()])); + fibgroups.insert( + 8, + build_fibgroup(&[e3.clone(), e4.clone(), e5.clone(), e6.clone(), e7.clone()]), + ); + + Self { entries, fibgroups } + } + fn random_pick_fibgroup(&self, rng: &mut ThreadRng) -> &FibGroup { + let chosen = rng.random_range(1..=self.fibgroups.len()); + self.fibgroups.get(&chosen).unwrap() + } + } + + fn get_entry_interface_index(entry: &FibEntry) -> InterfaceIndex { + if let PktInstruction::Egress(egress) = &entry.instructions[0] { + egress.ifindex.unwrap() + } else { + unreachable!() + } + } + fn test_packet() -> Packet { + let mut packet = build_test_ipv4_packet_with_transport(64, Some(NextHeader::UDP)).unwrap(); + let destination = IpAddr::from_str("192.168.1.1").expect("Bad dst ip address"); + packet.set_ip_destination(destination).unwrap(); + packet + } + fn mutate_packet(rng: &mut ThreadRng, packet: &mut Packet) { + packet + .set_udp_destination_port(UdpPort::new_checked(rng.random_range(20..=100)).unwrap()) + .unwrap(); + } + + // Test the concurrency of a SINGLE fib. NUM_WORKERS workers perform LPM lookups on a single FIB for + // a test packet while another thread fuzzes the route to forward the packet, by removing the route + // or aggressively changing the fibgroup (and fib entries) used for the prefix of that route. + #[test] + fn test_concurrency_fib() { + const NUM_PACKETS: u64 = 1000_00; + const NUM_WORKERS: u16 = 4; + + // sync main thread - worker thread(s) + let done = Arc::new(AtomicU16::new(0)); + + // create fib with writer and readers + let (mut fibw, fibr) = FibWriter::new(FibKey::Id(0)); + + // the prefix of the route that will be used to process a packet + let prefix = Prefix::from("192.168.1.0/24"); + + // the next-hop - we keep this throughout + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + + // spawn a reader thread (worker) + let rfactory = fibr.factory(); + let worker_done = done.clone(); + + // build shared database and randomizer + let randomrouter = Arc::new(RandomRouter::load()); + + /*************************/ + /* worker thread closure */ + /*************************/ + let worker_code = Arc::new(move |randomrouter: Arc| { + let fibreader = rfactory.handle(); + let mut prefix_hits = 0u64; // number of times lpm yielded prefix 192.168.1.0/24 + let mut drop_hits = 0u64; // number of times lpm yielded 0.0.0.0/0 - Drop + let mut stats: HashMap = HashMap::new(); + let mut rng = rand::rng(); + + // The packet each worker will repeatedly try to forward using the fib. + // The worker mutates the packet (udp dst port) to alter the hash outcome + // so that distinct fib entries of the route hit get a chance of being selected. + let mut packet = test_packet(); + + loop { + if let Some(fib) = fibreader.enter() { + mutate_packet(&mut rng, &mut packet); + + // do LPM + let (hit, entry) = fib.lpm_entry_prefix(&packet); + if hit != prefix { + assert_eq!(hit, Prefix::root_v4()); // fuzzer can remove route + drop_hits += 1; + continue; + } + assert!(randomrouter.entries.contains(entry)); + prefix_hits += 1; + + // get interface from fib entry. This is to keep some stats to see + // if the main thread really had a chance to fuzz the fib once the test ends. + let ifindex = get_entry_interface_index(entry); + stats + .entry(ifindex) + .and_modify(|counter| *counter += 1) + .or_insert(0); + + // stop after processing NUM_PACKETS with the target route + if prefix_hits >= NUM_PACKETS { + println!(" -- Worker --"); + println!("target hits: {prefix_hits} pkts"); + println!("drop hits : {drop_hits} pkts"); + println!("Stats: {stats:#?}"); + worker_done.fetch_add(1, Ordering::Relaxed); + break; + } + } + } + }); + + /* Spawn workers */ + for n in 1..=NUM_WORKERS { + let rand_router = randomrouter.clone(); + let value = worker_code.clone(); + let _ = Builder::new() + .name(format!("WORKER-{n}")) + .spawn(move || value(rand_router.clone())) + .unwrap(); + } + + /********************************************************/ + /* main thread: loops fuzzing / deleting a single route */ + /********************************************************/ + let mut updates = 0u64; + let mut route_adds = 0u64; + let mut route_replaces = 0u64; + let mut route_deletions = 0u64; + let mut rng = rand::rng(); + loop { + // randomly select a fib-group, register it and add the route to the + // target prefix. Fuzzer changes the fibgroup on every loop. + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + fibw.register_fibgroup(&nhkey, fibgroup, true); + if route_adds == 0 { + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + route_adds += 1; + } + + // every 10 loops replace route and fibgroup + if updates % 10 == 0 { + //fibw.unregister_fibgroup(&nhkey.clone(), false); + fibw.register_fibgroup(&nhkey, fibgroup, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + route_replaces += 1; + fibw.publish(); + } + if updates % 101 == 0 { + fibw.del_fibroute(prefix); + fibw.publish(); + route_deletions += 1; + } + + // iterations + updates += 1; + + // stop when all worker are done + if done.load(Ordering::Relaxed) == NUM_WORKERS { + println!("All workers finished!"); + break; + } + } + println!(" --- Fib fuzzer stats ---"); + println!("fibgroup updates:{updates}"); + println!("route adds: {route_adds}"); + println!("route replaces: {route_replaces}"); + println!("route deletions: {route_deletions}"); + } + + // Tests fib reader utilities returning guards + #[test] + fn test_fib_guards() { + // create fib + let (mut fibw, fibr) = FibWriter::new(FibKey::Id(0)); + + // add a route + let prefix = Prefix::from("192.168.1.0/24"); + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + let e1 = build_fib_entry_egress(1, "10.0.1.1", "eth1"); + let fibgroup1 = build_fibgroup(&[e1.clone()]); + fibw.register_fibgroup(&nhkey, &fibgroup1, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + fibw.publish(); + + // use the fib: do lpm for some destination and get a route + let destination = IpAddr::from_str("192.168.1.1").expect("Bad dst ip address"); + let route = &*fibr.lpm_route(destination).unwrap(); + assert!(route.has_entries()); + assert_eq!(route.len(), fibgroup1.len()); + assert!(route.iter().any(|g| g == &fibgroup1)); + + // use again the fib with a packet + let packet = test_packet(); + let (matched, entry1) = fibr.lpm_entry_prefix(&packet).unwrap(); + assert_eq!(matched, prefix); + assert!(fibgroup1.entries().contains(&*entry1)); + + // attempt to modify the route by modifying the fibgroup + let e2 = build_fib_entry_egress(2, "10.0.2.1", "eth2"); + let fibgroup2 = build_fibgroup(&[e2.clone()]); + fibw.register_fibgroup(&nhkey, &fibgroup2, false); + fibw.add_fibroute(prefix, vec![nhkey.clone()], false); + fibw.publish(); + + // a second query to the fib yields the updated value. This is counter-intuitive but correct, + // because of the number of readers we have and the fact that the original fib guard was forgotten. + let (_, entry2) = fibr.lpm_entry_prefix(&packet).unwrap(); + assert_eq!(&*entry2, &e2); + + // ... but the guard to the entry wasn't. + assert_eq!(&*entry1, &e1); + + // Additional queries while holding the guards would cause the writer to block. + // We can't test this here since there's a single thread and it would block forever. + } +} From f3b56b024eecbc2bf36619a0c73f15ae0a4d497b Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Sun, 19 Oct 2025 19:04:41 +0200 Subject: [PATCH 34/35] feat(routing): speculative fix for fib deletion While a fib is being deleted, it may be actively looked up to forward packets from dataplane workers. Under heavy load, misaligned memory accesses have been observed when a fib is removed. This patch avoids those by "invalidating" a fib prior to its deletion and requiring readers to check the validity of the fib before attempting any lookup. Signed-off-by: Fredi Raspall --- routing/src/fib/fibtype.rs | 38 +++++++++++++++++++++++++++++-------- routing/src/rib/vrftable.rs | 10 +++++++--- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/routing/src/fib/fibtype.rs b/routing/src/fib/fibtype.rs index cd4fb4d07..cc8c8f608 100644 --- a/routing/src/fib/fibtype.rs +++ b/routing/src/fib/fibtype.rs @@ -7,9 +7,9 @@ use left_right::{Absorb, ReadGuard, ReadHandle, ReadHandleFactory, WriteHandle}; use left_right_tlcache::Identity; -use std::hash::Hash; use std::net::IpAddr; use std::rc::Rc; +use std::{hash::Hash, sync::atomic::AtomicBool}; use lpm::prefix::{Ipv4Prefix, Ipv6Prefix, Prefix}; use lpm::trie::{PrefixMapTrie, TrieMap, TrieMapFactory}; @@ -59,6 +59,7 @@ pub struct Fib { routesv6: PrefixMapTrie, groupstore: FibGroupStore, vtep: Vtep, + valid: AtomicBool, } impl Hash for Fib { // We implement explicitly `std::hash::Hash` for `Fib` instead of deriving it because: @@ -82,6 +83,7 @@ impl Default for Fib { routesv6: PrefixMapTrie::create(), groupstore: FibGroupStore::new(), vtep: Vtep::new(), + valid: AtomicBool::new(true), }; // default route let route = FibRoute::with_fibgroup(fib.groupstore.get_drop_fibgroup_ref()); @@ -292,6 +294,7 @@ enum FibChange { AddFibRoute((Prefix, Vec)), DelFibRoute(Prefix), SetVtep(Vtep), + Invalidate, } impl Absorb for Fib { @@ -306,6 +309,9 @@ impl Absorb for Fib { FibChange::AddFibRoute((prefix, keys)) => self.build_add_fibroute(*prefix, keys), FibChange::DelFibRoute(prefix) => self.del_fibroute(*prefix), FibChange::SetVtep(vtep) => self.set_vtep(vtep), + FibChange::Invalidate => { + self.valid.store(false, std::sync::atomic::Ordering::SeqCst); + } } } fn sync_with(&mut self, first: &Self) { @@ -337,10 +343,6 @@ impl FibWriter { pub fn enter(&self) -> Option> { self.0.enter() } - #[must_use] - pub fn get_id(&self) -> Option { - self.0.enter().map(|fib| fib.get_id()) - } pub fn register_fibgroup(&mut self, key: &NhopKey, fibgroup: &FibGroup, publish: bool) { self.0 .append(FibChange::RegisterFibGroup((key.clone(), fibgroup.clone()))); @@ -383,6 +385,12 @@ impl FibWriter { pub fn as_fibreader(&self) -> FibReader { FibReader::new(self.0.clone()) } + pub fn destroy(mut self) { + self.0.append(FibChange::Invalidate); + self.publish(); + // this is sanity and should not be needed + while self.as_fibreader().enter().is_some() {} + } } #[derive(Clone, Debug)] @@ -393,8 +401,22 @@ impl FibReader { pub fn new(rhandle: ReadHandle) -> Self { FibReader(rhandle) } + #[must_use] + #[inline] + pub fn is_valid(&self) -> bool { + match self.0.enter() { + Some(fib) => fib.valid.load(std::sync::atomic::Ordering::Acquire), + None => false, + } + } pub fn enter(&self) -> Option> { - self.0.enter() + self.0.enter().map(|fib| { + if fib.valid.load(std::sync::atomic::Ordering::Acquire) { + Some(fib) + } else { + None + } + })? } #[inline(always)] /// Convert Rc> -> FibReader @@ -408,7 +430,7 @@ impl FibReader { } } pub fn get_id(&self) -> Option { - self.0.enter().map(|fib| fib.get_id()) + self.enter().map(|fib| fib.get_id()) } #[must_use] pub fn factory(&self) -> FibReaderFactory { @@ -451,7 +473,7 @@ impl FibReader { packet: &Packet, ) -> Option<(Prefix, ReadGuard<'_, FibEntry>)> { let mut prefix = Prefix::root_v4(); - let guarded_entry = self.0.enter().map(|guard| { + let guarded_entry = self.enter().map(|guard| { ReadGuard::map(guard, |fib| { let (hit, entry) = fib.lpm_entry_prefix(packet); prefix = hit; diff --git a/routing/src/rib/vrftable.rs b/routing/src/rib/vrftable.rs index b99627ed2..66ef38675 100644 --- a/routing/src/rib/vrftable.rs +++ b/routing/src/rib/vrftable.rs @@ -181,15 +181,19 @@ impl VrfTable { ) -> Result<(), RouterError> { // remove the vrf from the vrf table debug!("Removing VRF {vrfid}..."); - let Some(vrf) = self.by_id.remove(&vrfid) else { + let Some(mut vrf) = self.by_id.remove(&vrfid) else { error!("No vrf with id {vrfid} exists"); return Err(RouterError::NoSuchVrf); }; + + // detach interfaces + iftablew.detach_interfaces_from_vrf(vrfid); + // delete the corresponding fib - if vrf.fibw.is_some() { + if let Some(fibw) = vrf.fibw.take() { debug!("Deleting Fib for vrf {vrfid} from the FibTable"); self.fibtablew.del_fib(vrfid, vrf.vni); - iftablew.detach_interfaces_from_vrf(vrfid); + fibw.destroy(); } // if the VRF had a vni assigned, unregister it From 48d3ad6a12620dce2ff8a70b9968f84f1a7c1a97 Mon Sep 17 00:00:00 2001 From: Fredi Raspall Date: Sat, 18 Oct 2025 20:44:39 +0200 Subject: [PATCH 35/35] feat(routing): add fibtable concurrency test Also add crate concurrency as dependency for later use. Note that no test is currently using shuttle/loom at the moment. Signed-off-by: Fredi Raspall --- Cargo.lock | 1 + routing/Cargo.toml | 1 + routing/src/fib/test.rs | 150 +++++++++++++++++++++++++++++++++++++--- 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4571cd463..cbf119931 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1279,6 +1279,7 @@ dependencies = [ "bytes", "chrono", "dataplane-cli", + "dataplane-concurrency", "dataplane-config", "dataplane-left-right-tlcache", "dataplane-lpm", diff --git a/routing/Cargo.toml b/routing/Cargo.toml index 113fbd971..d3d146995 100644 --- a/routing/Cargo.toml +++ b/routing/Cargo.toml @@ -40,6 +40,7 @@ netdev = { workspace = true } [dev-dependencies] bolero = { workspace = true, default-features = false } +concurrency = { workspace = true } lpm = { workspace = true, features = ["testing"] } rand = { workspace = true, default-features = false, features = ["thread_rng"] } tracing-test = { workspace = true, features = [] } diff --git a/routing/src/fib/test.rs b/routing/src/fib/test.rs index a1497e2bd..6803e3af4 100644 --- a/routing/src/fib/test.rs +++ b/routing/src/fib/test.rs @@ -3,29 +3,35 @@ //! Fib module tests -#[cfg(test)] -pub mod tests { +#![cfg(test)] +use concurrency::concurrency_mode; + +#[concurrency_mode(std)] +mod tests { use crate::fib::fibobjects::FibEntry; use crate::fib::fibobjects::FibGroup; use crate::fib::fibobjects::PktInstruction; + use crate::fib::fibtable::FibTableWriter; + use crate::fib::fibtype::FibKey; + use crate::fib::fibtype::FibWriter; + use crate::rib::nexthop::NhopKey; + use net::ip::NextHeader; use net::packet::Packet; use net::packet::test_utils::build_test_ipv4_packet_with_transport; use net::udp::UdpPort; use net::{buffer::TestBuffer, interface::InterfaceIndex}; - use crate::fib::fibtype::FibKey; - use crate::fib::fibtype::FibWriter; - use crate::rib::nexthop::NhopKey; - use lpm::prefix::IpAddr; - use lpm::prefix::Prefix; + use lpm::prefix::{IpAddr, Prefix}; use rand::Rng; use rand::rngs::ThreadRng; use std::str::FromStr; use std::sync::Arc; use std::sync::atomic::AtomicU16; + use std::thread; use std::thread::Builder; + use std::time::{Duration, Instant}; use std::{collections::HashMap, collections::HashSet, sync::atomic::Ordering}; use crate::fib::fibgroupstore::tests::build_fib_entry_egress; @@ -102,6 +108,7 @@ pub mod tests { // Test the concurrency of a SINGLE fib. NUM_WORKERS workers perform LPM lookups on a single FIB for // a test packet while another thread fuzzes the route to forward the packet, by removing the route // or aggressively changing the fibgroup (and fib entries) used for the prefix of that route. + #[test] fn test_concurrency_fib() { const NUM_PACKETS: u64 = 1000_00; @@ -206,7 +213,6 @@ pub mod tests { // every 10 loops replace route and fibgroup if updates % 10 == 0 { - //fibw.unregister_fibgroup(&nhkey.clone(), false); fibw.register_fibgroup(&nhkey, fibgroup, false); fibw.add_fibroute(prefix, vec![nhkey.clone()], false); route_replaces += 1; @@ -221,7 +227,7 @@ pub mod tests { // iterations updates += 1; - // stop when all worker are done + // stop when all workers are done if done.load(Ordering::Relaxed) == NUM_WORKERS { println!("All workers finished!"); break; @@ -234,6 +240,132 @@ pub mod tests { println!("route deletions: {route_deletions}"); } + // Test the concurrency of a SINGLE fib within a FIBTABLE. NUM_WORKERS workers perform LPM lookups + // on a single FIB for a test packet while another thread fuzzes the route to forward the packet, by removing the route + // or aggressively changing the fibgroup (and fib entries) used for the prefix of that route. The fuzzer in this + // test also removes the FIB and adds it again. The workers use a thread-local cache to access the FIB. + #[test] + fn test_concurrency_fibtable() { + // number of threads looking up fibtable + const NUM_WORKERS: u16 = 7; + const NUM_PACKETS: u64 = 1_000_000; + const TENTH: u64 = NUM_PACKETS / 10; + + // create fibtable (empty, without any fib) + let (mut fibtw, fibtr) = FibTableWriter::new(); + let fibtrfactory = fibtr.factory(); + + // prefix to be hit by packets + let prefix = Prefix::from("192.168.1.0/24"); + + // shared counter of workers that finished + let done = Arc::new(AtomicU16::new(0)); + + let vrfid = 1; + + /* Spawn workers: each has its own reader for the fibtable */ + for n in 1..=NUM_WORKERS { + let fibtr = fibtrfactory.handle(); + let worker_done = done.clone(); + + Builder::new() + .name(format!("WORKER-{n}")) + .spawn(move || { + let mut rng = rand::rng(); + let mut packet = test_packet(); + let mut prefix_hits: u64 = 0; + let mut other_hits: u64 = 0; + let mut nofibs: u64 = 0; + let mut nofib_enter: u64 = 0; + loop { + mutate_packet(&mut rng, &mut packet); + if let Ok(fib) = fibtr.get_fib_reader(FibKey::Id(vrfid)) { + if let Some(fib) = fib.enter() { + let (hit, _fibentry) = fib.lpm_entry_prefix(&packet); + if hit == prefix { + prefix_hits += 1; + if prefix_hits % TENTH == 0 { + println!("Worker {n} is {} % done", prefix_hits * 100 / NUM_PACKETS); + } + + if prefix_hits >= NUM_PACKETS { + println!("=== Worker {n} finished ===="); + println!("Stats:"); + println!(" {prefix_hits:>8} packets hit {prefix}"); + println!(" {other_hits:>8} packets hit other prefix (0.0.0.0/0)"); + println!(" {nofibs:>8} packets found no fib"); + println!(" {nofib_enter:>8} packets found fib but could not enter"); + worker_done.fetch_add(1, Ordering::Relaxed); + break; + } + } else { + other_hits += 1; + } + } else { + nofib_enter += 1; + } + } else { + nofibs += 1; + } + } + }) + .unwrap(); + } + + /*****************************************************************/ + /* main thread (fuzzer): adds / deletes route / fibgroup and fib */ + /*****************************************************************/ + let nhkey = NhopKey::with_address(&IpAddr::from_str("7.0.0.1").unwrap()); + let mut rng = rand::rng(); + let randomrouter = RandomRouter::load(); + let mut updates = 0u64; + + let mut fibw = Some(fibtw.add_fib(vrfid, None)); + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + if let Some(fibw) = &mut fibw { + fibw.register_fibgroup(&nhkey, fibgroup, true); + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + } + let start = Instant::now(); + loop { + if fibw.is_none() { + fibw = Some(fibtw.add_fib(vrfid, None)); + } + if let Some(fibw) = &mut fibw { + if updates % 100 == 0 { + let fibgroup = randomrouter.random_pick_fibgroup(&mut rng); + fibw.register_fibgroup(&nhkey, fibgroup, true); + fibw.add_fibroute(prefix, vec![nhkey.clone()], true); + } + if updates % 150 == 0 { + fibw.del_fibroute(prefix); + fibw.publish(); + } + } + + if updates % 50 == 0 && fibw.is_some() { + fibtw.del_fib(1, None); + thread::sleep(Duration::from_millis(15)); + if true { + // fib gets deleted here + let fib = fibw.take(); + fib.unwrap().destroy(); + } + } + + // iterations + updates += 1; + + // stop when all workers are done + if done.load(Ordering::Relaxed) == NUM_WORKERS { + println!("All workers finished!"); + break; + } + } + let duration = start.elapsed(); + println!("Test duration: {:?}", duration); + } + // Tests fib reader utilities returning guards #[test] fn test_fib_guards() {