From 6dcd27f034fd326874328a8dbcef1c0e9502ac86 Mon Sep 17 00:00:00 2001 From: Lee Smet Date: Fri, 21 Mar 2025 14:24:33 +0100 Subject: [PATCH] Add queried and not found routes to route table Signed-off-by: Lee Smet --- mycelium/src/router.rs | 45 ++--- mycelium/src/routing_table.rs | 219 +++++++++++++-------- mycelium/src/routing_table/iter.rs | 27 +-- mycelium/src/routing_table/iter_mut.rs | 90 ++++----- mycelium/src/routing_table/subnet_entry.rs | 16 ++ 5 files changed, 229 insertions(+), 168 deletions(-) create mode 100644 mycelium/src/routing_table/subnet_entry.rs diff --git a/mycelium/src/router.rs b/mycelium/src/router.rs index 6c51bed..41e3873 100644 --- a/mycelium/src/router.rs +++ b/mycelium/src/router.rs @@ -7,7 +7,7 @@ use crate::{ packet::{ControlPacket, DataPacket}, peer::Peer, router_id::RouterId, - routing_table::{RouteEntry, RouteKey, RouteList, RoutingTable}, + routing_table::{RouteEntry, RouteKey, RouteList, Routes, RoutingTable}, seqno_cache::{SeqnoCache, SeqnoRequestCacheKey}, sequence_number::SeqNo, source_table::{FeasibilityDistance, SourceKey, SourceTable}, @@ -227,19 +227,24 @@ where /// Get the [`PublicKey`] for an [`IpAddr`] if a route exists to the IP. pub fn get_pubkey(&self, ip: IpAddr) -> Option { - self.routing_table - .best_routes(ip) - .and_then(|rl| if rl.is_empty() { None } else { Some(rl) }) - .map(|rl| rl[0].source().router_id().to_pubkey()) + if let Routes::Exist(routes) = self.routing_table.best_routes(ip) { + if routes.is_empty() { + None + } else { + Some(routes[0].source().router_id().to_pubkey()) + } + } else { + None + } } /// Gets the cached [`SharedSecret`] for the remote. pub fn get_shared_secret_from_dest(&self, dest: IpAddr) -> Option { - self.routing_table - .best_routes(dest) - // We can index here safely since we always have at least 1 route if we get - // Option::Some. - .map(|rl| rl.shared_secret().clone()) + if let Routes::Exist(routes) = self.routing_table.best_routes(dest) { + Some(routes.shared_secret().clone()) + } else { + None + } } /// Gets the cached [`SharedSecret`] based on the associated [`PublicKey`] of the remote. @@ -774,12 +779,7 @@ where .router_process_route_request(route_request.prefix().is_none()); // Handle the case of a single subnet. if let Some(subnet) = route_request.prefix() { - let update = if let Some(sre) = self - .routing_table - .routes(subnet) - .as_ref() - .and_then(|rl| rl.selected()) - { + let update = if let Some(sre) = self.routing_table.routes(subnet).selected() { trace!("Advertising selected route for {subnet} after route request"); // Optimization: Don't send an update if the selected route next-hop is the peer // who requested the route, as per the babel protocol the update will never be @@ -859,12 +859,7 @@ where // requested router id, or the router id is the same and the entries sequence number is // not smaller than the requested sequence number, send an update for the route // to the peer (triggered update). - if let Some(route_entry) = self - .routing_table - .routes(seqno_request.prefix()) - .as_ref() - .and_then(|rl| rl.selected()) - { + if let Some(route_entry) = self.routing_table.routes(seqno_request.prefix()).selected() { if !route_entry.metric().is_infinite() && (seqno_request.router_id() != route_entry.source().router_id() || !route_entry.seqno().lt(&seqno_request.seqno())) @@ -968,7 +963,9 @@ where } } - if let Some(possible_routes) = self.routing_table.routes(seqno_request.prefix()) { + if let Routes::Exist(possible_routes) = + self.routing_table.routes(seqno_request.prefix()) + { { let source_table = self.source_table.read().unwrap(); // First only consider feasible routes. @@ -1390,7 +1387,7 @@ where .iter() .cloned() .collect() - } else if let Some(known_routes) = known_routes { + } else if let Routes::Exist(known_routes) = known_routes { known_routes .iter() .filter_map(|re| { diff --git a/mycelium/src/routing_table.rs b/mycelium/src/routing_table.rs index fcba1d8..a8b50d9 100644 --- a/mycelium/src/routing_table.rs +++ b/mycelium/src/routing_table.rs @@ -4,8 +4,8 @@ use std::{ sync::{Arc, Mutex, MutexGuard}, }; -use arc_swap::ArcSwap; use ip_network_table_deps_treebitmap::IpLookupTable; +use subnet_entry::SubnetEntry; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; use tracing::{error, trace}; @@ -23,6 +23,51 @@ mod iter_mut; mod route_entry; mod route_key; mod route_list; +mod subnet_entry; + +pub enum Routes { + Exist(RouteListReadGuard), + Queried, + NoRoute, + None, +} + +impl Routes { + /// Returns the selected route if one exists. + pub fn selected(&self) -> Option<&RouteEntry> { + if let Routes::Exist(routes) = self { + routes.selected() + } else { + None + } + } + + /// Returns true if there are no routes + pub fn is_none(&self) -> bool { + !matches!(self, Routes::Exist { .. }) + } +} + +impl From<&SubnetEntry> for Routes { + fn from(value: &SubnetEntry) -> Self { + match value { + SubnetEntry::Exists { list } => { + Routes::Exist(RouteListReadGuard { inner: list.load() }) + } + SubnetEntry::Queried { .. } => Routes::Queried, + SubnetEntry::NoRoute { .. } => Routes::NoRoute, + } + } +} + +impl From> for Routes { + fn from(value: Option<&SubnetEntry>) -> Self { + match value { + Some(v) => v.into(), + None => Routes::None, + } + } +} /// The routing table holds a list of route entries for every known subnet. #[derive(Clone)] @@ -36,7 +81,7 @@ pub struct RoutingTable { #[derive(Default)] struct RoutingTableInner { - table: IpLookupTable>>, + table: IpLookupTable>, } /// Hold an exclusive write lock over the routing table. While this item is in scope, no other @@ -46,7 +91,7 @@ pub struct WriteGuard<'a> { routing_table: &'a RoutingTable, /// Owned copy of the RouteList, this is populated once mutable access the the RouteList has /// been requested. - value: Arc>, + value: Arc, /// Did the RouteList exist initially? exists: bool, /// The subnet we are writing to. @@ -79,7 +124,7 @@ impl RoutingTable { /// Get a list of the routes for the most precises [`Subnet`] known which contains the given /// [`IpAddr`]. - pub fn best_routes(&self, ip: IpAddr) -> Option { + pub fn best_routes(&self, ip: IpAddr) -> Routes { let IpAddr::V6(ip) = ip else { panic!("Only IPv6 is supported currently"); }; @@ -88,17 +133,18 @@ impl RoutingTable { .expect("Write handle is saved on the router so it is not dropped yet.") .table .longest_match(ip) - .map(|(_, _, rl)| RouteListReadGuard { inner: rl.load() }) + .map(|(_, _, rl)| rl.as_ref()) + .into() } /// Get a list of all routes for the given subnet. Changes to the RoutingTable after this /// method returns will not be visible and require this method to be called again to be /// observed. - pub fn routes(&self, subnet: Subnet) -> Option { + pub fn routes(&self, subnet: Subnet) -> Routes { let subnet_ip = if let IpAddr::V6(ip) = subnet.address() { ip } else { - return None; + return Routes::None; }; self.reader @@ -106,7 +152,8 @@ impl RoutingTable { .expect("Write handle is saved on the router so it is not dropped yet.") .table .exact_match(subnet_ip, subnet.prefix_len().into()) - .map(|rl| RouteListReadGuard { inner: rl.load() }) + .map(Arc::as_ref) + .into() } /// Gets continued read access to the `RoutingTable`. While the returned @@ -172,7 +219,9 @@ impl RoutingTable { panic!("IP v4 addresses are not supported") }; - let value = Arc::new(Arc::new(RouteList::new(shared_secret)).into()); + let value = Arc::new(SubnetEntry::Exists { + list: Arc::new(RouteList::new(shared_secret)).into(), + }); WriteGuard { routing_table: self, @@ -199,7 +248,10 @@ impl RoutingTable { .table .longest_match(ip) .and_then(|(_, _, rl)| { - let rl = rl.load(); + let SubnetEntry::Exists { list } = &**rl else { + return None; + }; + let rl = list.load(); if rl.is_empty() || !rl[0].selected() { None } else { @@ -263,9 +315,10 @@ impl WriteGuard<'_> { /// Loads the current [`RouteList`]. #[inline] pub fn routes(&self) -> RouteListReadGuard { - RouteListReadGuard { - inner: self.value.load(), - } + let SubnetEntry::Exists { list } = &*self.value else { + panic!("Write guard for non-route SubnetEntry") + }; + RouteListReadGuard { inner: list.load() } } /// Get mutable access to the [`RouteList`]. This will update the [`RouteList`] in place @@ -279,96 +332,106 @@ impl WriteGuard<'_> { ) -> bool { let mut res = false; let mut delete = false; - self.value.rcu(|rl| { - let mut new_val = rl.clone(); - let v = Arc::make_mut(&mut new_val); + if let SubnetEntry::Exists { list } = &*self.value { + list.rcu(|rl| { + let mut new_val = rl.clone(); + let v = Arc::make_mut(&mut new_val); - res = op(v, &self.expired_route_entry_sink, &self.cancellation_token); - delete = v.is_empty(); + res = op(v, &self.expired_route_entry_sink, &self.cancellation_token); + delete = v.is_empty(); - new_val - }); + new_val + }); - if delete && self.exists { - trace!(subnet = %self.subnet, "Deleting subnet which became empty after updating"); - let mut writer = self.routing_table.writer.lock().unwrap(); + if delete && self.exists { + trace!(subnet = %self.subnet, "Deleting subnet which became empty after updating"); + let mut writer = self.routing_table.writer.lock().unwrap(); - writer.append(RoutingTableOplogEntry::Delete(self.subnet)); - writer.publish(); + writer.append(RoutingTableOplogEntry::Delete(self.subnet)); + writer.publish(); + } + + res + } else { + false } - - res } /// Set the [`RouteEntry`] with the given [`neighbour`](Peer) as the selected route. pub fn set_selected(&mut self, neighbour: &Peer) { - self.value.rcu(|routes| { - let mut new_routes = routes.clone(); - let routes = Arc::make_mut(&mut new_routes); - let Some(pos) = routes.iter().position(|re| re.neighbour() == neighbour) else { - error!( - neighbour = neighbour.connection_identifier(), - "Failed to select route entry with given route key, no such entry" - ); - return new_routes; - }; + if let SubnetEntry::Exists { list } = &*self.value { + list.rcu(|routes| { + let mut new_routes = routes.clone(); + let routes = Arc::make_mut(&mut new_routes); + let Some(pos) = routes.iter().position(|re| re.neighbour() == neighbour) else { + error!( + neighbour = neighbour.connection_identifier(), + "Failed to select route entry with given route key, no such entry" + ); + return new_routes; + }; - // We don't need a check for an empty list here, since we found a selected route there - // _MUST_ be at least 1 entry. - // Set the first element to unselected, then select the proper element so this also works - // in case the existing route is "reselected". - routes[0].set_selected(false); - routes[pos].set_selected(true); - routes.swap(0, pos); + // We don't need a check for an empty list here, since we found a selected route there + // _MUST_ be at least 1 entry. + // Set the first element to unselected, then select the proper element so this also works + // in case the existing route is "reselected". + routes[0].set_selected(false); + routes[pos].set_selected(true); + routes.swap(0, pos); - new_routes - }); + new_routes + }); + } } /// Unconditionally unselects the selected route, if one is present. /// /// In case no route is selected, this is a no-op. pub fn unselect(&mut self) { - self.value.rcu(|v| { - let mut new_val = v.clone(); - let new_ref = Arc::make_mut(&mut new_val); + if let SubnetEntry::Exists { list } = &*self.value { + list.rcu(|v| { + let mut new_val = v.clone(); + let new_ref = Arc::make_mut(&mut new_val); - if let Some(e) = new_ref.get_mut(0) { - e.set_selected(false); - } + if let Some(e) = new_ref.get_mut(0) { + e.set_selected(false); + } - new_val - }); + new_val + }); + } } } impl Drop for WriteGuard<'_> { fn drop(&mut self) { // FIXME: try to get rid of clones on the Arc here - let value = self.value.load(); - match self.exists { - // The route list did not exist, and now it is not empty, so an entry was added. We - // need to add the route list to the routing table. - false if !value.is_empty() => { - trace!(subnet = %self.subnet, "Inserting new route list for subnet"); - let mut writer = self.routing_table.writer.lock().unwrap(); - writer.append(RoutingTableOplogEntry::Upsert( - self.subnet, - Arc::clone(&self.value), - )); - writer.publish(); + if let SubnetEntry::Exists { list } = &*self.value { + let value = list.load(); + match self.exists { + // The route list did not exist, and now it is not empty, so an entry was added. We + // need to add the route list to the routing table. + false if !value.is_empty() => { + trace!(subnet = %self.subnet, "Inserting new route list for subnet"); + let mut writer = self.routing_table.writer.lock().unwrap(); + writer.append(RoutingTableOplogEntry::Upsert( + self.subnet, + Arc::clone(&self.value), + )); + writer.publish(); + } + // There was an existing route list which is now empty, so the entry for this subnet + // needs to be deleted in the routing table. + true if value.is_empty() => { + trace!(subnet = %self.subnet, "Removing route list for subnet"); + let mut writer = self.routing_table.writer.lock().unwrap(); + writer.append(RoutingTableOplogEntry::Delete(self.subnet)); + writer.publish(); + } + // Nothing to do in these cases. Either no value was inserted in a non existing + // routelist, or an existing one was updated in place. + _ => {} } - // There was an existing route list which is now empty, so the entry for this subnet - // needs to be deleted in the routing table. - true if value.is_empty() => { - trace!(subnet = %self.subnet, "Removing route list for subnet"); - let mut writer = self.routing_table.writer.lock().unwrap(); - writer.append(RoutingTableOplogEntry::Delete(self.subnet)); - writer.publish(); - } - // Nothing to do in these cases. Either no value was inserted in a non existing - // routelist, or an existing one was updated in place. - _ => {} } } } @@ -376,7 +439,7 @@ impl Drop for WriteGuard<'_> { /// Operations allowed on the left_right for the routing table. enum RoutingTableOplogEntry { /// Insert or Update the value for the given subnet. - Upsert(Subnet, Arc>), + Upsert(Subnet, Arc), /// Delete the entry for the given subnet. Delete(Subnet), } diff --git a/mycelium/src/routing_table/iter.rs b/mycelium/src/routing_table/iter.rs index e05aefe..c48fb01 100644 --- a/mycelium/src/routing_table/iter.rs +++ b/mycelium/src/routing_table/iter.rs @@ -1,21 +1,19 @@ use std::{net::Ipv6Addr, sync::Arc}; -use arc_swap::ArcSwap; +use crate::subnet::Subnet; -use crate::{routing_table::RouteList, subnet::Subnet}; - -use super::RouteListReadGuard; +use super::{subnet_entry::SubnetEntry, RouteListReadGuard}; /// An iterator over a [`routing table`](super::RoutingTable) giving read only access to /// [`RouteList`]'s. pub struct RoutingTableIter<'a>( - ip_network_table_deps_treebitmap::Iter<'a, Ipv6Addr, Arc>>, + ip_network_table_deps_treebitmap::Iter<'a, Ipv6Addr, Arc>, ); impl<'a> RoutingTableIter<'a> { /// Create a new `RoutingTableIter` which will iterate over all entries in a [`RoutingTable`]. pub(super) fn new( - inner: ip_network_table_deps_treebitmap::Iter<'a, Ipv6Addr, Arc>>, + inner: ip_network_table_deps_treebitmap::Iter<'a, Ipv6Addr, Arc>, ) -> Self { Self(inner) } @@ -25,12 +23,15 @@ impl Iterator for RoutingTableIter<'_> { type Item = (Subnet, RouteListReadGuard); fn next(&mut self) -> Option { - self.0.next().map(|(ip, prefix_size, rl)| { - ( - Subnet::new(ip.into(), prefix_size as u8) - .expect("Routing table contains valid subnets"), - RouteListReadGuard { inner: rl.load() }, - ) - }) + for (ip, prefix_size, rl) in self.0.by_ref() { + if let SubnetEntry::Exists { list } = &**rl { + return Some(( + Subnet::new(ip.into(), prefix_size as u8) + .expect("Routing table contains valid subnets"), + RouteListReadGuard { inner: list.load() }, + )); + } + } + None } } diff --git a/mycelium/src/routing_table/iter_mut.rs b/mycelium/src/routing_table/iter_mut.rs index 5155309..839ba3b 100644 --- a/mycelium/src/routing_table/iter_mut.rs +++ b/mycelium/src/routing_table/iter_mut.rs @@ -1,11 +1,12 @@ -use arc_swap::ArcSwap; use tokio::sync::mpsc; use tokio_util::sync::CancellationToken; use tracing::trace; use crate::subnet::Subnet; -use super::{RouteKey, RouteList, RoutingTableInner, RoutingTableOplogEntry}; +use super::{ + subnet_entry::SubnetEntry, RouteKey, RouteList, RoutingTableInner, RoutingTableOplogEntry, +}; use std::{ net::Ipv6Addr, sync::{Arc, MutexGuard}, @@ -16,7 +17,7 @@ use std::{ pub struct RoutingTableIterMut<'a, 'b> { write_guard: &'b mut MutexGuard<'a, left_right::WriteHandle>, - iter: ip_network_table_deps_treebitmap::Iter<'b, Ipv6Addr, Arc>>, + iter: ip_network_table_deps_treebitmap::Iter<'b, Ipv6Addr, Arc>, expired_route_entry_sink: mpsc::Sender, cancel_token: CancellationToken, @@ -28,7 +29,7 @@ impl<'a, 'b> RoutingTableIterMut<'a, 'b> { 'a, left_right::WriteHandle, >, - iter: ip_network_table_deps_treebitmap::Iter<'b, Ipv6Addr, Arc>>, + iter: ip_network_table_deps_treebitmap::Iter<'b, Ipv6Addr, Arc>, expired_route_entry_sink: mpsc::Sender, cancel_token: CancellationToken, @@ -44,22 +45,24 @@ impl<'a, 'b> RoutingTableIterMut<'a, 'b> { /// Get the next item in this iterator. This is not implemented as the [`Iterator`] trait, /// since we hand out items which are lifetime bound to this struct. pub fn next<'c>(&'c mut self) -> Option<(Subnet, RoutingTableIterMutEntry<'a, 'c>)> { - self.iter.next().map(|(ip, prefix_size, rl)| { - let subnet = Subnet::new(ip.into(), prefix_size as u8) - .expect("Routing table contains valid subnets"); - ( - subnet, - RoutingTableIterMutEntry { - writer: self.write_guard, - store: Arc::clone(rl), - value: rl.load_full(), - owned: false, + for (ip, prefix_size, rl) in self.iter.by_ref() { + if matches!(&**rl, SubnetEntry::Exists { .. }) { + let subnet = Subnet::new(ip.into(), prefix_size as u8) + .expect("Routing table contains valid subnets"); + return Some(( subnet, - expired_route_entry_sink: self.expired_route_entry_sink.clone(), - cancellation_token: self.cancel_token.clone(), - }, - ) - }) + RoutingTableIterMutEntry { + writer: self.write_guard, + store: Arc::clone(rl), + subnet, + expired_route_entry_sink: self.expired_route_entry_sink.clone(), + cancellation_token: self.cancel_token.clone(), + }, + )); + }; + } + + None } } @@ -69,9 +72,7 @@ pub struct RoutingTableIterMutEntry<'a, 'b> { &'b mut MutexGuard<'a, left_right::WriteHandle>, /// Owned copy of the RouteList, this is populated once mutable access the the RouteList has /// been requested. - store: Arc>, - value: Arc, - owned: bool, + store: Arc, /// The subnet we are writing to. subnet: Subnet, expired_route_entry_sink: mpsc::Sender, @@ -79,45 +80,28 @@ pub struct RoutingTableIterMutEntry<'a, 'b> { } impl RoutingTableIterMutEntry<'_, '_> { + /// Updates the routes for this entry pub fn update_routes, &CancellationToken)>( &mut self, mut op: F, ) { let mut delete = false; - self.store.rcu(|rl| { - let mut new_val = rl.clone(); - let v = Arc::make_mut(&mut new_val); + if let SubnetEntry::Exists { list } = &*self.store { + list.rcu(|rl| { + let mut new_val = rl.clone(); + let v = Arc::make_mut(&mut new_val); - op(v, &self.expired_route_entry_sink, &self.cancellation_token); - delete = v.is_empty(); + op(v, &self.expired_route_entry_sink, &self.cancellation_token); + delete = v.is_empty(); - new_val - }); + new_val + }); - if delete { - trace!(subnet = %self.subnet, "Queue subnet for deletion since route list is now empty"); - self.writer - .append(RoutingTableOplogEntry::Delete(self.subnet)); - } - } -} - -impl Drop for RoutingTableIterMutEntry<'_, '_> { - fn drop(&mut self) { - // If owned is false, we never got a mutable reference, and the existing value is a - // reference to the already existing value in the route table. So we don't have to do - // anything here. - if !self.owned { - return; - } - - // FIXME: try to get rid of clones on the Arc here - // There was an existing route list which is now empty, so the entry for this subnet - // needs to be deleted in the routing table. - if self.value.is_empty() { - trace!(subnet = %self.subnet, "Removing route list for subnet"); - self.writer - .append(RoutingTableOplogEntry::Delete(self.subnet)); + if delete { + trace!(subnet = %self.subnet, "Queue subnet for deletion since route list is now empty"); + self.writer + .append(RoutingTableOplogEntry::Delete(self.subnet)); + } } } } diff --git a/mycelium/src/routing_table/subnet_entry.rs b/mycelium/src/routing_table/subnet_entry.rs new file mode 100644 index 0000000..4e8dd83 --- /dev/null +++ b/mycelium/src/routing_table/subnet_entry.rs @@ -0,0 +1,16 @@ +use arc_swap::ArcSwap; + +use super::RouteList; + +/// An entry for a [Subnet](crate::subnet::Subnet) in the routing table. +#[allow(dead_code)] +pub enum SubnetEntry { + /// Routes for the given subnet exist + Exists { list: ArcSwap }, + /// Routes are being queried from peers for the given subnet, but we haven't gotten a response + /// yet + Queried { query_start: tokio::time::Instant }, + /// We queried our peers for the subnet, but we didn't get a valid response in time, so there + /// is for sure no route to the subnet. + NoRoute { expiry: tokio::time::Instant }, +}