From 1262d9b1d7bc935a7e5493c936eb1eae5112cbdb Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Wed, 14 Oct 2026 01:51:06 +0000 Subject: [PATCH 1/9] [softnpu, mcast] wire up the softnpu backend for multicast table ops Previously, SoftNPU didn't interact with multicast. `mc_group_create` and `mc_port_add` returned a stubbed `OperationUnsupported`, and port remove/destroy were no-ops. With bitmap-based replication in p4rs ([#240](https://github.com/oxidecomputer/p4/pull/240)), SoftNPU can replicate off of port bitmaps carried in the replication table entry itself. On the Tofino, membership subscriptions live in the PRE and change independently of match-action state. In the SoftNPU work, membership is captured at write time, and a membership mutation means rewriting the affected replication entries (when triggered). Includes: - *asic/SoftNPU*: - Live replication entries are tracked per group and rewritten when members are added/removed. The replication tracking lock covers both the snapshot and table updates. - Source filter, mcast NAT (tagged and untagged), egress decap, and mcast src MAC tables now map onto their sidecar-lite tables respectively. - `egress.asic_id_to_port` maps ASIC port IDs to logical port numbers for indexing the decap bitmap (same as on Tofino). - `TableRemove` is sent before `TableAdd` because p4rs drops an add whose key already exists and keeps the older entry (action and params). - *dpd/mcast*: - `mcast::reset` deletes each known group's entries before the table clears. SoftNPU's `Table::clear` is still a stub. - `del_entries_where` skips the stale-entry table sweep when the backend cannot list entries (e.g., in SoftNPU). - softnpu-lib is pinned to `zl/multicast` @ softnpu#183, which pulls in p4#240. SoftNPU loads sidecar-lite#152 at runtime. --- Cargo.lock | 19 +- Cargo.toml | 3 +- asic/src/softnpu/mcast.rs | 477 ++++++++++++++ asic/src/softnpu/mod.rs | 126 +++- asic/src/softnpu/table.rs | 686 ++++++++++++++++---- dpd-client/tests/integration_tests/mcast.rs | 154 +++++ dpd/src/mcast/mod.rs | 12 + dpd/src/table/mcast/mod.rs | 21 +- 8 files changed, 1339 insertions(+), 159 deletions(-) create mode 100644 asic/src/softnpu/mcast.rs diff --git a/Cargo.lock b/Cargo.lock index 5d72be0c..5fa0d903 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,7 +188,7 @@ dependencies = [ "slog", "slog-async", "slog-term", - "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=main)", + "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=zl%2Fmulticast)", "strum 0.28.0", "thiserror 2.0.18", "tofino 0.1.0 (git+https://github.com/oxidecomputer/tofino?branch=main)", @@ -5002,6 +5002,17 @@ dependencies = [ "usdt 0.5.0", ] +[[package]] +name = "p4rs" +version = "0.1.0" +source = "git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast#3d7c18bd4d243f19bc1702f20b990dba09c35e95" +dependencies = [ + "bitvec", + "num", + "serde", + "usdt 0.5.0", +] + [[package]] name = "p9ds" version = "0.1.0" @@ -7167,9 +7178,9 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu?branch=main#7e015d167a914777bc21434d1c61f205f22993b1" +source = "git+https://github.com/oxidecomputer/softnpu?branch=zl%2Fmulticast#26615d82bdad8dd9928239b88896864f83aab326" dependencies = [ - "p4rs", + "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast)", "serde", "serde_json", "tokio", @@ -7180,7 +7191,7 @@ name = "softnpu" version = "0.2.0" source = "git+https://github.com/oxidecomputer/softnpu#7e015d167a914777bc21434d1c61f205f22993b1" dependencies = [ - "p4rs", + "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=main)", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 49d0b634..942cddb1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,8 @@ oxnet = { version = "0.1.6", default-features = false, features = ["schemars", " propolis = { git = "https://github.com/oxidecomputer/propolis" } smf = { git = "https://github.com/illumos/smf-rs" } slog-error-chain = { git = "https://github.com/oxidecomputer/slog-error-chain", branch = "main" } -softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "main"} +# TODO: set back to main once zl/multicast merges +softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "zl/multicast"} tofino = { git = "https://github.com/oxidecomputer/tofino", branch = "main" } transceiver-controller = { git = "https://github.com/oxidecomputer/transceiver-control", branch = "main" } diff --git a/asic/src/softnpu/mcast.rs b/asic/src/softnpu/mcast.rs new file mode 100644 index 00000000..b015d88b --- /dev/null +++ b/asic/src/softnpu/mcast.rs @@ -0,0 +1,477 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at https://mozilla.org/MPL/2.0/ +// +// Copyright 2026 Oxide Computer Company + +//! [Sidecar-lite] handles packet replication via port bitmaps in the P4 +//! pipeline. This module tracks group membership for the `AsicMulticastOps` +//! contract and caches the installed replication entries for replaying +//! membership updates into sidecar-lite. +//! +//! [Sidecar-lite]: https://github.com/oxidecomputer/sidecar-lite + +use std::collections::{BTreeMap, HashMap, HashSet, hash_map::Entry}; + +use aal::{AsicError, AsicResult}; + +/// The sidecar-lite replication table that holds underlay/external +/// port bitmaps for an underlay multicast group. +pub const MCAST_REPLICATION_V6_TABLE: &str = + "ingress.mcast.mcast_replication_v6"; + +/// The sidecar-lite action that writes the external and underlay port bitmaps +/// (and replication id) for a [`MCAST_REPLICATION_V6_TABLE`] entry. +pub const SET_PORT_BITMAP_ACTION: &str = "set_port_bitmap"; + +/// A replication table entry's group references, tracked for later +/// membership changes to recompute port bitmaps when necessary. +/// +/// On the production sidecar, the replication table holds a group identifier, +/// while the replication engine tracks live membership subscriptions, which means +/// that any port additions do not trigger table rewrites. +/// +/// The SoftNPU pipeline, however, is built around a generic port bitmap +/// structure, appended into the table entry at write time, forcing each +/// membership change to rewrite each entry that references the mutated +/// group. +#[derive(Clone, Copy, Debug)] +struct ReplEntryRef { + external_grp: u16, + underlay_grp: u16, + rid: u16, +} + +/// A replication entry refreshed against current membership, available to be +/// rewritten into the SoftNPU pipeline. +#[derive(Clone, Debug)] +pub struct RefreshedReplEntry { + /// Match keyset bytes identifying the table entry (the underlay group's + /// destination address). + pub keyset: Vec, + /// Ports in the external replication group, as a port bitmap. + pub external_bitmap: u128, + /// Ports in the underlay replication group, as a port bitmap. + pub underlay_bitmap: u128, + /// Replication ID, which keys the egress decap-ports table. + pub rid: u16, +} + +/// Multicast group membership and the replication entries derived from it. +/// +/// Unlike Tofino, SoftNPU has no PRE, leading us to track group membership +/// here and rewrite the mutated replication entries' port bitmap when a +/// group's membership change is triggered. +#[derive(Default)] +pub struct McGroupData { + groups: HashMap>, + /// Replication table entries keyed on their matched keyset, recorded at + /// table-write time so membership changes can refresh bitmaps. + repl_entries: BTreeMap, ReplEntryRef>, +} + +fn no_group(group_id: u16) -> AsicError { + AsicError::Missing(format!("no such multicast group: {group_id}")) +} + +impl McGroupData { + /// Get the list of multicast domains. + pub fn domains(&self) -> Vec { + self.groups.keys().copied().collect() + } + + /// Build a 128-bit port bitmap for a group. Bit N is set if port N + /// is a member. + /// + /// Returns zero for unknown groups. + pub fn port_bitmap(&self, group_id: u16) -> u128 { + self.groups.get(&group_id).map_or(0, |ports| { + ports.iter().fold(0, |bitmap, &port| bitmap | (1u128 << port)) + }) + } + + /// Get the number of ports in a multicast domain. + pub fn domain_port_count(&self, group_id: u16) -> AsicResult { + match self.groups.get(&group_id) { + Some(g) => Ok(g.len()), + None => Err(no_group(group_id)), + } + } + + /// Add a port to a multicast domain. Port must be < 128 to fit + /// within sidecar-lite's 128-bit replication bitmap. + pub fn domain_port_add( + &mut self, + group_id: u16, + port: u16, + _rid: u16, + _level1_excl_id: u16, + ) -> AsicResult<()> { + if port >= 128 { + return Err(AsicError::InvalidArg(format!( + "port {port} exceeds softnpu 128-port bitmap limit" + ))); + } + + let group = + self.groups.get_mut(&group_id).ok_or_else(|| no_group(group_id))?; + + group.insert(port).then_some(()).ok_or_else(|| { + AsicError::Exists(format!( + "multicast group {group_id} already contains port {port}" + )) + }) + } + + /// Remove a port from a multicast domain. + pub fn domain_port_remove( + &mut self, + group_id: u16, + port: u16, + ) -> AsicResult<()> { + let group = + self.groups.get_mut(&group_id).ok_or_else(|| no_group(group_id))?; + + group.remove(&port).then_some(()).ok_or_else(|| { + AsicError::Missing(format!( + "multicast group {group_id} doesn't contain port {port}" + )) + }) + } + + /// Create a multicast domain. + pub fn domain_create(&mut self, group_id: u16) -> AsicResult<()> { + match self.groups.entry(group_id) { + Entry::Occupied(_) => Err(AsicError::Exists(format!( + "multicast group {group_id} already exists" + ))), + Entry::Vacant(vacant) => { + vacant.insert(HashSet::new()); + Ok(()) + } + } + } + + /// Destroy a multicast domain. + pub fn domain_destroy(&mut self, group_id: u16) -> AsicResult<()> { + self.groups.remove(&group_id).ok_or_else(|| no_group(group_id))?; + self.repl_entries.retain(|_, entry| { + entry.external_grp != group_id && entry.underlay_grp != group_id + }); + Ok(()) + } + + /// Get the total number of multicast domains. + pub fn domains_count(&self) -> usize { + self.groups.len() + } + + /// Record a replication table entry's group references at (table-)write + /// time so that a later membership mutation can refresh its related + /// bitmaps. + pub fn record_repl_entry( + &mut self, + keyset: Vec, + external_grp: u16, + underlay_grp: u16, + rid: u16, + ) { + self.repl_entries + .insert(keyset, ReplEntryRef { external_grp, underlay_grp, rid }); + } + + /// Forget a replication table entry when it is removed from the pipeline. + pub fn forget_repl_entry(&mut self, keyset: &[u8]) { + self.repl_entries.remove(keyset); + } + + /// Forget every tracked replication entry when the replication table is + /// cleared, so a later membership change for a reused group identifier + /// cannot resurrect a stale bitmap from a forgotten keyset. + pub fn clear_repl_entries(&mut self) { + self.repl_entries.clear(); + } + + /// Recompute the bitmaps for every replication entry that references + /// `group_id` in either its external or underlay slot, returning the + /// refreshed [`RefreshedReplEntry`] values for the caller to rewrite into + /// the SoftNPU pipeline. + pub fn refresh_params_for_group( + &self, + group_id: u16, + ) -> Vec { + self.repl_entries + .iter() + .filter(|(_, e)| { + e.external_grp == group_id || e.underlay_grp == group_id + }) + .map(|(keyset, e)| RefreshedReplEntry { + keyset: keyset.clone(), + external_bitmap: self.port_bitmap(e.external_grp), + underlay_bitmap: self.port_bitmap(e.underlay_grp), + rid: e.rid, + }) + .collect() + } + + /// Validate that the current group count does not exceed the limit. + pub fn set_max_nodes( + &mut self, + max_nodes: u32, + _max_link_aggregated_nodes: u32, + ) -> AsicResult<()> { + let total = self.domains_count(); + if u64::try_from(total).unwrap_or(u64::MAX) > u64::from(max_nodes) { + return Err(AsicError::InvalidArg(format!( + "number of multicast groups {total} exceeds max nodes {max_nodes}" + ))); + } + + Ok(()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn group_lifecycle() { + let mut group_data = McGroupData::default(); + + // Create group, add ports. + group_data.domain_create(100).unwrap(); + group_data.domain_port_add(100, 1, 0, 0).unwrap(); + group_data.domain_port_add(100, 5, 0, 0).unwrap(); + + assert_eq!(group_data.domain_port_count(100).unwrap(), 2); + assert_eq!(group_data.domains_count(), 1); + + // Remove a port. + group_data.domain_port_remove(100, 1).unwrap(); + assert_eq!(group_data.domain_port_count(100).unwrap(), 1); + + // Destroy group. + group_data.domain_destroy(100).unwrap(); + assert_eq!(group_data.domains_count(), 0); + } + + #[test] + fn duplicate_group_rejected() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + assert!(matches!( + group_data.domain_create(1), + Err(AsicError::Exists(_)) + )); + } + + #[test] + fn duplicate_port_rejected() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + group_data.domain_port_add(1, 5, 0, 0).unwrap(); + assert!(matches!( + group_data.domain_port_add(1, 5, 0, 0), + Err(AsicError::Exists(_)) + )); + } + + #[test] + fn remove_nonexistent_port_rejected() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + assert!(matches!( + group_data.domain_port_remove(1, 99), + Err(AsicError::Missing(_)) + )); + } + + #[test] + fn operations_on_missing_group_rejected() { + let mut group_data = McGroupData::default(); + assert!(matches!( + group_data.domain_port_add(42, 1, 0, 0), + Err(AsicError::Missing(_)) + )); + assert!(matches!( + group_data.domain_port_remove(42, 1), + Err(AsicError::Missing(_)) + )); + assert!(matches!( + group_data.domain_port_count(42), + Err(AsicError::Missing(_)) + )); + assert!(matches!( + group_data.domain_destroy(42), + Err(AsicError::Missing(_)) + )); + } + + #[test] + fn port_bitmap_zero_for_empty_or_unknown_group() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + assert_eq!(group_data.port_bitmap(1), 0); + assert_eq!(group_data.port_bitmap(999), 0); + } + + #[test] + fn port_bitmap_tracks_membership() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + group_data.domain_port_add(1, 0, 0, 0).unwrap(); + group_data.domain_port_add(1, 3, 0, 0).unwrap(); + group_data.domain_port_add(1, 7, 0, 0).unwrap(); + assert_eq!(group_data.port_bitmap(1), (1 << 0) | (1 << 3) | (1 << 7)); + + group_data.domain_port_remove(1, 0).unwrap(); + assert_eq!(group_data.port_bitmap(1), (1 << 3) | (1 << 7)); + } + + #[test] + fn port_bitmap_high_port() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + group_data.domain_port_add(1, 127, 0, 0).unwrap(); + + let bm = group_data.port_bitmap(1); + assert_eq!(bm & (1u128 << 127), 1u128 << 127); + } + + #[test] + fn port_add_rejects_ports_above_127() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + // Port 128 is out of range for a 128-bit bitmap. + assert!(matches!( + group_data.domain_port_add(1, 128, 0, 0), + Err(AsicError::InvalidArg(_)) + )); + } + + #[test] + fn set_max_nodes_validates() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + group_data.domain_create(2).unwrap(); + + assert!(matches!( + group_data.set_max_nodes(1, 0), + Err(AsicError::InvalidArg(_)) + )); + assert!(group_data.set_max_nodes(2, 0).is_ok()); + assert!(group_data.set_max_nodes(100, 0).is_ok()); + } + + #[test] + fn refresh_recomputes_bitmap_after_membership_change() { + let mut group_data = McGroupData::default(); + // Underlay group with one member, recorded as a replication + // entry keyed by the underlay destination address bytes. + group_data.domain_create(65533).unwrap(); + group_data.domain_port_add(65533, 1, 0, 0).unwrap(); + // ff04::1, an admin-scoped IPv6 multicast destination, matching the + // 128-bit key of the sidecar-lite replication table. + let keyset = std::net::Ipv6Addr::new(0xff04, 0, 0, 0, 0, 0, 0, 1) + .octets() + .to_vec(); + group_data.record_repl_entry(keyset.clone(), 0, 65533, 65533); + + // Refreshed against current membership @ only port 1. + let initial = group_data.refresh_params_for_group(65533); + assert_eq!(initial.len(), 1); + assert_eq!(initial[0].underlay_bitmap, 1u128 << 1); + + // A later member add must surface in a fresh refresh. + group_data.domain_port_add(65533, 2, 0, 0).unwrap(); + group_data.domain_port_add(65533, 3, 0, 0).unwrap(); + let refreshed = group_data.refresh_params_for_group(65533); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].keyset, keyset); + assert_eq!(refreshed[0].rid, 65533); + assert_eq!( + refreshed[0].underlay_bitmap, + (1u128 << 1) | (1u128 << 2) | (1u128 << 3) + ); + } + + #[test] + fn refresh_skips_unrelated_groups_and_forgotten_entries() { + let mut group_data = McGroupData::default(); + group_data.domain_create(100).unwrap(); + group_data.domain_create(200).unwrap(); + group_data.domain_port_add(100, 4, 0, 0).unwrap(); + + // These are opaque single-byte keysets standing in for distinct + // replication entries (not any real address). + let key_a = vec![0xaa]; + let key_b = vec![0xbb]; + + group_data.record_repl_entry(key_a.clone(), 0, 100, 7); + group_data.record_repl_entry(key_b, 0, 200, 8); + + // Only the entry referencing group 100 is refreshed. + let refreshed = group_data.refresh_params_for_group(100); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].keyset, key_a); + + // After forgetting it, the group has no entries to refresh. + group_data.forget_repl_entry(&key_a); + assert!(group_data.refresh_params_for_group(100).is_empty()); + } + + #[test] + fn refresh_matches_external_slot() { + let mut group_data = McGroupData::default(); + group_data.domain_create(50).unwrap(); + group_data.domain_port_add(50, 9, 0, 0).unwrap(); + // Group 50 sits in the external slot here, not the underlay slot. + group_data.record_repl_entry(vec![0xcc], 50, 0, 1); + + let refreshed = group_data.refresh_params_for_group(50); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].external_bitmap, 1u128 << 9); + assert_eq!(refreshed[0].underlay_bitmap, 0); + } + + #[test] + fn clear_repl_entries_drops_all_tracked_entries() { + let mut group_data = McGroupData::default(); + group_data.domain_create(100).unwrap(); + group_data.domain_create(200).unwrap(); + group_data.domain_port_add(100, 4, 0, 0).unwrap(); + group_data.domain_port_add(200, 5, 0, 0).unwrap(); + group_data.record_repl_entry(vec![0xaa], 0, 100, 7); + group_data.record_repl_entry(vec![0xbb], 0, 200, 8); + + // Both entries are tracked before the clear. + assert_eq!(group_data.refresh_params_for_group(100).len(), 1); + assert_eq!(group_data.refresh_params_for_group(200).len(), 1); + + group_data.clear_repl_entries(); + + // After clearing, no keyset survives to be resurrected, even though the + // group memberships themselves are untouched. + assert!(group_data.refresh_params_for_group(100).is_empty()); + assert!(group_data.refresh_params_for_group(200).is_empty()); + assert_eq!(group_data.domain_port_count(100).unwrap(), 1); + assert_eq!(group_data.domain_port_count(200).unwrap(), 1); + } + + #[test] + fn destroy_group_drops_its_repl_entries() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + group_data.domain_create(2).unwrap(); + group_data.domain_port_add(2, 3, 0, 0).unwrap(); + group_data.record_repl_entry(vec![0xaa], 1, 2, 7); + group_data.record_repl_entry(vec![0xbb], 2, 2, 8); + + group_data.domain_destroy(1).unwrap(); + + let refreshed = group_data.refresh_params_for_group(2); + assert_eq!(refreshed.len(), 1); + assert_eq!(refreshed[0].keyset, vec![0xbb]); + } +} diff --git a/asic/src/softnpu/mod.rs b/asic/src/softnpu/mod.rs index 5d539e9b..619edebb 100644 --- a/asic/src/softnpu/mod.rs +++ b/asic/src/softnpu/mod.rs @@ -7,7 +7,7 @@ use std::collections::HashMap; use std::sync::Mutex; -use slog::{Logger, o}; +use slog::{Logger, debug, o}; use tokio::sync::mpsc; use uuid::Uuid; @@ -27,6 +27,7 @@ use common::ports::{ use softnpu_lib::ManagementRequest; +pub mod mcast; pub mod mgmt; pub mod table; @@ -102,8 +103,8 @@ pub struct Handle { log: Logger, ports: Mutex>, pub mgmt_config: mgmt::ManagementConfig, - update_tx: Mutex>>, + mc_data: Mutex, } impl Handle { @@ -129,6 +130,7 @@ impl Handle { ports: Mutex::new(HashMap::new()), mgmt_config, update_tx: Mutex::new(None), + mc_data: Mutex::new(mcast::McGroupData::default()), }) } @@ -142,50 +144,130 @@ impl Handle { } pub fn fini(&self) {} + + /// Rewrite the replication entries refreshed after a membership change. + /// + /// SoftNPU fixes each entry's port bitmaps at write time and has no + /// in-place table update. Each affected entry is removed and re-added with + /// the recomputed bitmaps. + fn rewrite_replication_entries( + &self, + entries: Vec, + ) { + use softnpu_lib::{TableAdd, TableRemove}; + + for entry in entries { + // Logged at debug, not trace, to keep the recomputed membership + // visible in a release build. The bitmaps are the post-refresh + // output ports; an empty one means that the group has no members + // in it to replicate to. + debug!( + self.log, + "refreshing mcast replication entry: \ + external_bitmap={:#x} underlay_bitmap={:#x} rid={}", + entry.external_bitmap, + entry.underlay_bitmap, + entry.rid + ); + + let remove = ManagementRequest::TableRemove(TableRemove { + keyset_data: entry.keyset.clone(), + table: mcast::MCAST_REPLICATION_V6_TABLE.to_string(), + }); + mgmt::write(remove, &self.mgmt_config); + + let parameter_data = [ + entry.external_bitmap.to_le_bytes().as_slice(), + entry.underlay_bitmap.to_le_bytes().as_slice(), + entry.rid.to_le_bytes().as_slice(), + ] + .concat(); + + let add = ManagementRequest::TableAdd(TableAdd { + table: mcast::MCAST_REPLICATION_V6_TABLE.to_string(), + action: mcast::SET_PORT_BITMAP_ACTION.to_string(), + keyset_data: entry.keyset, + parameter_data, + }); + mgmt::write(add, &self.mgmt_config); + } + } } impl AsicMulticastOps for Handle { fn mc_domains(&self) -> Vec { - let len = self.ports.lock().unwrap().len() as u16; - (0..len).collect() + let mc_data = self.mc_data.lock().unwrap(); + mc_data.domains() } - fn mc_port_count(&self, _group_id: u16) -> AsicResult { - Ok(self.ports.lock().unwrap().len()) + fn mc_port_count(&self, group_id: u16) -> AsicResult { + let mc_data = self.mc_data.lock().unwrap(); + mc_data.domain_port_count(group_id) } fn mc_port_add( &self, - _group_id: u16, - _port: u16, - _rid: u16, - _level1_excl_id: u16, + group_id: u16, + port: u16, + rid: u16, + level1_excl_id: u16, ) -> AsicResult<()> { - Err(AsicError::OperationUnsupported) + slog::info!( + self.log, + "adding port {port} to multicast group {group_id}" + ); + // Hold the lock across the refresh and rewrite for the + // recomputation of the bitmap snapshot and its table writes are atomic + // concerning concurrent membership change(s). The rewrite touches only + // `mgmt_config`, not `mc_data`; therefore, no re-entrant lock. + let mut mc_data = self.mc_data.lock().unwrap(); + mc_data.domain_port_add(group_id, port, rid, level1_excl_id)?; + let refreshed = mc_data.refresh_params_for_group(group_id); + self.rewrite_replication_entries(refreshed); + Ok(()) } - fn mc_port_remove(&self, _group_id: u16, _port: u16) -> AsicResult<()> { + fn mc_port_remove(&self, group_id: u16, port: u16) -> AsicResult<()> { + slog::info!( + self.log, + "removing port {port} from multicast group {group_id}" + ); + let mut mc_data = self.mc_data.lock().unwrap(); + mc_data.domain_port_remove(group_id, port)?; + let refreshed = mc_data.refresh_params_for_group(group_id); + self.rewrite_replication_entries(refreshed); Ok(()) } - fn mc_group_create(&self, _group_id: u16) -> AsicResult<()> { - Err(AsicError::OperationUnsupported) + fn mc_group_create(&self, group_id: u16) -> AsicResult<()> { + slog::info!(self.log, "creating multicast group {group_id}"); + let mut mc_data = self.mc_data.lock().unwrap(); + mc_data.domain_create(group_id) } - fn mc_group_destroy(&self, _group_id: u16) -> AsicResult<()> { - Ok(()) + fn mc_group_destroy(&self, group_id: u16) -> AsicResult<()> { + slog::info!(self.log, "destroying multicast group {group_id}"); + let mut mc_data = self.mc_data.lock().unwrap(); + mc_data.domain_destroy(group_id) } fn mc_groups_count(&self) -> AsicResult { - Ok(self.ports.lock().unwrap().len()) + let mc_data = self.mc_data.lock().unwrap(); + Ok(mc_data.domains_count()) } fn mc_set_max_nodes( &self, - _max_nodes: u32, - _max_link_aggregated_nodes: u32, + max_nodes: u32, + max_link_aggregated_nodes: u32, ) -> AsicResult<()> { - Ok(()) + slog::info!( + self.log, + "setting max nodes to {max_nodes}, \ + max link aggregated nodes to {max_link_aggregated_nodes}" + ); + let mut mc_data = self.mc_data.lock().unwrap(); + mc_data.set_max_nodes(max_nodes, max_link_aggregated_nodes) } } @@ -241,7 +323,7 @@ impl AsicOps for Handle { let asic_port_id = self.port_to_asic_id(port_hdl)?; let tx = self.update_tx.lock().unwrap(); if let Some(tx) = tx.as_ref() { - // When a port is enabled in softnpu, it automatically comes online. + // When a port is enabled in SoftNPU, it automatically comes online. // When switching between enabled and disabled, we send the main body // of dpd the PortUpdate events we would expect to see on real hardware. let present = @@ -267,7 +349,7 @@ impl AsicOps for Handle { } } } else { - slog::debug!(self.log, "no PortUpdate handler registered"); + debug!(self.log, "no PortUpdate handler registered"); } Ok(()) } diff --git a/asic/src/softnpu/table.rs b/asic/src/softnpu/table.rs index 56d9c1fe..9ffbcf5e 100644 --- a/asic/src/softnpu/table.rs +++ b/asic/src/softnpu/table.rs @@ -4,6 +4,8 @@ // // Copyright 2026 Oxide Computer Company +use std::hash::Hash; + use slog::{error, trace}; use softnpu_lib::{ManagementRequest, TableAdd, TableRemove}; @@ -14,16 +16,37 @@ use aal::{ }; use common::table::TableType; -/// Represents a handle to a SoftNPU ASIC table. The `id` member corresponds to -/// the table path in the P4 program. Well known sidecar-lite.p4 paths follow -/// below. +// Match field names used by the VLAN validity dispatch. +const VALID_FIELD: &str = "$valid"; +const VLAN_ID_FIELD: &str = "vlan_id"; + +// Sidecar-lite untagged table names related to VLAN dispatch. +const MCAST_NAT_V4_UNTAGGED: &str = "ingress.nat.nat_v4_mcast_untagged"; +const MCAST_NAT_V6_UNTAGGED: &str = "ingress.nat.nat_v6_mcast_untagged"; + +/// Represents a handle to a SoftNPU ASIC table. +/// +/// The `type_` member identifies the table and `softnpu_table_name()` +/// maps it to the corresponding path in sidecar-lite.p4. pub struct Table { type_: TableType, implemented: bool, + /// Alternate sidecar-lite table for untagged entries ([`VALID_FIELD`] is + /// false). + /// + /// x4c does not support `$valid` as a table key. Any table that matches + /// on VLAN validity splits into tagged/untagged variants per entry. + /// [`VLAN_ID_FIELD`] is stripped from the keyset for the untagged + /// table. + /// + /// Currently used by multicast NAT. + untagged_id: Option<&'static str>, size: usize, } impl Table { + /// Return the sidecar-lite table name for this table, or `None` if the + /// table is not implemented in the SoftNPU backend. pub fn softnpu_table_name(&self) -> Option<&'static str> { if self.implemented { match self.type_ { @@ -44,6 +67,28 @@ impl Table { TableType::ArpIpv4 => Some("ingress.resolver.resolver_v4"), TableType::NeighborIpv6 => Some("ingress.resolver.resolver_v6"), TableType::PortMacAddress => Some("ingress.mac.mac_rewrite"), + TableType::McastIpv6 => { + Some(crate::softnpu::mcast::MCAST_REPLICATION_V6_TABLE) + } + TableType::McastIpv4SrcFilter => { + Some("ingress.mcast.mcast_source_filter_v4") + } + TableType::McastIpv6SrcFilter => { + Some("ingress.mcast.mcast_source_filter_v6") + } + TableType::NatIngressIpv4Mcast => { + Some("ingress.nat.nat_v4_mcast") + } + TableType::NatIngressIpv6Mcast => { + Some("ingress.nat.nat_v6_mcast") + } + TableType::McastEgressDecapPorts => { + Some("egress.tbl_decap_ports") + } + TableType::PortMacAddressMcast => Some("egress.mcast_src_mac"), + TableType::McastEgressPortMapping => { + Some("egress.asic_id_to_port") + } _ => panic!( "implemented table {} has no softnpu table", self.type_ @@ -53,16 +98,161 @@ impl Table { None } } + + /// Inspect the VLAN `$valid` match field to route entries to the + /// tagged or untagged sidecar-lite table. + /// + /// Returns the primary table name for tables without a VLAN split, or + /// `None` when the table is unimplemented. + fn resolve_vlan_table_id( + &self, + fields: &[MatchEntryField], + ) -> Option<&str> { + if let Some(untagged) = self.untagged_id { + let is_untagged = fields.iter().any(|f| { + f.name == VALID_FIELD + && matches!( + &f.value, + MatchEntryValue::Value(ValueTypes::U64(0)) + ) + }); + if is_untagged { + return Some(untagged); + } + } + self.softnpu_table_name() + } + + /// Filter match fields for sidecar-lite serialization. + /// + /// This strips the `$valid` field and strips the `vlan_id` when targeting + /// the untagged table. + fn filter_vlan_match_fields( + &self, + fields: Vec, + target_table: &str, + ) -> Vec { + if self.untagged_id.is_none() { + return fields; + } + + let is_untagged = self.untagged_id == Some(target_table); + fields + .into_iter() + .filter(|f| { + // Dispatch already consumed $valid so we can drop it here. + if f.name == VALID_FIELD { + return false; + } + // Untagged table has no vlan_id key. + if is_untagged && f.name == VLAN_ID_FIELD { + return false; + } + true + }) + .collect() + } + + /// Translate a `configure_mcastv6` request into a sidecar-lite + /// `set_port_bitmap` addition, holding the replication tracking lock across + /// the entire op. + /// + /// DPD sends `(mcast_grp_a, mcast_grp_b, rid, level1_excl_id, + /// level2_excl_id)`, while sidecar-lite expects `(external, underlay, rid)`, + /// from `McGroupData` membership. On SoftNPU, the bitmap is a write-time + /// snapshot, unlike the live replication engine on the sidecar, allowing + /// recorded group references to recompute later membership changes to the + /// entry. + /// + /// The lock is held from the bitmap snapshot through to the table write on + /// a concurrent membership refresh, which takes the same lock across its + /// own rewrite (see @ `mc_port_add`). + /// + /// This cannot interleave a newer bitmap that this older snapshot would + /// then overwrite. + fn mcast_replication_add( + &self, + hdl: &Handle, + table: String, + keyset_data: Vec, + action_data: &aal::ActionData, + ) -> AsicResult<()> { + let action = &action_data.action; + let external_grp = arg_u16(hdl, action_data, action, "mcast_grp_a")?; + let underlay_grp = arg_u16(hdl, action_data, action, "mcast_grp_b")?; + let rid = arg_u16(hdl, action_data, action, "rid")?; + + let mut mc_data = hdl.mc_data.lock().unwrap(); + let external_bitmap = mc_data.port_bitmap(external_grp); + let underlay_bitmap = mc_data.port_bitmap(underlay_grp); + mc_data.record_repl_entry( + keyset_data.clone(), + external_grp, + underlay_grp, + rid, + ); + + let parameter_data = [ + external_bitmap.to_le_bytes().as_slice(), + underlay_bitmap.to_le_bytes().as_slice(), + rid.to_le_bytes().as_slice(), + ] + .concat(); + + // Logged at debug for the initial bitmap snapshot to be visible in a + // release build. + slog::debug!( + hdl.log, + "mcast replication add: external_grp={external_grp} \ + underlay_grp={underlay_grp} external_bitmap={external_bitmap:#x} \ + underlay_bitmap={underlay_bitmap:#x} rid={rid}" + ); + trace!(hdl.log, "table: {table}"); + trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); + trace!(hdl.log, "parameter_data:\n{parameter_data:#?}"); + + replace_entry( + hdl, + table, + crate::softnpu::mcast::SET_PORT_BITMAP_ACTION.to_string(), + keyset_data, + parameter_data, + ); + + Ok(()) + } } -// All tables are defined to be 1024 entries deep +fn replace_entry( + hdl: &Handle, + table: String, + action: String, + keyset_data: Vec, + parameter_data: Vec, +) { + let remove = ManagementRequest::TableRemove(TableRemove { + table: table.clone(), + keyset_data: keyset_data.clone(), + }); + crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); + + let add = ManagementRequest::TableAdd(TableAdd { + table, + action, + keyset_data, + parameter_data, + }); + crate::softnpu::mgmt::write(add, &hdl.mgmt_config); +} + +// All tables are defined to be 4096 entries deep. const TABLE_SIZE: usize = 4096; impl TableOps for Table { fn new(hdl: &Handle, type_: TableType) -> AsicResult { - // TODO just mapping sidecar.p4 things onto simplified sidecar-lite.p4 - // things to get started. - let implemented = match type_ { + // Mapping sidecar.p4 table types onto simplified sidecar-lite.p4 + // equivalents. + let (implemented, untagged_id) = match type_ { TableType::RouteIdxIpv4 | TableType::RouteFwdIpv4 | TableType::RouteIdxIpv6 @@ -75,22 +265,39 @@ impl TableOps for Table { | TableType::NatIngressIpv4 | TableType::NatIngressIpv6 | TableType::AttachedSubnetIpv4 - | TableType::AttachedSubnetIpv6 => true, + | TableType::AttachedSubnetIpv6 => (true, None), + TableType::McastIpv6 + | TableType::McastIpv4SrcFilter + | TableType::McastIpv6SrcFilter + | TableType::McastEgressDecapPorts + | TableType::McastEgressPortMapping + | TableType::PortMacAddressMcast => (true, None), + TableType::NatIngressIpv4Mcast => { + (true, Some(MCAST_NAT_V4_UNTAGGED)) + } + TableType::NatIngressIpv6Mcast => { + (true, Some(MCAST_NAT_V6_UNTAGGED)) + } x => { - error!(hdl.log, "TABLE NOT HANDLED {x}"); - false + error!(hdl.log, "table not handled: {x}"); + (false, None) } }; - Ok(Table { type_, implemented, size: TABLE_SIZE }) + Ok(Table { type_, implemented, untagged_id, size: TABLE_SIZE }) } fn size(&self) -> usize { self.size } - fn clear(&self, _hdl: &Handle) -> AsicResult<()> { - //TODO implement in softnpu + fn clear(&self, hdl: &Handle) -> AsicResult<()> { + // TODO: the management protocol has no clear message, meaning + // that entries stay in the pipeline. Send a `TableClear` once + // softnpu and propolis support one. + if self.type_ == TableType::McastIpv6 { + hdl.mc_data.lock().unwrap().clear_repl_entries(); + } Ok(()) } @@ -100,19 +307,43 @@ impl TableOps for Table { key: &M, data: &A, ) -> AsicResult<()> { - let Some(table) = self.softnpu_table_name() else { + let Some(default_table) = self.softnpu_table_name() else { return Ok(()); }; let name = self.type_.to_string(); let match_data = key.key_to_ir().unwrap(); let action_data = data.action_to_ir().unwrap(); + // For tables with VLAN dispatch, resolve which sidecar-lite table + // to target and filter out synthetic match fields. + let (table, fields) = { + let resolved = self + .resolve_vlan_table_id(&match_data.fields) + .unwrap_or(default_table); + let filtered = + self.filter_vlan_match_fields(match_data.fields, resolved); + (resolved.to_string(), filtered) + }; trace!(hdl.log, "entry_add called"); trace!(hdl.log, "table: {name}"); - trace!(hdl.log, "match_data:\n{:#?}", match_data); + trace!(hdl.log, "match_data (filtered): {fields:#?}"); trace!(hdl.log, "action_data:\n{:#?}", action_data); - let keyset_data = keyset_data(match_data.fields, self.type_); + let keyset_data = keyset_data(fields, self.type_); + + // Multicast replication is handled ahead of the generic match so the + // replication tracking lock can be held from the bitmap snapshot through + // to the table write. + if let (TableType::McastIpv6, "configure_mcastv6") = + (self.type_, action_data.action.as_str()) + { + return self.mcast_replication_add( + hdl, + table, + keyset_data, + &action_data, + ); + } let (action, parameter_data) = match ( self.type_, @@ -432,130 +663,90 @@ impl TableOps for Table { | (TableType::NatIngressIpv6, "forward_ipv6_to") | (TableType::AttachedSubnetIpv4, "forward_to_v4") | (TableType::AttachedSubnetIpv6, "forward_to_v6") => { - let mut target = Vec::new(); - let mut vni = Vec::new(); - let mut mac = Vec::new(); + forward_to_sled_params(hdl, &name, action_data)? + } + (TableType::NatIngressIpv4Mcast, "mcast_forward_ipv4_to") + | (TableType::NatIngressIpv6Mcast, "mcast_forward_ipv6_to") => { + forward_to_sled_params(hdl, &name, action_data)? + } + // Multicast source filters: no action parameters. + (TableType::McastIpv4SrcFilter, "allow_source_mcastv4") + | (TableType::McastIpv6SrcFilter, "allow_source_mcastv6") => { + ("allow_source", Vec::new()) + } + // Multicast egress decap: pack 8x32-bit bitmap into 128 bits. + // + // DPD sends set_decap_ports with 8x32-bit bitmap fields + // keyed on the RID. Sidecar-lite expects a single 128-bit + // bitmap. We pack the low 4 chunks (ports 0-127) into a + // u128 for sidecar-lite's bit<128> decap_bitmap field. + (TableType::McastEgressDecapPorts, "set_decap_ports") => { + ("set_decap_ports", pack_decap_bitmap(&action_data)) + } + (TableType::McastEgressDecapPorts, "set_decap_ports_and_vlan") => { + let mut params = pack_decap_bitmap(&action_data); + let vlan_id = arg_u16(hdl, &action_data, &name, "vlan_id")?; + params.extend_from_slice(&vlan_id.to_le_bytes()); + ("set_decap_ports_and_vlan", params) + } + // Multicast egress MAC rewrite. + (TableType::PortMacAddressMcast, "rewrite") => { + let mut params = Vec::new(); for arg in action_data.args { - match arg.name.as_str() { - "target" => { - // "target" is 128 bits - let mut data: Vec = Vec::new(); - match &arg.value { - ValueTypes::U64(_) => { - // Currently the ValueType is always Ptr - error!( - hdl.log, - "expected ValueType::Ptr, \ - received ValueType::U64" - ); - return Ok(()); - } - ValueTypes::Ptr(v) => { - data.extend_from_slice(v.as_slice()); - } - } - let len = data.len(); - let buf = &mut data[len - 16..]; - buf.reverse(); - target.extend_from_slice(buf); - } - "vni" => { - // "vni" is 24 bits - let mut data: Vec = Vec::new(); - match &arg.value { - ValueTypes::U64(v) => { - data.extend_from_slice(&v.to_le_bytes()); - } - ValueTypes::Ptr(_) => { - // Currently the ValueType is always U64 - error!( - hdl.log, - "expected ValueType::U64, \ - received ValueType::Ptr" - ); - return Ok(()); - } - } - vni.extend_from_slice(&data[0..3]); - } - "inner_mac" => { - // "mac" is 48 bits - let mut data: Vec = Vec::new(); - match &arg.value { - ValueTypes::U64(v) => { - data.extend_from_slice(&v.to_le_bytes()); - } - ValueTypes::Ptr(_) => { - // Currently the ValueType is always U64 - error!( - hdl.log, - "expected ValueType::U64, \ - received ValueType::Ptr" - ); - return Ok(()); - } - } - mac.extend_from_slice(&data[0..6]) + match arg.value { + ValueTypes::U64(v) => { + let mac = v.to_le_bytes(); + params.extend_from_slice(&mac[0..6]); } - _ => { - error!(hdl.log, "unknown argument: {}", arg.name); - return Ok(()); + ValueTypes::Ptr(v) => { + params.extend_from_slice(v.as_slice()); } } } - let mut params = Vec::new(); - // arguments currently don't arrive in the correct order, - // so we'll order them manually - params.extend_from_slice(target.as_slice()); - params.extend_from_slice(vni.as_slice()); - params.extend_from_slice(mac.as_slice()); - ("forward_to_sled", params) + ("rewrite_src_mac", params) } + (TableType::McastEgressPortMapping, "set_port_number") => { + ("set_port_number", port_number_param(&action_data)?) + } + // Sidecar-lite's resolvers support drop (by default). + (TableType::ArpIpv4, "drop") + | (TableType::NeighborIpv6, "drop") => ("drop", Vec::new()), + // The local_v4/local_v6 tables have no drop action and dpd programs + // dropv4/dropv6 during port IP setup. + (TableType::PortAddrIpv4, "dropv4") + | (TableType::PortAddrIpv6, "dropv6") => return Ok(()), (_, x) => { - error!(hdl.log, "ACTION NOT HANDLED {name} {x}"); - return Ok(()); + error!(hdl.log, "action not handled: {name} {x}"); + return Err(AsicError::InvalidArg(format!( + "{name}: action {x} not handled by the softnpu backend" + ))); } }; let action = action.to_string(); trace!(hdl.log, "sending request to softnpu"); - trace!(hdl.log, "table: {name}"); + trace!(hdl.log, "table: {table}"); trace!(hdl.log, "action: {:#?}", action); trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); trace!(hdl.log, "parameter_data:\n{:#?}", parameter_data); - let msg = ManagementRequest::TableAdd(TableAdd { - table: table.to_string(), - action, - keyset_data, - parameter_data, - }); - - crate::softnpu::mgmt::write(msg, &hdl.mgmt_config); + replace_entry(hdl, table, action, keyset_data, parameter_data); Ok(()) } - fn entry_update( + fn entry_update( &self, hdl: &Handle, key: &M, data: &A, ) -> AsicResult<()> { - let Some(_table) = self.softnpu_table_name() else { - return Ok(()); - }; - let name = self.type_.to_string(); - - let match_data = key.key_to_ir().unwrap(); - let action_data = data.action_to_ir().unwrap(); - - trace!(hdl.log, "entry_update called"); - trace!(hdl.log, "table: {name}"); - trace!(hdl.log, "match_data:\n{:#?}", match_data); - trace!(hdl.log, "action_data:\n{:#?}", action_data); - - //TODO implement in softnpu - Ok(()) + // SoftNPU does not currently support in-place updates. `entry_add` + // removes the key before adding it back with the new action data. + // + // Note: this is not an atomic op, as a lookup between the two messages + // could miss the entry, and mgmt IO failures panic in mgmt::write + // rather than surface errors. + self.entry_add(hdl, key, data) } fn entry_del( @@ -563,26 +754,38 @@ impl TableOps for Table { hdl: &Handle, key: &M, ) -> AsicResult<()> { - let Some(table) = self.softnpu_table_name() else { + let Some(default_table) = self.softnpu_table_name() else { return Ok(()); }; let name = self.type_.to_string(); let match_data = key.key_to_ir().unwrap(); + let (table, fields) = { + let resolved = self + .resolve_vlan_table_id(&match_data.fields) + .unwrap_or(default_table); + let filtered = + self.filter_vlan_match_fields(match_data.fields, resolved); + (resolved.to_string(), filtered) + }; trace!(hdl.log, "entry_del called"); trace!(hdl.log, "table: {name}"); - trace!(hdl.log, "match_data:\n{:#?}", match_data); + trace!(hdl.log, "match_data (filtered): {fields:#?}"); + + let keyset_data = keyset_data(fields, self.type_); - let keyset_data = keyset_data(match_data.fields, self.type_); + // Stop tracking a replication entry once it leaves the pipeline so a + // later membership change does not resurrect a stale bitmap. + if self.type_ == TableType::McastIpv6 { + hdl.mc_data.lock().unwrap().forget_repl_entry(&keyset_data); + } trace!(hdl.log, "sending request to softnpu"); - trace!(hdl.log, "table: {name}"); + trace!(hdl.log, "table: {table}"); trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); - let msg = ManagementRequest::TableRemove(TableRemove { - keyset_data, - table: table.to_string(), - }); + let msg = + ManagementRequest::TableRemove(TableRemove { keyset_data, table }); crate::softnpu::mgmt::write(msg, &hdl.mgmt_config); @@ -606,8 +809,148 @@ impl TableOps for Table { } } +/// Extract the `forward_to_sled` action parameters shared by unicast and +/// multicast NAT/attached-subnet actions. +/// +/// Returns the sidecar-lite action name and serialized parameter bytes. +fn forward_to_sled_params( + hdl: &Handle, + name: &str, + action_data: aal::ActionData, +) -> AsicResult<(&'static str, Vec)> { + for arg in &action_data.args { + if !matches!(arg.name.as_str(), "target" | "vni" | "inner_mac") { + error!(hdl.log, "unknown argument: {} in {name}", arg.name); + return Err(AsicError::InvalidArg(format!( + "{name}: unknown argument {}", + arg.name + ))); + } + } + + let target = arg_ptr(hdl, &action_data, name, "target")?; + let vni = arg_u64(hdl, &action_data, name, "vni")?; + let mac = arg_u64(hdl, &action_data, name, "inner_mac")?; + + let Some(start) = target.len().checked_sub(16) else { + return Err(AsicError::InvalidArg(format!( + "{name}: target is {} bytes, expected at least 16", + target.len() + ))); + }; + + let mut params = Vec::new(); + let mut target_bytes = target[start..].to_vec(); + target_bytes.reverse(); + params.extend_from_slice(&target_bytes); + params.extend_from_slice(&vni.to_le_bytes()[0..3]); + params.extend_from_slice(&mac.to_le_bytes()[0..6]); + Ok(("forward_to_sled", params)) +} + +fn find_arg<'a>( + action_data: &'a aal::ActionData, + action: &str, + arg_name: &str, +) -> AsicResult<&'a ValueTypes> { + action_data + .args + .iter() + .find(|arg| arg.name == arg_name) + .map(|arg| &arg.value) + .ok_or_else(|| { + AsicError::InvalidArg(format!( + "{action}: missing argument {arg_name}" + )) + }) +} + +fn arg_u64( + hdl: &Handle, + action_data: &aal::ActionData, + action: &str, + arg_name: &str, +) -> AsicResult { + match find_arg(action_data, action, arg_name)? { + ValueTypes::U64(v) => Ok(*v), + ValueTypes::Ptr(_) => { + error!(hdl.log, "expected ValueType::U64, received ValueType::Ptr"); + Err(AsicError::InvalidArg(format!( + "{action}: {arg_name} expects ValueType::U64" + ))) + } + } +} + +fn arg_u16( + hdl: &Handle, + action_data: &aal::ActionData, + action: &str, + arg_name: &str, +) -> AsicResult { + let v = arg_u64(hdl, action_data, action, arg_name)?; + u16::try_from(v).map_err(|_| { + AsicError::InvalidArg(format!("{action}: {arg_name} {v} exceeds u16")) + }) +} + +fn arg_ptr<'a>( + hdl: &Handle, + action_data: &'a aal::ActionData, + action: &str, + arg_name: &str, +) -> AsicResult<&'a [u8]> { + match find_arg(action_data, action, arg_name)? { + ValueTypes::Ptr(v) => Ok(v.as_slice()), + ValueTypes::U64(_) => { + error!(hdl.log, "expected ValueType::Ptr, received ValueType::U64"); + Err(AsicError::InvalidArg(format!( + "{action}: {arg_name} expects ValueType::Ptr" + ))) + } + } +} + +/// Pack the low 4 chunks (ports 0-127) of DPD's 8x32-bit decap port +/// bitmap into the 16 bytes of sidecar-lite's bit<128> decap_bitmap +/// field (in little-endian). +/// +/// Caveats: The high chunks are always zero on this backend and ports >= 128 +/// are rejected when adding members. Emitting exactly 16 bytes keeps +/// any following action parameter (the tagged variant's vlan_id) at +/// the offset sidecar-lite reads it from. +fn pack_decap_bitmap(args: &aal::ActionData) -> Vec { + let mut chunks = [0u32; 4]; + for arg in &args.args { + if let ValueTypes::U64(v) = &arg.value { + match arg.name.as_str() { + "ports_0" => chunks[0] = *v as u32, + "ports_1" => chunks[1] = *v as u32, + "ports_2" => chunks[2] = *v as u32, + "ports_3" => chunks[3] = *v as u32, + _ => {} + } + } + } + chunks.iter().flat_map(|c| c.to_le_bytes()).collect() +} + +fn port_number_param(args: &aal::ActionData) -> AsicResult> { + args.args + .iter() + .find(|arg| arg.name == "port_number") + .and_then(|arg| u8::try_from(&arg.value).ok()) + .map(|port_number| vec![port_number]) + .ok_or_else(|| { + AsicError::InvalidArg(format!( + "{}: missing or invalid port_number", + args.action + )) + }) +} + /// Extract keys from `match_data` and ensure that they are -/// in a data structure with the correct length +/// in a data structure with the correct length. fn keyset_data(match_data: Vec, table: TableType) -> Vec { let mut keyset_data: Vec = Vec::new(); for m in match_data { @@ -622,7 +965,7 @@ fn keyset_data(match_data: Vec, table: TableType) -> Vec { keyset_data.extend_from_slice(&data[..4]); } TableType::NeighborIpv6 => { - // "nexthop_ipv4" => bit<128> + // "nexthop_ipv6" => bit<128> let mut buf = Vec::new(); serialize_value_type(&x, &mut buf); buf.reverse(); @@ -655,6 +998,52 @@ fn keyset_data(match_data: Vec, table: TableType) -> Vec { buf.reverse(); keyset_data.extend_from_slice(&buf); } + // Multicast replication: hdr.ipv6.dst => bit<128> + TableType::McastIpv6 => { + let mut buf = Vec::new(); + serialize_value_type(&x, &mut buf); + buf.reverse(); + keyset_data.extend_from_slice(&buf); + } + // Multicast source filter exact keys: inner dst => + // bit<32> or bit<128>. The LPM src key is handled + // in the LPM branch. + TableType::McastIpv4SrcFilter => { + serialize_value_type(&x, &mut data); + keyset_data.extend_from_slice(&data[..4]); + } + TableType::McastIpv6SrcFilter => { + let mut buf = Vec::new(); + serialize_value_type(&x, &mut buf); + buf.reverse(); + keyset_data.extend_from_slice(&buf); + } + // Multicast NAT: dst (bit<32> or bit<128>) plus + // vlan_id (bit<12>, 2 bytes) only when targeting the + // tagged variant. + TableType::NatIngressIpv4Mcast => { + serialize_value_type(&x, &mut data); + let width = if m.name == VLAN_ID_FIELD { 2 } else { 4 }; + keyset_data.extend_from_slice(&data[..width]); + } + TableType::NatIngressIpv6Mcast => { + if m.name == VLAN_ID_FIELD { + serialize_value_type(&x, &mut data); + keyset_data.extend_from_slice(&data[..2]); + } else { + let mut buf = Vec::new(); + serialize_value_type(&x, &mut buf); + buf.reverse(); + keyset_data.extend_from_slice(&buf); + } + } + // Multicast egress tables: port => bit<16> + TableType::McastEgressDecapPorts + | TableType::McastEgressPortMapping + | TableType::PortMacAddressMcast => { + serialize_value_type(&x, &mut data); + keyset_data.extend_from_slice(&data[..2]); + } _ => { serialize_value_type(&x, &mut keyset_data); } @@ -670,7 +1059,13 @@ fn keyset_data(match_data: Vec, table: TableType) -> Vec { serialize_value_type_be(&x.prefix, &mut data); keyset_data.extend_from_slice(&data[data.len() - 4..]); // prefix length for longest prefix match operation - keyset_data.push(x.len as u8) + keyset_data.push(x.len as u8); + } + TableType::McastIpv4SrcFilter => { + // "src_addr" => bit<32> lpm + serialize_value_type_be(&x.prefix, &mut data); + keyset_data.extend_from_slice(&data[data.len() - 4..]); + keyset_data.push(x.len as u8); } _ => { serialize_value_type_be(&x.prefix, &mut keyset_data); @@ -682,7 +1077,7 @@ fn keyset_data(match_data: Vec, table: TableType) -> Vec { MatchEntryValue::Range(x) => { match table { TableType::NatIngressIpv4 | TableType::NatIngressIpv6 => { - // "l4_dst_port" => ingress.nat_id: range => bit<16> + // "l4_dst_port" => ingress.nat_id: range => bit<16> let low = &x.low.to_le_bytes(); let high = &x.high.to_le_bytes(); keyset_data.extend_from_slice(&low[..2]); @@ -725,3 +1120,38 @@ fn serialize_value_type_be(x: &ValueTypes, data: &mut Vec) { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn port_mapping_serializes_for_sidecar_lite() { + let fields = vec![MatchEntryField { + name: "egress_port".to_string(), + value: MatchEntryValue::Value(ValueTypes::U64(0x0102)), + }]; + assert_eq!( + keyset_data(fields, TableType::McastEgressPortMapping), + vec![0x02, 0x01] + ); + + let action_data = aal::ActionData { + action: "set_port_number".to_string(), + args: vec![aal::ActionArg { + name: "port_number".to_string(), + value: ValueTypes::U64(17), + }], + }; + assert_eq!(port_number_param(&action_data).unwrap(), vec![17]); + + let missing = aal::ActionData { + action: "set_port_number".to_string(), + args: Vec::new(), + }; + assert!(matches!( + port_number_param(&missing), + Err(AsicError::InvalidArg(_)) + )); + } +} diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 273e5f88..47e2334d 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -2237,6 +2237,14 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); + let ctr_baseline_mcast_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast")) + .await + .unwrap(); + let ctr_baseline_mcast_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast")) + .await + .unwrap(); let ctr_baseline_external_egress1 = switch .get_counter(&port_label_egress1, Some("multicast_external")) .await @@ -2266,6 +2274,26 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() .await .unwrap(); + // One decapped replica should be counted per external member port. + check_counter_incremented( + switch, + &port_label_egress1, + ctr_baseline_mcast_egress1, + 1, + Some("multicast"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress2, + ctr_baseline_mcast_egress2, + 1, + Some("multicast"), + ) + .await + .unwrap(); + // Decapped replicas to external members must be attributed to the // external counter, not the underlay one. check_counter_incremented( @@ -2621,6 +2649,22 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); + let ctr_baseline_mcast_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast")) + .await + .unwrap(); + let ctr_baseline_mcast_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast")) + .await + .unwrap(); + let ctr_baseline_mcast_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast")) + .await + .unwrap(); + let ctr_baseline_mcast_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast")) + .await + .unwrap(); let ctr_baseline_underlay_egress1 = switch .get_counter(&port_label_egress1, Some("multicast_underlay")) .await @@ -2666,6 +2710,24 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe .await .unwrap(); + // One replica should be counted per member port, external & underlay alike. + for (label, baseline) in [ + (&port_label_egress1, ctr_baseline_mcast_egress1), + (&port_label_egress2, ctr_baseline_mcast_egress2), + (&port_label_egress3, ctr_baseline_mcast_egress3), + (&port_label_egress4, ctr_baseline_mcast_egress4), + ] { + check_counter_incremented( + switch, + label, + baseline, + 1, + Some("multicast"), + ) + .await + .unwrap(); + } + // Still-encapsulated replicas to underlay members count as underlay, // not external, for UNDERLAY_EXTERNAL tagged groups. check_counter_incremented( @@ -7568,6 +7630,98 @@ async fn test_source_filter_update_to_any() -> TestResult { "After update with Any, should have only 1 entry (the /0)" ); + // Narrow to specific sources for (*,G) -> (S,G) transitions, which is + // exercised below with `sources: None`. + let narrow_entry = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::ExternalInternalForwarding { + nat_target: nat_target.clone(), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(10) }, + sources: Some(vec![ + types::IpSrc::Exact("192.168.1.1".parse().unwrap()), + types::IpSrc::Exact("192.168.1.2".parse().unwrap()), + ]), + }; + + let narrowed = switch + .client + .multicast_group_update_external( + &external_group_ip.try_into().unwrap(), + &make_tag(TEST_TAG), + &narrow_entry, + ) + .await + .expect("Should narrow external group sources") + .into_inner(); + + assert_eq!( + narrowed.sources.as_deref().map(<[_]>::len), + Some(2), + "Narrowing should restore 2 specific sources" + ); + + let after_narrowed_table = switch + .client + .table_dump(SOURCE_FILTER_IPV4_TABLE, false) + .await + .expect("Should dump table after narrowing"); + + assert_eq!( + after_narrowed_table.entries.len(), + baseline_count + 2, + "Narrowing should restore 2 specific entries" + ); + + let keep_entry = types::MulticastGroupUpdateExternalEntry { + internal_forwarding: types::ExternalInternalForwarding { + nat_target: nat_target.clone(), + }, + external_forwarding: types::ExternalForwarding { vlan_id: Some(10) }, + sources: None, + }; + + let kept = switch + .client + .multicast_group_update_external( + &external_group_ip.try_into().unwrap(), + &make_tag(TEST_TAG), + &keep_entry, + ) + .await + .expect("Should update external group with sources: None") + .into_inner(); + + assert_eq!( + kept.sources, narrowed.sources, + "sources: None must preserve the current filter" + ); + + // Round-trip via GET to confirm the stored representation matches. + let fetched = switch + .client + .multicast_group_get(&external_group_ip) + .await + .expect("Should get external group") + .into_inner(); + + assert_eq!( + get_sources(&fetched), + kept.sources, + "GET after sources: None must agree with the update response" + ); + + let after_keep_table = switch + .client + .table_dump(SOURCE_FILTER_IPV4_TABLE, false) + .await + .expect("Should dump table after sources: None"); + + assert_eq!( + after_keep_table.entries.len(), + baseline_count + 2, + "sources: None must keep the 2 specific entries" + ); + // Cleanup in correct order: external first, then internal cleanup_test_group(switch, external_group_ip, TEST_TAG).await.unwrap(); cleanup_test_group(switch, internal_group_ip, TEST_TAG).await diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 71006f3c..3e02cf3f 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -1144,6 +1144,18 @@ pub(crate) fn reset_tag(s: &Switch, tag: &str) -> DpdResult<()> { pub(crate) fn reset(s: &Switch) -> DpdResult<()> { let mut mcast = s.mcast.lock().unwrap(); + // Remove each known group's entries before the table clears below, as + // SoftNPU's `clear` cannot empty its tables. + for group in mcast.groups.values() { + if let Err(e) = delete_group_tables(s, group, &mut Vec::new()) { + error!( + s.log, + "failed to delete table entries for multicast group {}: {e:?}", + group.ip() + ); + } + } + // Destroy ASIC groups let group_ids = s.asic_hdl.mc_domains(); for group_id in group_ids { diff --git a/dpd/src/table/mcast/mod.rs b/dpd/src/table/mcast/mod.rs index 3a34aeb9..c68f5f1b 100644 --- a/dpd/src/table/mcast/mod.rs +++ b/dpd/src/table/mcast/mod.rs @@ -13,12 +13,15 @@ use std::{ net::{Ipv4Addr, Ipv6Addr}, }; -use aal::{ActionParse, MatchParse}; +use aal::{ActionParse, AsicError, MatchParse}; use aal_macros::*; use common::table::TableType; use slog::debug; -use crate::{Switch, types::DpdResult}; +use crate::{ + Switch, + types::{DpdError, DpdResult}, +}; pub(crate) mod mcast_egress; pub(crate) mod mcast_nat; @@ -39,9 +42,19 @@ where M: MatchParse + Hash + fmt::Display, A: ActionParse, { - let keys: Vec = s + let entries = match s .table_get(table)? - .get_entries::(&s.asic_hdl, false)? + .get_entries::(&s.asic_hdl, false) + { + Ok(entries) => entries, + Err(DpdError::Switch(AsicError::OperationUnsupported)) => { + debug!(s.log, "{table} entry listing unsupported, skip clearing"); + return Ok(()); + } + Err(e) => return Err(e), + }; + + let keys: Vec = entries .into_iter() .filter_map(|(key, _)| matches(&key).then_some(key)) .collect(); From 8c742a310eaac6df666c807f901d96bdf8e1c140 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 19 Oct 2026 03:42:12 +0000 Subject: [PATCH 2/9] [deps] p4(rs) update --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 5fa0d903..10dc9490 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5005,7 +5005,7 @@ dependencies = [ [[package]] name = "p4rs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast#3d7c18bd4d243f19bc1702f20b990dba09c35e95" +source = "git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast#35bca4f7d8ff5ccdb0fe443dc718699f0456153e" dependencies = [ "bitvec", "num", From b52e7687d60934c7319a14b42886df9448989dc9 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 19 Oct 2026 18:03:55 +0000 Subject: [PATCH 3/9] [deps] softnpu and p4(rs) to main --- Cargo.lock | 21 +++++---------------- Cargo.toml | 3 +-- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 10dc9490..684e0ad7 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,7 +188,7 @@ dependencies = [ "slog", "slog-async", "slog-term", - "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=zl%2Fmulticast)", + "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=main)", "strum 0.28.0", "thiserror 2.0.18", "tofino 0.1.0 (git+https://github.com/oxidecomputer/tofino?branch=main)", @@ -4994,18 +4994,7 @@ dependencies = [ [[package]] name = "p4rs" version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p4?branch=main#c13435444832c28e0fb19bc65eaa8b431583a1cf" -dependencies = [ - "bitvec", - "num", - "serde", - "usdt 0.5.0", -] - -[[package]] -name = "p4rs" -version = "0.1.0" -source = "git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast#35bca4f7d8ff5ccdb0fe443dc718699f0456153e" +source = "git+https://github.com/oxidecomputer/p4?branch=main#c8b190f46901d4c1b8eef1ca35be243b18653471" dependencies = [ "bitvec", "num", @@ -7178,9 +7167,9 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu?branch=zl%2Fmulticast#26615d82bdad8dd9928239b88896864f83aab326" +source = "git+https://github.com/oxidecomputer/softnpu?branch=main#e19bb5f09ee8d108bc53b80585238796cd3bf272" dependencies = [ - "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=zl%2Fmulticast)", + "p4rs", "serde", "serde_json", "tokio", @@ -7191,7 +7180,7 @@ name = "softnpu" version = "0.2.0" source = "git+https://github.com/oxidecomputer/softnpu#7e015d167a914777bc21434d1c61f205f22993b1" dependencies = [ - "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=main)", + "p4rs", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 942cddb1..49d0b634 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,8 +57,7 @@ oxnet = { version = "0.1.6", default-features = false, features = ["schemars", " propolis = { git = "https://github.com/oxidecomputer/propolis" } smf = { git = "https://github.com/illumos/smf-rs" } slog-error-chain = { git = "https://github.com/oxidecomputer/slog-error-chain", branch = "main" } -# TODO: set back to main once zl/multicast merges -softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "zl/multicast"} +softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "main"} tofino = { git = "https://github.com/oxidecomputer/tofino", branch = "main" } transceiver-controller = { git = "https://github.com/oxidecomputer/transceiver-control", branch = "main" } From 1188ce3b929f6c895dbabf8cf6c66912e39281ad Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Thu, 29 Oct 2026 07:57:08 +0000 Subject: [PATCH 4/9] [review] address review items --- asic/src/softnpu/mcast.rs | 109 ++++++++++++++++++++------------------ asic/src/softnpu/mod.rs | 5 +- asic/src/softnpu/table.rs | 53 +++++++++++------- 3 files changed, 92 insertions(+), 75 deletions(-) diff --git a/asic/src/softnpu/mcast.rs b/asic/src/softnpu/mcast.rs index b015d88b..b87e20f5 100644 --- a/asic/src/softnpu/mcast.rs +++ b/asic/src/softnpu/mcast.rs @@ -11,7 +11,7 @@ //! //! [Sidecar-lite]: https://github.com/oxidecomputer/sidecar-lite -use std::collections::{BTreeMap, HashMap, HashSet, hash_map::Entry}; +use std::collections::{BTreeMap, HashMap, hash_map::Entry}; use aal::{AsicError, AsicResult}; @@ -64,7 +64,7 @@ pub struct RefreshedReplEntry { /// group's membership change is triggered. #[derive(Default)] pub struct McGroupData { - groups: HashMap>, + groups: HashMap, /// Replication table entries keyed on their matched keyset, recorded at /// table-write time so membership changes can refresh bitmaps. repl_entries: BTreeMap, ReplEntryRef>, @@ -85,17 +85,15 @@ impl McGroupData { /// /// Returns zero for unknown groups. pub fn port_bitmap(&self, group_id: u16) -> u128 { - self.groups.get(&group_id).map_or(0, |ports| { - ports.iter().fold(0, |bitmap, &port| bitmap | (1u128 << port)) - }) + self.groups.get(&group_id).copied().unwrap_or(0) } /// Get the number of ports in a multicast domain. pub fn domain_port_count(&self, group_id: u16) -> AsicResult { - match self.groups.get(&group_id) { - Some(g) => Ok(g.len()), - None => Err(no_group(group_id)), - } + self.groups + .get(&group_id) + .map(|ports| ports.count_ones() as usize) + .ok_or_else(|| no_group(group_id)) } /// Add a port to a multicast domain. Port must be < 128 to fit @@ -107,20 +105,22 @@ impl McGroupData { _rid: u16, _level1_excl_id: u16, ) -> AsicResult<()> { - if port >= 128 { + if u32::from(port) >= u128::BITS { return Err(AsicError::InvalidArg(format!( "port {port} exceeds softnpu 128-port bitmap limit" ))); } - let group = + let ports = self.groups.get_mut(&group_id).ok_or_else(|| no_group(group_id))?; - - group.insert(port).then_some(()).ok_or_else(|| { - AsicError::Exists(format!( + let bit = 1u128 << port; + if *ports & bit != 0 { + return Err(AsicError::Exists(format!( "multicast group {group_id} already contains port {port}" - )) - }) + ))); + } + *ports |= bit; + Ok(()) } /// Remove a port from a multicast domain. @@ -129,14 +129,16 @@ impl McGroupData { group_id: u16, port: u16, ) -> AsicResult<()> { - let group = + let ports = self.groups.get_mut(&group_id).ok_or_else(|| no_group(group_id))?; - - group.remove(&port).then_some(()).ok_or_else(|| { - AsicError::Missing(format!( + let bit = 1u128.checked_shl(u32::from(port)).unwrap_or(0); + if *ports & bit == 0 { + return Err(AsicError::Missing(format!( "multicast group {group_id} doesn't contain port {port}" - )) - }) + ))); + } + *ports &= !bit; + Ok(()) } /// Create a multicast domain. @@ -146,7 +148,7 @@ impl McGroupData { "multicast group {group_id} already exists" ))), Entry::Vacant(vacant) => { - vacant.insert(HashSet::new()); + vacant.insert(0); Ok(()) } } @@ -185,9 +187,13 @@ impl McGroupData { self.repl_entries.remove(keyset); } - /// Forget every tracked replication entry when the replication table is - /// cleared, so a later membership change for a reused group identifier - /// cannot resurrect a stale bitmap from a forgotten keyset. + /// Iterate over the keysets of every tracked replication entry. + pub fn repl_keysets(&self) -> impl Iterator + '_ { + self.repl_entries.keys().map(Vec::as_slice) + } + + /// Forget every tracked replication entry once its pipeline entry has + /// been removed. pub fn clear_repl_entries(&mut self) { self.repl_entries.clear(); } @@ -199,10 +205,10 @@ impl McGroupData { pub fn refresh_params_for_group( &self, group_id: u16, - ) -> Vec { + ) -> impl Iterator + '_ { self.repl_entries .iter() - .filter(|(_, e)| { + .filter(move |(_, e)| { e.external_grp == group_id || e.underlay_grp == group_id }) .map(|(keyset, e)| RefreshedReplEntry { @@ -211,17 +217,12 @@ impl McGroupData { underlay_bitmap: self.port_bitmap(e.underlay_grp), rid: e.rid, }) - .collect() } /// Validate that the current group count does not exceed the limit. - pub fn set_max_nodes( - &mut self, - max_nodes: u32, - _max_link_aggregated_nodes: u32, - ) -> AsicResult<()> { + pub fn set_max_nodes(&self, max_nodes: u32) -> AsicResult<()> { let total = self.domains_count(); - if u64::try_from(total).unwrap_or(u64::MAX) > u64::from(max_nodes) { + if total > usize::try_from(max_nodes).unwrap_or(usize::MAX) { return Err(AsicError::InvalidArg(format!( "number of multicast groups {total} exceeds max nodes {max_nodes}" ))); @@ -357,11 +358,11 @@ mod tests { group_data.domain_create(2).unwrap(); assert!(matches!( - group_data.set_max_nodes(1, 0), + group_data.set_max_nodes(1), Err(AsicError::InvalidArg(_)) )); - assert!(group_data.set_max_nodes(2, 0).is_ok()); - assert!(group_data.set_max_nodes(100, 0).is_ok()); + assert!(group_data.set_max_nodes(2).is_ok()); + assert!(group_data.set_max_nodes(100).is_ok()); } #[test] @@ -379,14 +380,16 @@ mod tests { group_data.record_repl_entry(keyset.clone(), 0, 65533, 65533); // Refreshed against current membership @ only port 1. - let initial = group_data.refresh_params_for_group(65533); + let initial: Vec<_> = + group_data.refresh_params_for_group(65533).collect(); assert_eq!(initial.len(), 1); assert_eq!(initial[0].underlay_bitmap, 1u128 << 1); // A later member add must surface in a fresh refresh. group_data.domain_port_add(65533, 2, 0, 0).unwrap(); group_data.domain_port_add(65533, 3, 0, 0).unwrap(); - let refreshed = group_data.refresh_params_for_group(65533); + let refreshed: Vec<_> = + group_data.refresh_params_for_group(65533).collect(); assert_eq!(refreshed.len(), 1); assert_eq!(refreshed[0].keyset, keyset); assert_eq!(refreshed[0].rid, 65533); @@ -412,13 +415,14 @@ mod tests { group_data.record_repl_entry(key_b, 0, 200, 8); // Only the entry referencing group 100 is refreshed. - let refreshed = group_data.refresh_params_for_group(100); + let refreshed: Vec<_> = + group_data.refresh_params_for_group(100).collect(); assert_eq!(refreshed.len(), 1); assert_eq!(refreshed[0].keyset, key_a); // After forgetting it, the group has no entries to refresh. group_data.forget_repl_entry(&key_a); - assert!(group_data.refresh_params_for_group(100).is_empty()); + assert!(group_data.refresh_params_for_group(100).next().is_none()); } #[test] @@ -429,14 +433,15 @@ mod tests { // Group 50 sits in the external slot here, not the underlay slot. group_data.record_repl_entry(vec![0xcc], 50, 0, 1); - let refreshed = group_data.refresh_params_for_group(50); + let refreshed: Vec<_> = + group_data.refresh_params_for_group(50).collect(); assert_eq!(refreshed.len(), 1); assert_eq!(refreshed[0].external_bitmap, 1u128 << 9); assert_eq!(refreshed[0].underlay_bitmap, 0); } #[test] - fn clear_repl_entries_drops_all_tracked_entries() { + fn clear_repl_entries_after_listing_keysets() { let mut group_data = McGroupData::default(); group_data.domain_create(100).unwrap(); group_data.domain_create(200).unwrap(); @@ -445,16 +450,15 @@ mod tests { group_data.record_repl_entry(vec![0xaa], 0, 100, 7); group_data.record_repl_entry(vec![0xbb], 0, 200, 8); - // Both entries are tracked before the clear. - assert_eq!(group_data.refresh_params_for_group(100).len(), 1); - assert_eq!(group_data.refresh_params_for_group(200).len(), 1); + let keysets: Vec<_> = + group_data.repl_keysets().map(<[u8]>::to_vec).collect(); + assert_eq!(keysets, vec![vec![0xaa], vec![0xbb]]); group_data.clear_repl_entries(); - // After clearing, no keyset survives to be resurrected, even though the - // group memberships themselves are untouched. - assert!(group_data.refresh_params_for_group(100).is_empty()); - assert!(group_data.refresh_params_for_group(200).is_empty()); + assert_eq!(group_data.repl_keysets().count(), 0); + assert!(group_data.refresh_params_for_group(100).next().is_none()); + assert!(group_data.refresh_params_for_group(200).next().is_none()); assert_eq!(group_data.domain_port_count(100).unwrap(), 1); assert_eq!(group_data.domain_port_count(200).unwrap(), 1); } @@ -470,7 +474,8 @@ mod tests { group_data.domain_destroy(1).unwrap(); - let refreshed = group_data.refresh_params_for_group(2); + let refreshed: Vec<_> = + group_data.refresh_params_for_group(2).collect(); assert_eq!(refreshed.len(), 1); assert_eq!(refreshed[0].keyset, vec![0xbb]); } diff --git a/asic/src/softnpu/mod.rs b/asic/src/softnpu/mod.rs index 619edebb..337a5655 100644 --- a/asic/src/softnpu/mod.rs +++ b/asic/src/softnpu/mod.rs @@ -152,7 +152,7 @@ impl Handle { /// the recomputed bitmaps. fn rewrite_replication_entries( &self, - entries: Vec, + entries: impl IntoIterator, ) { use softnpu_lib::{TableAdd, TableRemove}; @@ -266,8 +266,7 @@ impl AsicMulticastOps for Handle { "setting max nodes to {max_nodes}, \ max link aggregated nodes to {max_link_aggregated_nodes}" ); - let mut mc_data = self.mc_data.lock().unwrap(); - mc_data.set_max_nodes(max_nodes, max_link_aggregated_nodes) + self.mc_data.lock().unwrap().set_max_nodes(max_nodes) } } diff --git a/asic/src/softnpu/table.rs b/asic/src/softnpu/table.rs index f3a1f134..ef6eb738 100644 --- a/asic/src/softnpu/table.rs +++ b/asic/src/softnpu/table.rs @@ -139,7 +139,7 @@ impl Table { trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); trace!(hdl.log, "parameter_data:\n{parameter_data:#?}"); - replace_entry( + add_entry( hdl, table, crate::softnpu::mcast::SET_PORT_BITMAP_ACTION.to_string(), @@ -151,19 +151,13 @@ impl Table { } } -fn replace_entry( +fn add_entry( hdl: &Handle, table: String, action: String, keyset_data: Vec, parameter_data: Vec, ) { - let remove = ManagementRequest::TableRemove(TableRemove { - table: table.clone(), - keyset_data: keyset_data.clone(), - }); - crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); - let add = ManagementRequest::TableAdd(TableAdd { table, action, @@ -216,12 +210,31 @@ impl TableOps for Table { } fn clear(&self, hdl: &Handle) -> AsicResult<()> { - // TODO: the management protocol has no clear message, meaning - // that entries stay in the pipeline. Send a `TableClear` once - // softnpu and propolis support one. - if self.type_ == TableType::McastIpv6 { - hdl.mc_data.lock().unwrap().clear_repl_entries(); + // TODO: the management protocol has no clear message (yet). + // + // For now, only the multicast replication entries tracked in `mc_data` + // get removed here, one `TableRemove` per keyset, and every other + // table retains its entries. + // + // Send a `TableClear` once softnpu and propolis support one (the + // pipeline side landed in oxidecomputer/p4#278). + if self.type_ != TableType::McastIpv6 { + return Ok(()); + } + + let Some(table) = self.softnpu_table_name() else { + return Ok(()); + }; + + let mut mc_data = hdl.mc_data.lock().unwrap(); + for keyset_data in mc_data.repl_keysets() { + let remove = ManagementRequest::TableRemove(TableRemove { + table: table.to_string(), + keyset_data: keyset_data.to_vec(), + }); + crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); } + mc_data.clear_repl_entries(); Ok(()) } @@ -622,7 +635,7 @@ impl TableOps for Table { ("rewrite_src_mac", params) } (TableType::McastEgressPortMapping, "set_port_number") => { - ("set_port_number", port_number_param(&action_data)?) + ("set_port_number", vec![port_number_param(&action_data)?]) } // Sidecar-lite's resolvers support drop (by default). (TableType::ArpIpv4, "drop") @@ -645,7 +658,7 @@ impl TableOps for Table { trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); trace!(hdl.log, "parameter_data:\n{:#?}", parameter_data); - replace_entry(hdl, table, action, keyset_data, parameter_data); + add_entry(hdl, table, action, keyset_data, parameter_data); Ok(()) } @@ -656,12 +669,13 @@ impl TableOps for Table { key: &M, data: &A, ) -> AsicResult<()> { - // SoftNPU does not currently support in-place updates. `entry_add` - // removes the key before adding it back with the new action data. + // SoftNPU does not currently support in-place updates. The key is + // removed via `entry_del` and added back with the new action data. // // Note: this is not an atomic op, as a lookup between the two messages // could miss the entry, and mgmt IO failures panic in mgmt::write // rather than surface errors. + self.entry_del(hdl, key)?; self.entry_add(hdl, key, data) } @@ -845,12 +859,11 @@ fn pack_decap_bitmap(args: &aal::ActionData) -> Vec { chunks.iter().flat_map(|c| c.to_le_bytes()).collect() } -fn port_number_param(args: &aal::ActionData) -> AsicResult> { +fn port_number_param(args: &aal::ActionData) -> AsicResult { args.args .iter() .find(|arg| arg.name == "port_number") .and_then(|arg| u8::try_from(&arg.value).ok()) - .map(|port_number| vec![port_number]) .ok_or_else(|| { AsicError::InvalidArg(format!( "{}: missing or invalid port_number", @@ -1045,7 +1058,7 @@ mod tests { value: ValueTypes::U64(17), }], }; - assert_eq!(port_number_param(&action_data).unwrap(), vec![17]); + assert_eq!(port_number_param(&action_data).unwrap(), 17); let missing = aal::ActionData { action: "set_port_number".to_string(), From 563cb147e03fed8ed1ec1233c2b0050b1547b011 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 30 Oct 2026 16:08:44 +0000 Subject: [PATCH 5/9] [review, softnpu] replace entry on add and don't drop on a dup add --- asic/src/softnpu/table.rs | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/asic/src/softnpu/table.rs b/asic/src/softnpu/table.rs index ef6eb738..7f16a9ae 100644 --- a/asic/src/softnpu/table.rs +++ b/asic/src/softnpu/table.rs @@ -139,7 +139,7 @@ impl Table { trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); trace!(hdl.log, "parameter_data:\n{parameter_data:#?}"); - add_entry( + replace_entry( hdl, table, crate::softnpu::mcast::SET_PORT_BITMAP_ACTION.to_string(), @@ -151,13 +151,27 @@ impl Table { } } -fn add_entry( +/// Write an entry, or replace any entry already associated with the key. +/// +/// Note: SoftNPU's `TableAdd` retrains an existing entry in place and reports +/// nothing back. The backend key lookup does not return `AsicError::Exists`, +/// for example. +/// +/// Removing the key before the add ensures that the pipeline is in agreement +/// with what DPD wrote. +fn replace_entry( hdl: &Handle, table: String, action: String, keyset_data: Vec, parameter_data: Vec, ) { + let remove = ManagementRequest::TableRemove(TableRemove { + table: table.clone(), + keyset_data: keyset_data.clone(), + }); + crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); + let add = ManagementRequest::TableAdd(TableAdd { table, action, @@ -658,7 +672,7 @@ impl TableOps for Table { trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); trace!(hdl.log, "parameter_data:\n{:#?}", parameter_data); - add_entry(hdl, table, action, keyset_data, parameter_data); + replace_entry(hdl, table, action, keyset_data, parameter_data); Ok(()) } @@ -669,13 +683,12 @@ impl TableOps for Table { key: &M, data: &A, ) -> AsicResult<()> { - // SoftNPU does not currently support in-place updates. The key is - // removed via `entry_del` and added back with the new action data. + // SoftNPU does not currently support in-place updates. `entry_add` + // removes the key first before adding it back with the new action data. // // Note: this is not an atomic op, as a lookup between the two messages // could miss the entry, and mgmt IO failures panic in mgmt::write // rather than surface errors. - self.entry_del(hdl, key)?; self.entry_add(hdl, key, data) } From eb55743d1fab425f74d53774a0ecf8bf2ab5b6c5 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Thu, 5 Nov 2026 11:26:20 +0000 Subject: [PATCH 6/9] [softnpu, mcast] clear known src filters on update/delete + sweep after This also replaces ``make_tag(TEST_TAG)` w/ `TEST_TAG`` in two dpd-client mcast tests. --- asic/src/chaos/mod.rs | 3 + asic/src/chaos/table.rs | 5 +- dpd-client/tests/integration_tests/mcast.rs | 4 +- dpd/src/mcast/mod.rs | 43 ++++--- dpd/src/mcast/rollback.rs | 123 ++++++++++++++++++++ 5 files changed, 159 insertions(+), 19 deletions(-) diff --git a/asic/src/chaos/mod.rs b/asic/src/chaos/mod.rs index 3df78875..aabdb348 100644 --- a/asic/src/chaos/mod.rs +++ b/asic/src/chaos/mod.rs @@ -274,6 +274,8 @@ pub struct AsicConfig { pub table_entry_update: TableChaos, pub table_entry_del: TableChaos, pub mc_config: AsicMulticastConfig, + #[serde(default)] + pub entries_unsupported: BTreeSet, } impl AsicConfig { @@ -318,6 +320,7 @@ impl AsicConfig { faults: BTreeMap::new(), unsupported: BTreeSet::new(), }, + entries_unsupported: BTreeSet::new(), } } diff --git a/asic/src/chaos/table.rs b/asic/src/chaos/table.rs index 6083dc1e..d5f6d0f8 100644 --- a/asic/src/chaos/table.rs +++ b/asic/src/chaos/table.rs @@ -112,9 +112,12 @@ impl TableOps for Table { fn get_entries( &self, - _hdl: &Handle, + hdl: &Handle, _from_hardware: bool, ) -> AsicResult> { + if hdl.config.entries_unsupported.contains(&self.type_) { + return Err(AsicError::OperationUnsupported); + } self.keys .lock() .unwrap() diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 8023a3f1..7abe0b72 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -7619,7 +7619,7 @@ async fn test_source_filter_update_to_any() -> TestResult { .client .multicast_group_update_external( &external_group_ip.try_into().unwrap(), - &make_tag(TEST_TAG), + &TEST_TAG, &narrow_entry, ) .await @@ -7656,7 +7656,7 @@ async fn test_source_filter_update_to_any() -> TestResult { .client .multicast_group_update_external( &external_group_ip.try_into().unwrap(), - &make_tag(TEST_TAG), + &TEST_TAG, &keep_entry, ) .await diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index dc9d4705..0d6d5c5d 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -1827,13 +1827,29 @@ fn clear_source_filter( SourceFilterKey::new(group_ip, source).and_then(|key| key.clear(s)) } -/// Remove ASIC source-filter entries for `group_ip` that are not in -/// `sources`. +/// Remove ASIC source-filter entries for `group` that are not in `sources`. +/// +/// The group's stored sources missing from what `sources` contains are cleared +/// first, then any other entry for the group address is swept from what the +/// table lists. +/// +/// Backends that don't list entries (e.g., SoftNPU) skip the periodic sweep. fn remove_unwanted_source_filters( s: &Switch, - group_ip: IpAddr, + group: &MulticastGroup, sources: &SourceFilter, ) -> DpdResult<()> { + let group_ip = group.ip(); + if let GroupKind::External { sources: old_sources, .. } = &group.kind { + apply_each( + old_sources + .iter() + .filter(|old| !sources.iter().any(|new| new == *old)), + |source| clear_source_filter(s, group_ip, source), + ) + .map_err(|failures| failures.head.1)?; + } + let mut ipv4_sources = BTreeSet::new(); let mut ipv6_sources = BTreeSet::new(); for source in sources.iter() { @@ -1848,17 +1864,17 @@ fn remove_unwanted_source_filters( } match group_ip { - IpAddr::V4(group) => { + IpAddr::V4(ipv4) => { table::mcast::mcast_src_filter::retain_ipv4_sources( s, - group, + ipv4, &ipv4_sources, ) } - IpAddr::V6(group) => { + IpAddr::V6(ipv6) => { table::mcast::mcast_src_filter::retain_ipv6_sources( s, - group, + ipv6, &ipv6_sources, ) } @@ -2352,7 +2368,7 @@ fn update_external_tables( set_source_filter(s, group_ip, source) }) .map_err(|failures| failures.head.1)?; - remove_unwanted_source_filters(s, group_ip, new_sources)?; + remove_unwanted_source_filters(s, group_entry, new_sources)?; let new_vlan_id = new_group_info.external_forwarding.vlan_id; @@ -2474,20 +2490,15 @@ fn delete_group_tables( match &group.kind { GroupKind::External { sources, .. } => { deleted.push(DeleteStep::SourceFilters); - let removed = match group_ip { + remove_source_filters(s, group_ip, sources)?; + match group_ip { IpAddr::V4(ipv4) => { table::mcast::mcast_src_filter::del_ipv4_entries(s, ipv4) } IpAddr::V6(ipv6) => { table::mcast::mcast_src_filter::del_ipv6_entries(s, ipv6) } - }; - match removed { - Err(DpdError::Switch(AsicError::OperationUnsupported)) => { - remove_source_filters(s, group_ip, sources)? - } - res => res?, - } + }?; match group_ip { IpAddr::V4(ipv4) => { table::mcast::mcast_nat::clear_ipv4_entry(s, ipv4) diff --git a/dpd/src/mcast/rollback.rs b/dpd/src/mcast/rollback.rs index 0911ebac..953845be 100644 --- a/dpd/src/mcast/rollback.rs +++ b/dpd/src/mcast/rollback.rs @@ -1827,6 +1827,129 @@ mod chaos_tests { } } + const UNLISTED_SOURCE_CASES: [(&str, &str, &str); 2] = [ + ("224.1.2.3", "192.0.2.1", "192.0.2.2"), + ("ff0e::1", "2001:db8::1", "2001:db8::2"), + ]; + + fn unlisted_source_filter_group( + external_ip: IpAddr, + tag: &str, + source_ips: &[IpAddr], + ) -> (Switch, NatTarget) { + let (_, source_table, _) = external_tables(external_ip); + let mut config = Config::default(); + config.asic_config.entries_unsupported.insert(source_table); + let (switch, _logs) = chaos_switch(config); + let (underlay_ip, _) = install_underlay_group(&switch); + let nat_target = external_nat_target(external_ip, underlay_ip); + let sources: Vec = source_ips + .iter() + .map(|source_ip| serde_json::json!({ "Exact": source_ip })) + .collect(); + add_group_external( + &switch, + external_entry( + external_ip, + tag, + nat_target, + None, + serde_json::Value::Array(sources), + ), + ) + .unwrap(); + (switch, nat_target) + } + + fn take_source_filter( + switch: &Switch, + external_ip: IpAddr, + source_ip: IpAddr, + ) -> bool { + let (_, source_table, _) = external_tables(external_ip); + let (_, _, deletes_before, _) = table_usage(switch, source_table); + clear_source_filter(switch, external_ip, IpSrc::Exact(source_ip)) + .unwrap(); + let (_, _, deletes_after, _) = table_usage(switch, source_table); + deletes_after > deletes_before + } + + #[test] + fn test_external_create_with_unlisted_source_filters() { + for (group_ip, first_source, second_source) in UNLISTED_SOURCE_CASES { + let external_ip: IpAddr = group_ip.parse().unwrap(); + let source_ips: [IpAddr; 2] = + [first_source.parse().unwrap(), second_source.parse().unwrap()]; + let (switch, _) = unlisted_source_filter_group( + external_ip, + "external-unlisted-create-test", + &source_ips, + ); + + for source_ip in source_ips { + assert!( + take_source_filter(&switch, external_ip, source_ip), + "{source_ip} -> {external_ip}" + ); + } + } + } + + #[test] + fn test_external_update_removes_unlisted_source_filters() { + for (group_ip, kept_source, removed_source) in UNLISTED_SOURCE_CASES { + let external_ip: IpAddr = group_ip.parse().unwrap(); + let kept_source: IpAddr = kept_source.parse().unwrap(); + let removed_source: IpAddr = removed_source.parse().unwrap(); + let tag = "external-unlisted-update-test"; + let (switch, nat_target) = unlisted_source_filter_group( + external_ip, + tag, + &[kept_source, removed_source], + ); + + modify_group_external( + &switch, + ExternalMulticastIp::new(external_ip).unwrap(), + tag, + MulticastGroupUpdateExternalEntry { + internal_forwarding: ExternalInternalForwarding { + nat_target: nat_target.try_into().unwrap(), + }, + external_forwarding: ExternalForwarding { vlan_id: None }, + sources: Some(vec![SourceEntry::Exact( + ExactSource::new(kept_source).unwrap(), + )]), + }, + ) + .unwrap(); + + assert!(!take_source_filter(&switch, external_ip, removed_source)); + assert!(take_source_filter(&switch, external_ip, kept_source)); + } + } + + #[test] + fn test_external_delete_removes_unlisted_source_filters() { + for (group_ip, first_source, second_source) in UNLISTED_SOURCE_CASES { + let external_ip: IpAddr = group_ip.parse().unwrap(); + let source_ips: [IpAddr; 2] = + [first_source.parse().unwrap(), second_source.parse().unwrap()]; + let tag = "external-unlisted-delete-test"; + let (switch, _) = + unlisted_source_filter_group(external_ip, tag, &source_ips); + + del_group(&switch, external_ip, tag).unwrap(); + + for source_ip in source_ips { + assert!( + !take_source_filter(&switch, external_ip, source_ip), + "{source_ip} -> {external_ip}" + ); + } + } + } + #[test] fn test_external_delete_vlan_cleanup_failure_marks_group() { for group_ip in ["224.1.2.3", "ff0e::1"] { From 795b42279b3ac5cbf4b7801abce70a10c941c179 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 9 Nov 2026 03:47:49 +0000 Subject: [PATCH 7/9] [rollback, repair] preserve decap coverage during cleanup and repair On a PUT to an mcast group awaiting repair, we now rewrite the egress decap bitmap from its stored membership before modifying membership (in the case that rollback may have dropped it). Underlay cleanup now deletes the replication entry first and then maintains the decap bitmap if the delete fails. Decapsulation holds a superset of all the ports replication can still send to. --- dpd/src/mcast/mod.rs | 11 +++++++++-- dpd/src/mcast/rollback.rs | 30 +++++++++++++++++++----------- 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 34309043..3682a318 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -1167,7 +1167,8 @@ pub(crate) fn modify_group_underlay( } // Repair the route before changing memberships or replication. - if mcast.pending_repairs.contains(&group_key) { + let repair = mcast.pending_repairs.contains(&group_key); + if repair { table::mcast::mcast_route::set_ipv6_entry(s, group_ip.into(), None)?; mc_group_set(s, existing_group.external_group_id())?; mc_group_set(s, existing_group.underlay_group_id())?; @@ -1180,7 +1181,13 @@ pub(crate) fn modify_group_underlay( }); let widened = (!members.is_empty()) - .then(|| original.widened_external_bitmap(&members)) + .then(|| { + original.widened_external_bitmap(&members).or_else(|| { + repair.then(|| { + create_port_bitmap(original.members(), Direction::External) + }) + }) + }) .flatten(); let mut added_members = Vec::new(); let mut removed_members = Vec::new(); diff --git a/dpd/src/mcast/rollback.rs b/dpd/src/mcast/rollback.rs index 10e9d019..a4abc7aa 100644 --- a/dpd/src/mcast/rollback.rs +++ b/dpd/src/mcast/rollback.rs @@ -407,7 +407,23 @@ impl<'a> GroupCreateRollbackContext<'a> { return Ok(()); }; - unwind_outcome([ + self.log_rollback_error( + "delete IPv6 replication entry", + &format!("for group {ipv6}"), + table::mcast::mcast_replication::clear_ipv6_entry( + self.switch, + ipv6, + ), + ) + .inspect_err(|_| { + warn!( + self.switch.log, + "replication entry delete failed, skipping decap bitmap delete"; + "group" => %ipv6, + "external_group_id" => %self.external_id, + ); + }) + .and_then(|_| { self.log_rollback_error( "delete IPv6 egress bitmap entry", &format!("for external group {}", self.external_id), @@ -415,16 +431,8 @@ impl<'a> GroupCreateRollbackContext<'a> { self.switch, self.external_id, ), - ), - self.log_rollback_error( - "delete IPv6 replication entry", - &format!("for group {ipv6}"), - table::mcast::mcast_replication::clear_ipv6_entry( - self.switch, - ipv6, - ), - ), - ]) + ) + }) } fn remove_external_entries(&self) -> DpdResult<()> { From c6392f8ae20f70652b01423817edae50ff6f2267 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sat, 7 Nov 2026 02:00:34 +0000 Subject: [PATCH 8/9] [review] address feedback + table upserts & clears Note on *deps*: - softnpu -> zl/table-clear (oxidecomputer/softnpu#234) - p4 -> zl/upsert-table-entries (oxidecomputer/p4#282) --- Cargo.lock | 19 +- Cargo.toml | 2 +- asic/src/chaos/fault.rs | 40 +- asic/src/chaos/mod.rs | 12 +- asic/src/chaos/table.rs | 4 +- asic/src/softnpu/mcast.rs | 259 +----- asic/src/softnpu/mgmt.rs | 32 +- asic/src/softnpu/mod.rs | 97 +-- asic/src/softnpu/table.rs | 155 ++-- dpd-client/tests/integration_tests/mcast.rs | 62 -- dpd/src/mcast/mod.rs | 240 ++++-- dpd/src/mcast/rollback.rs | 885 ++++++++++++++------ dpd/src/table/mcast/mod.rs | 29 +- 13 files changed, 1000 insertions(+), 836 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a9144a22..a7353abf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -188,7 +188,7 @@ dependencies = [ "slog", "slog-async", "slog-term", - "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=main)", + "softnpu 0.2.0 (git+https://github.com/oxidecomputer/softnpu?branch=zl%2Ftable-clear)", "strum 0.28.0", "thiserror 2.0.18", "tofino 0.1.0 (git+https://github.com/oxidecomputer/tofino?branch=main)", @@ -5069,6 +5069,17 @@ dependencies = [ "usdt 0.5.0", ] +[[package]] +name = "p4rs" +version = "0.1.0" +source = "git+https://github.com/oxidecomputer/p4?branch=zl%2Fupsert-table-entries#b9baa79b4dc277501f860325da34403b89fb4ee9" +dependencies = [ + "bitvec", + "num", + "serde", + "usdt 0.5.0", +] + [[package]] name = "p9ds" version = "0.1.0" @@ -7291,9 +7302,9 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu?branch=main#e19bb5f09ee8d108bc53b80585238796cd3bf272" +source = "git+https://github.com/oxidecomputer/softnpu?branch=zl%2Ftable-clear#618a149ff169ed8a9c1620a3b1a2410f5cf5d53b" dependencies = [ - "p4rs", + "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=zl%2Fupsert-table-entries)", "serde", "serde_json", "tokio", @@ -7304,7 +7315,7 @@ name = "softnpu" version = "0.2.0" source = "git+https://github.com/oxidecomputer/softnpu#7e015d167a914777bc21434d1c61f205f22993b1" dependencies = [ - "p4rs", + "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=main)", "serde", "serde_json", "tokio", diff --git a/Cargo.toml b/Cargo.toml index 1dbe3ba6..28cc77c8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,7 @@ oxnet = { version = "0.1.6", default-features = false, features = ["schemars", " propolis = { git = "https://github.com/oxidecomputer/propolis" } smf = { git = "https://github.com/illumos/smf-rs" } slog-error-chain = { git = "https://github.com/oxidecomputer/slog-error-chain", branch = "main" } -softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "main"} +softnpu-lib = { git = "https://github.com/oxidecomputer/softnpu" , package = "softnpu" , branch = "zl/table-clear"} tofino = { git = "https://github.com/oxidecomputer/tofino", branch = "main" } transceiver-controller = { git = "https://github.com/oxidecomputer/transceiver-control", branch = "main" } diff --git a/asic/src/chaos/fault.rs b/asic/src/chaos/fault.rs index 99a15b8d..b568a252 100644 --- a/asic/src/chaos/fault.rs +++ b/asic/src/chaos/fault.rs @@ -46,31 +46,32 @@ pub(super) struct Fault { calls: u64, /// The call numbers that fail. range: CallRange, + /// The error returned by a failing execution. + err: fn(String) -> AsicError, } impl Fault { pub(super) fn new(range: CallRange) -> Self { - Self { calls: 0, range } + Self { calls: 0, range, err: AsicError::Synthetic } } } impl Handle { pub(super) fn check_mc_fault(&self, op: McOp) -> AsicResult<()> { - if self.config.mc_config.unsupported.contains(&op) { - return Err(AsicError::OperationUnsupported); - } - let failed_call = { let mut faults = self.mc_faults.lock().unwrap(); faults.get_mut(&op).and_then(|fault| { fault.calls += 1; - fault.range.contains(fault.calls).then_some(fault.calls) + fault + .range + .contains(fault.calls) + .then_some((fault.calls, fault.err)) }) }; - if let Some(call) = failed_call { - slog::error!(self.log, "chaos error: {} (call {})", op, call); - return Err(AsicError::Synthetic(op.to_string())); + if let Some((call, err)) = failed_call { + slog::error!(self.log, "chaos error: {op} (call {call})"); + return Err(err(op.to_string())); } self.config.mc_config.chaos(op).unfurled(&self.log, &op.to_string()) @@ -93,20 +94,20 @@ impl Handle { let mut faults = self.table_faults.lock().unwrap(); faults.get_mut(&(table, op)).and_then(|fault| { fault.calls += 1; - fault.range.contains(fault.calls).then_some(fault.calls) + fault + .range + .contains(fault.calls) + .then_some((fault.calls, fault.err)) }) }; match failed_call { - Some(call) => { + Some((call, err)) => { slog::error!( self.log, - "chaos table error: {} on {} (call {})", - op, - table, - call + "chaos table error: {op} on {table} (call {call})" ); - Err(AsicError::Synthetic(op.to_string())) + Err(err(op.to_string())) } None => Ok(()), } @@ -117,11 +118,10 @@ impl Handle { table: TableType, op: TableOp, range: CallRange, + error: fn(String) -> AsicError, ) { - self.table_faults - .lock() - .unwrap() - .insert((table, op), Fault::new(range)); + let fault = Fault { err: error, ..Fault::new(range) }; + self.table_faults.lock().unwrap().insert((table, op), fault); } pub fn clear_table_faults(&self, table: TableType, op: TableOp) { diff --git a/asic/src/chaos/mod.rs b/asic/src/chaos/mod.rs index aabdb348..2590bd12 100644 --- a/asic/src/chaos/mod.rs +++ b/asic/src/chaos/mod.rs @@ -63,7 +63,7 @@ impl Chaos { /// Return a chaos error according to the underlying probability value. pub fn unfurled(&self, log: &Logger, message: &str) -> AsicResult<()> { if self.value > random() { - slog::error!(log, "chaos error: {}", message); + slog::error!(log, "chaos error: {message}"); return Err(AsicError::Synthetic(message.into())); } Ok(()) @@ -150,7 +150,7 @@ impl TableChaos { if let Some(value) = self.values.get(&id) && *value > random() { - slog::error!(log, "chaos table error: {}", message); + slog::error!(log, "chaos table error: {message}"); return Err(AsicError::Synthetic(message.into())); } Ok(()) @@ -201,6 +201,7 @@ pub enum TableOp { EntryAdd, EntryUpdate, EntryDel, + EntryList, } impl fmt::Display for TableOp { @@ -209,6 +210,7 @@ impl fmt::Display for TableOp { Self::EntryAdd => "table_entry_add", Self::EntryUpdate => "table_entry_update", Self::EntryDel => "table_entry_del", + Self::EntryList => "table_entry_list", }) } } @@ -226,8 +228,6 @@ pub struct AsicMulticastConfig { pub mc_port_list: Chaos, #[serde(default)] pub faults: BTreeMap, - #[serde(default)] - pub unsupported: BTreeSet, } impl AsicMulticastConfig { @@ -274,8 +274,6 @@ pub struct AsicConfig { pub table_entry_update: TableChaos, pub table_entry_del: TableChaos, pub mc_config: AsicMulticastConfig, - #[serde(default)] - pub entries_unsupported: BTreeSet, } impl AsicConfig { @@ -318,9 +316,7 @@ impl AsicConfig { mc_set_max_nodes: Chaos::new(v), mc_port_list: Chaos::new(v), faults: BTreeMap::new(), - unsupported: BTreeSet::new(), }, - entries_unsupported: BTreeSet::new(), } } diff --git a/asic/src/chaos/table.rs b/asic/src/chaos/table.rs index d5f6d0f8..510a89b6 100644 --- a/asic/src/chaos/table.rs +++ b/asic/src/chaos/table.rs @@ -115,9 +115,7 @@ impl TableOps for Table { hdl: &Handle, _from_hardware: bool, ) -> AsicResult> { - if hdl.config.entries_unsupported.contains(&self.type_) { - return Err(AsicError::OperationUnsupported); - } + hdl.check_table_fault(self.type_, TableOp::EntryList)?; self.keys .lock() .unwrap() diff --git a/asic/src/softnpu/mcast.rs b/asic/src/softnpu/mcast.rs index b87e20f5..ddc2747d 100644 --- a/asic/src/softnpu/mcast.rs +++ b/asic/src/softnpu/mcast.rs @@ -4,14 +4,13 @@ // // Copyright 2026 Oxide Computer Company -//! [Sidecar-lite] handles packet replication via port bitmaps in the P4 +//! [sidecar-lite] handles packet replication via port bitmaps in the P4 //! pipeline. This module tracks group membership for the `AsicMulticastOps` -//! contract and caches the installed replication entries for replaying -//! membership updates into sidecar-lite. +//! contract. //! -//! [Sidecar-lite]: https://github.com/oxidecomputer/sidecar-lite +//! [sidecar-lite]: https://github.com/oxidecomputer/sidecar-lite -use std::collections::{BTreeMap, HashMap, hash_map::Entry}; +use std::collections::{HashMap, hash_map::Entry}; use aal::{AsicError, AsicResult}; @@ -24,50 +23,18 @@ pub const MCAST_REPLICATION_V6_TABLE: &str = /// (and replication id) for a [`MCAST_REPLICATION_V6_TABLE`] entry. pub const SET_PORT_BITMAP_ACTION: &str = "set_port_bitmap"; -/// A replication table entry's group references, tracked for later -/// membership changes to recompute port bitmaps when necessary. -/// -/// On the production sidecar, the replication table holds a group identifier, -/// while the replication engine tracks live membership subscriptions, which means -/// that any port additions do not trigger table rewrites. -/// -/// The SoftNPU pipeline, however, is built around a generic port bitmap -/// structure, appended into the table entry at write time, forcing each -/// membership change to rewrite each entry that references the mutated -/// group. -#[derive(Clone, Copy, Debug)] -struct ReplEntryRef { - external_grp: u16, - underlay_grp: u16, - rid: u16, -} - -/// A replication entry refreshed against current membership, available to be -/// rewritten into the SoftNPU pipeline. -#[derive(Clone, Debug)] -pub struct RefreshedReplEntry { - /// Match keyset bytes identifying the table entry (the underlay group's - /// destination address). - pub keyset: Vec, - /// Ports in the external replication group, as a port bitmap. - pub external_bitmap: u128, - /// Ports in the underlay replication group, as a port bitmap. - pub underlay_bitmap: u128, - /// Replication ID, which keys the egress decap-ports table. - pub rid: u16, -} +/// The number of ports a group's bitmap can contain, matching sidecar-lite's +/// `bit<128>` port bitmaps. +const PORT_BITMAP_WIDTH: u16 = u128::BITS as u16; -/// Multicast group membership and the replication entries derived from it. +/// Multicast group membership, defined as a port bitmap per group. /// -/// Unlike Tofino, SoftNPU has no PRE, leading us to track group membership -/// here and rewrite the mutated replication entries' port bitmap when a -/// group's membership change is triggered. +/// SoftNPU has no replication engine to inherently support group membership, +/// so we track it here instead. The bitmaps get resolved when dpd writes a +/// replication entry, and dpd rewrites that entry after each membership change. #[derive(Default)] pub struct McGroupData { groups: HashMap, - /// Replication table entries keyed on their matched keyset, recorded at - /// table-write time so membership changes can refresh bitmaps. - repl_entries: BTreeMap, ReplEntryRef>, } fn no_group(group_id: u16) -> AsicError { @@ -96,6 +63,15 @@ impl McGroupData { .ok_or_else(|| no_group(group_id)) } + /// List the ports in a multicast domain (in ascending order). + pub fn domain_port_list(&self, group_id: u16) -> AsicResult> { + let ports = + self.groups.get(&group_id).ok_or_else(|| no_group(group_id))?; + Ok((0..PORT_BITMAP_WIDTH) + .filter(|&port| *ports & (1u128 << port) != 0) + .collect()) + } + /// Add a port to a multicast domain. Port must be < 128 to fit /// within sidecar-lite's 128-bit replication bitmap. pub fn domain_port_add( @@ -105,7 +81,7 @@ impl McGroupData { _rid: u16, _level1_excl_id: u16, ) -> AsicResult<()> { - if u32::from(port) >= u128::BITS { + if port >= PORT_BITMAP_WIDTH { return Err(AsicError::InvalidArg(format!( "port {port} exceeds softnpu 128-port bitmap limit" ))); @@ -157,9 +133,6 @@ impl McGroupData { /// Destroy a multicast domain. pub fn domain_destroy(&mut self, group_id: u16) -> AsicResult<()> { self.groups.remove(&group_id).ok_or_else(|| no_group(group_id))?; - self.repl_entries.retain(|_, entry| { - entry.external_grp != group_id && entry.underlay_grp != group_id - }); Ok(()) } @@ -168,57 +141,6 @@ impl McGroupData { self.groups.len() } - /// Record a replication table entry's group references at (table-)write - /// time so that a later membership mutation can refresh its related - /// bitmaps. - pub fn record_repl_entry( - &mut self, - keyset: Vec, - external_grp: u16, - underlay_grp: u16, - rid: u16, - ) { - self.repl_entries - .insert(keyset, ReplEntryRef { external_grp, underlay_grp, rid }); - } - - /// Forget a replication table entry when it is removed from the pipeline. - pub fn forget_repl_entry(&mut self, keyset: &[u8]) { - self.repl_entries.remove(keyset); - } - - /// Iterate over the keysets of every tracked replication entry. - pub fn repl_keysets(&self) -> impl Iterator + '_ { - self.repl_entries.keys().map(Vec::as_slice) - } - - /// Forget every tracked replication entry once its pipeline entry has - /// been removed. - pub fn clear_repl_entries(&mut self) { - self.repl_entries.clear(); - } - - /// Recompute the bitmaps for every replication entry that references - /// `group_id` in either its external or underlay slot, returning the - /// refreshed [`RefreshedReplEntry`] values for the caller to rewrite into - /// the SoftNPU pipeline. - pub fn refresh_params_for_group( - &self, - group_id: u16, - ) -> impl Iterator + '_ { - self.repl_entries - .iter() - .filter(move |(_, e)| { - e.external_grp == group_id || e.underlay_grp == group_id - }) - .map(|(keyset, e)| RefreshedReplEntry { - keyset: keyset.clone(), - external_bitmap: self.port_bitmap(e.external_grp), - underlay_bitmap: self.port_bitmap(e.underlay_grp), - rid: e.rid, - }) - } - /// Validate that the current group count does not exceed the limit. pub fn set_max_nodes(&self, max_nodes: u32) -> AsicResult<()> { let total = self.domains_count(); @@ -246,11 +168,13 @@ mod tests { group_data.domain_port_add(100, 5, 0, 0).unwrap(); assert_eq!(group_data.domain_port_count(100).unwrap(), 2); + assert_eq!(group_data.domain_port_list(100).unwrap(), vec![1, 5]); assert_eq!(group_data.domains_count(), 1); // Remove a port. group_data.domain_port_remove(100, 1).unwrap(); assert_eq!(group_data.domain_port_count(100).unwrap(), 1); + assert_eq!(group_data.domain_port_list(100).unwrap(), vec![5]); // Destroy group. group_data.domain_destroy(100).unwrap(); @@ -292,19 +216,23 @@ mod tests { fn operations_on_missing_group_rejected() { let mut group_data = McGroupData::default(); assert!(matches!( - group_data.domain_port_add(42, 1, 0, 0), + group_data.domain_port_add(404, 1, 0, 0), + Err(AsicError::Missing(_)) + )); + assert!(matches!( + group_data.domain_port_remove(404, 1), Err(AsicError::Missing(_)) )); assert!(matches!( - group_data.domain_port_remove(42, 1), + group_data.domain_port_count(404), Err(AsicError::Missing(_)) )); assert!(matches!( - group_data.domain_port_count(42), + group_data.domain_port_list(404), Err(AsicError::Missing(_)) )); assert!(matches!( - group_data.domain_destroy(42), + group_data.domain_destroy(404), Err(AsicError::Missing(_)) )); } @@ -340,6 +268,16 @@ mod tests { assert_eq!(bm & (1u128 << 127), 1u128 << 127); } + #[test] + fn port_list_covers_bitmap_range() { + let mut group_data = McGroupData::default(); + group_data.domain_create(1).unwrap(); + assert!(group_data.domain_port_list(1).unwrap().is_empty()); + group_data.domain_port_add(1, 127, 0, 0).unwrap(); + group_data.domain_port_add(1, 0, 0, 0).unwrap(); + assert_eq!(group_data.domain_port_list(1).unwrap(), vec![0, 127]); + } + #[test] fn port_add_rejects_ports_above_127() { let mut group_data = McGroupData::default(); @@ -364,119 +302,4 @@ mod tests { assert!(group_data.set_max_nodes(2).is_ok()); assert!(group_data.set_max_nodes(100).is_ok()); } - - #[test] - fn refresh_recomputes_bitmap_after_membership_change() { - let mut group_data = McGroupData::default(); - // Underlay group with one member, recorded as a replication - // entry keyed by the underlay destination address bytes. - group_data.domain_create(65533).unwrap(); - group_data.domain_port_add(65533, 1, 0, 0).unwrap(); - // ff04::1, an admin-scoped IPv6 multicast destination, matching the - // 128-bit key of the sidecar-lite replication table. - let keyset = std::net::Ipv6Addr::new(0xff04, 0, 0, 0, 0, 0, 0, 1) - .octets() - .to_vec(); - group_data.record_repl_entry(keyset.clone(), 0, 65533, 65533); - - // Refreshed against current membership @ only port 1. - let initial: Vec<_> = - group_data.refresh_params_for_group(65533).collect(); - assert_eq!(initial.len(), 1); - assert_eq!(initial[0].underlay_bitmap, 1u128 << 1); - - // A later member add must surface in a fresh refresh. - group_data.domain_port_add(65533, 2, 0, 0).unwrap(); - group_data.domain_port_add(65533, 3, 0, 0).unwrap(); - let refreshed: Vec<_> = - group_data.refresh_params_for_group(65533).collect(); - assert_eq!(refreshed.len(), 1); - assert_eq!(refreshed[0].keyset, keyset); - assert_eq!(refreshed[0].rid, 65533); - assert_eq!( - refreshed[0].underlay_bitmap, - (1u128 << 1) | (1u128 << 2) | (1u128 << 3) - ); - } - - #[test] - fn refresh_skips_unrelated_groups_and_forgotten_entries() { - let mut group_data = McGroupData::default(); - group_data.domain_create(100).unwrap(); - group_data.domain_create(200).unwrap(); - group_data.domain_port_add(100, 4, 0, 0).unwrap(); - - // These are opaque single-byte keysets standing in for distinct - // replication entries (not any real address). - let key_a = vec![0xaa]; - let key_b = vec![0xbb]; - - group_data.record_repl_entry(key_a.clone(), 0, 100, 7); - group_data.record_repl_entry(key_b, 0, 200, 8); - - // Only the entry referencing group 100 is refreshed. - let refreshed: Vec<_> = - group_data.refresh_params_for_group(100).collect(); - assert_eq!(refreshed.len(), 1); - assert_eq!(refreshed[0].keyset, key_a); - - // After forgetting it, the group has no entries to refresh. - group_data.forget_repl_entry(&key_a); - assert!(group_data.refresh_params_for_group(100).next().is_none()); - } - - #[test] - fn refresh_matches_external_slot() { - let mut group_data = McGroupData::default(); - group_data.domain_create(50).unwrap(); - group_data.domain_port_add(50, 9, 0, 0).unwrap(); - // Group 50 sits in the external slot here, not the underlay slot. - group_data.record_repl_entry(vec![0xcc], 50, 0, 1); - - let refreshed: Vec<_> = - group_data.refresh_params_for_group(50).collect(); - assert_eq!(refreshed.len(), 1); - assert_eq!(refreshed[0].external_bitmap, 1u128 << 9); - assert_eq!(refreshed[0].underlay_bitmap, 0); - } - - #[test] - fn clear_repl_entries_after_listing_keysets() { - let mut group_data = McGroupData::default(); - group_data.domain_create(100).unwrap(); - group_data.domain_create(200).unwrap(); - group_data.domain_port_add(100, 4, 0, 0).unwrap(); - group_data.domain_port_add(200, 5, 0, 0).unwrap(); - group_data.record_repl_entry(vec![0xaa], 0, 100, 7); - group_data.record_repl_entry(vec![0xbb], 0, 200, 8); - - let keysets: Vec<_> = - group_data.repl_keysets().map(<[u8]>::to_vec).collect(); - assert_eq!(keysets, vec![vec![0xaa], vec![0xbb]]); - - group_data.clear_repl_entries(); - - assert_eq!(group_data.repl_keysets().count(), 0); - assert!(group_data.refresh_params_for_group(100).next().is_none()); - assert!(group_data.refresh_params_for_group(200).next().is_none()); - assert_eq!(group_data.domain_port_count(100).unwrap(), 1); - assert_eq!(group_data.domain_port_count(200).unwrap(), 1); - } - - #[test] - fn destroy_group_drops_its_repl_entries() { - let mut group_data = McGroupData::default(); - group_data.domain_create(1).unwrap(); - group_data.domain_create(2).unwrap(); - group_data.domain_port_add(2, 3, 0, 0).unwrap(); - group_data.record_repl_entry(vec![0xaa], 1, 2, 7); - group_data.record_repl_entry(vec![0xbb], 2, 2, 8); - - group_data.domain_destroy(1).unwrap(); - - let refreshed: Vec<_> = - group_data.refresh_params_for_group(2).collect(); - assert_eq!(refreshed.len(), 1); - assert_eq!(refreshed[0].keyset, vec![0xbb]); - } } diff --git a/asic/src/softnpu/mgmt.rs b/asic/src/softnpu/mgmt.rs index 65f24338..a803ea93 100644 --- a/asic/src/softnpu/mgmt.rs +++ b/asic/src/softnpu/mgmt.rs @@ -5,9 +5,11 @@ // Copyright 2026 Oxide Computer Company use std::fs::OpenOptions; +use std::io; use std::io::Read; use std::io::Write; +use aal::{AsicError, AsicResult}; use propolis::hw::virtio::softnpu::{MANAGEMENT_MESSAGE_PREAMBLE, SOFTNPU_TTY}; use softnpu_lib::ManagementRequest; use std::os::unix::net::UnixDatagram; @@ -49,11 +51,18 @@ impl std::str::FromStr for SoftnpuManagement { } /// Send a management request to a SoftNPU asic. -pub fn write(msg: ManagementRequest, config: &ManagementConfig) { +pub fn write( + msg: ManagementRequest, + config: &ManagementConfig, +) -> AsicResult<()> { match config { ManagementConfig::UART => write_uart(msg), ManagementConfig::UDS { socket_path } => write_uds(msg, socket_path), } + .map_err(|err| AsicError::Io { + ctx: "SoftNPU management write".to_string(), + err, + }) } /// Read a management request response from a SoftNPU asic. @@ -64,27 +73,28 @@ pub fn read(msg: ManagementRequest, config: &ManagementConfig) -> String { } } -fn write_uart(msg: ManagementRequest) { +fn write_uart(msg: ManagementRequest) -> io::Result<()> { let mut buf = Vec::new(); buf.push(MANAGEMENT_MESSAGE_PREAMBLE); - let mut js = serde_json::to_vec(&msg).unwrap(); + let mut js = serde_json::to_vec(&msg)?; js.retain(|x| *x != b'\n'); buf.extend_from_slice(&js); buf.push(b'\n'); - let mut f = OpenOptions::new().write(true).open(SOFTNPU_TTY).unwrap(); + let mut f = OpenOptions::new().write(true).open(SOFTNPU_TTY)?; - f.write_all(&buf).unwrap(); - f.sync_all().unwrap(); + f.write_all(&buf)?; + f.sync_all() } -fn write_uds(msg: ManagementRequest, socket_path: &str) { - let uds = UnixDatagram::unbound().unwrap(); +fn write_uds(msg: ManagementRequest, socket_path: &str) -> io::Result<()> { + let uds = UnixDatagram::unbound()?; - let buf = serde_json::to_vec(&msg).unwrap(); + let buf = serde_json::to_vec(&msg)?; let mut server_handle = socket_path.to_owned(); server_handle.push_str(SERVER_PATH); - uds.send_to(&buf, server_handle).unwrap(); + uds.send_to(&buf, server_handle)?; + Ok(()) } fn read_uart(msg: ManagementRequest) -> String { @@ -109,7 +119,7 @@ fn read_uart(msg: ManagementRequest) -> String { fn read_uds(msg: ManagementRequest, socket_path: &str) -> String { let client_uds = bind_uds(socket_path); - write_uds(msg, socket_path); + write_uds(msg, socket_path).unwrap(); let mut buf = vec![0u8; 10240]; let n = client_uds.recv(&mut buf).unwrap(); diff --git a/asic/src/softnpu/mod.rs b/asic/src/softnpu/mod.rs index 1bc419d6..38310561 100644 --- a/asic/src/softnpu/mod.rs +++ b/asic/src/softnpu/mod.rs @@ -25,7 +25,7 @@ use common::ports::{ PortFec, PortId, PortMedia, PortPrbsMode, PortSpeed, TxEq, }; -use softnpu_lib::ManagementRequest; +use softnpu_lib::{ManagementRequest, ManagementResponse}; pub mod mcast; pub mod mgmt; @@ -144,54 +144,6 @@ impl Handle { } pub fn fini(&self) {} - - /// Rewrite the replication entries refreshed after a membership change. - /// - /// SoftNPU fixes each entry's port bitmaps at write time and has no - /// in-place table update. Each affected entry is removed and re-added with - /// the recomputed bitmaps. - fn rewrite_replication_entries( - &self, - entries: impl IntoIterator, - ) { - use softnpu_lib::{TableAdd, TableRemove}; - - for entry in entries { - // Logged at debug, not trace, to keep the recomputed membership - // visible in a release build. The bitmaps are the post-refresh - // output ports; an empty one means that the group has no members - // in it to replicate to. - debug!( - self.log, - "refreshing mcast replication entry: \ - external_bitmap={:#x} underlay_bitmap={:#x} rid={}", - entry.external_bitmap, - entry.underlay_bitmap, - entry.rid - ); - - let remove = ManagementRequest::TableRemove(TableRemove { - keyset_data: entry.keyset.clone(), - table: mcast::MCAST_REPLICATION_V6_TABLE.to_string(), - }); - mgmt::write(remove, &self.mgmt_config); - - let parameter_data = [ - entry.external_bitmap.to_le_bytes().as_slice(), - entry.underlay_bitmap.to_le_bytes().as_slice(), - entry.rid.to_le_bytes().as_slice(), - ] - .concat(); - - let add = ManagementRequest::TableAdd(TableAdd { - table: mcast::MCAST_REPLICATION_V6_TABLE.to_string(), - action: mcast::SET_PORT_BITMAP_ACTION.to_string(), - keyset_data: entry.keyset, - parameter_data, - }); - mgmt::write(add, &self.mgmt_config); - } - } } impl AsicMulticastOps for Handle { @@ -205,9 +157,8 @@ impl AsicMulticastOps for Handle { mc_data.domain_port_count(group_id) } - fn mc_port_list(&self, _group_id: u16) -> AsicResult> { - // TODO: Read members from replication table bitmaps via DumpRequest. - Err(AsicError::OperationUnsupported) + fn mc_port_list(&self, group_id: u16) -> AsicResult> { + self.mc_data.lock().unwrap().domain_port_list(group_id) } fn mc_port_add( @@ -221,15 +172,12 @@ impl AsicMulticastOps for Handle { self.log, "adding port {port} to multicast group {group_id}" ); - // Hold the lock across the refresh and rewrite for the - // recomputation of the bitmap snapshot and its table writes are atomic - // concerning concurrent membership change(s). The rewrite touches only - // `mgmt_config`, not `mc_data`; therefore, no re-entrant lock. - let mut mc_data = self.mc_data.lock().unwrap(); - mc_data.domain_port_add(group_id, port, rid, level1_excl_id)?; - let refreshed = mc_data.refresh_params_for_group(group_id); - self.rewrite_replication_entries(refreshed); - Ok(()) + self.mc_data.lock().unwrap().domain_port_add( + group_id, + port, + rid, + level1_excl_id, + ) } fn mc_port_remove(&self, group_id: u16, port: u16) -> AsicResult<()> { @@ -237,11 +185,7 @@ impl AsicMulticastOps for Handle { self.log, "removing port {port} from multicast group {group_id}" ); - let mut mc_data = self.mc_data.lock().unwrap(); - mc_data.domain_port_remove(group_id, port)?; - let refreshed = mc_data.refresh_params_for_group(group_id); - self.rewrite_replication_entries(refreshed); - Ok(()) + self.mc_data.lock().unwrap().domain_port_remove(group_id, port) } fn mc_group_create(&self, group_id: u16) -> AsicResult<()> { @@ -442,13 +386,28 @@ impl AsicOps for Handle { } fn get_connectors(&self) -> Vec { - let msg = ManagementRequest::RadixRequest; - crate::softnpu::mgmt::write(msg, &self.mgmt_config); let response = crate::softnpu::mgmt::read( ManagementRequest::RadixRequest, &self.mgmt_config, ); - let radix: u16 = response.trim().parse().unwrap(); + // NOTE: propolis responds over the UART in ASCII, due to a guest tty + // without a raw mode. + // + // + // + // NOTE: standalone SoftNPU responds over UDS with a JSON + // `ManagementResponse`. + // + // + let radix: u16 = match &self.mgmt_config { + mgmt::ManagementConfig::UART => response.trim().parse().unwrap(), + mgmt::ManagementConfig::UDS { .. } => { + match serde_json::from_str(response.trim()).unwrap() { + ManagementResponse::RadixResponse(radix) => radix, + other => panic!("unexpected radix response: {other:?}"), + } + } + }; let mut result = vec![Connector::CPU]; for i in 1..radix + 1 { diff --git a/asic/src/softnpu/table.rs b/asic/src/softnpu/table.rs index 7f16a9ae..c8e5fb43 100644 --- a/asic/src/softnpu/table.rs +++ b/asic/src/softnpu/table.rs @@ -7,7 +7,7 @@ use std::hash::Hash; use slog::{error, trace}; -use softnpu_lib::{ManagementRequest, TableAdd, TableRemove}; +use softnpu_lib::{ManagementRequest, TableAdd, TableClear, TableRemove}; use crate::softnpu::Handle; use aal::{ @@ -82,22 +82,15 @@ impl Table { } /// Translate a `configure_mcastv6` request into a sidecar-lite - /// `set_port_bitmap` addition, holding the replication tracking lock across - /// the entire op. + /// `set_port_bitmap` addition. /// - /// DPD sends `(mcast_grp_a, mcast_grp_b, rid, level1_excl_id, - /// level2_excl_id)`, while sidecar-lite expects `(external, underlay, rid)`, - /// from `McGroupData` membership. On SoftNPU, the bitmap is a write-time - /// snapshot, unlike the live replication engine on the sidecar, allowing - /// recorded group references to recompute later membership changes to the - /// entry. + /// dpd sends `(mcast_grp_a, mcast_grp_b, rid, level1_excl_id, + /// level2_excl_id)`, while sidecar-lite expects + /// `(external, underlay, rid)`, from `McGroupData` membership. /// - /// The lock is held from the bitmap snapshot through to the table write on - /// a concurrent membership refresh, which takes the same lock across its - /// own rewrite (see @ `mc_port_add`). - /// - /// This cannot interleave a newer bitmap that this older snapshot would - /// then overwrite. + /// SoftNPU's bitmap is a write-time snapshot (the Sidecar's replication + /// engine tracks membership live). dpd rewrites the entry after membership + /// changes to refresh it. fn mcast_replication_add( &self, hdl: &Handle, @@ -110,15 +103,13 @@ impl Table { let underlay_grp = arg_u16(hdl, action_data, action, "mcast_grp_b")?; let rid = arg_u16(hdl, action_data, action, "rid")?; - let mut mc_data = hdl.mc_data.lock().unwrap(); - let external_bitmap = mc_data.port_bitmap(external_grp); - let underlay_bitmap = mc_data.port_bitmap(underlay_grp); - mc_data.record_repl_entry( - keyset_data.clone(), - external_grp, - underlay_grp, - rid, - ); + let (external_bitmap, underlay_bitmap) = { + let mc_data = hdl.mc_data.lock().unwrap(); + ( + mc_data.port_bitmap(external_grp), + mc_data.port_bitmap(underlay_grp), + ) + }; let parameter_data = [ external_bitmap.to_le_bytes().as_slice(), @@ -127,8 +118,6 @@ impl Table { ] .concat(); - // Logged at debug for the initial bitmap snapshot to be visible in a - // release build. slog::debug!( hdl.log, "mcast replication add: external_grp={external_grp} \ @@ -139,46 +128,32 @@ impl Table { trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); trace!(hdl.log, "parameter_data:\n{parameter_data:#?}"); - replace_entry( + add_entry( hdl, table, crate::softnpu::mcast::SET_PORT_BITMAP_ACTION.to_string(), keyset_data, parameter_data, - ); - - Ok(()) + ) } } -/// Write an entry, or replace any entry already associated with the key. -/// -/// Note: SoftNPU's `TableAdd` retrains an existing entry in place and reports -/// nothing back. The backend key lookup does not return `AsicError::Exists`, -/// for example. -/// -/// Removing the key before the add ensures that the pipeline is in agreement -/// with what DPD wrote. -fn replace_entry( +/// Send a `TableAdd` request for `table`, which replaces any entry already +/// under the key (oxidecomputer/p4#282), serving as both an add and an update. +fn add_entry( hdl: &Handle, table: String, action: String, keyset_data: Vec, parameter_data: Vec, -) { - let remove = ManagementRequest::TableRemove(TableRemove { - table: table.clone(), - keyset_data: keyset_data.clone(), - }); - crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); - +) -> AsicResult<()> { let add = ManagementRequest::TableAdd(TableAdd { table, action, keyset_data, parameter_data, }); - crate::softnpu::mgmt::write(add, &hdl.mgmt_config); + crate::softnpu::mgmt::write(add, &hdl.mgmt_config) } // All tables are defined to be 4096 entries deep. @@ -216,7 +191,11 @@ impl TableOps for Table { } }; - Ok(Table { type_, implemented, size: TABLE_SIZE }) + let tbl = Table { type_, implemented, size: TABLE_SIZE }; + // The SoftNPU pipeline outlives the dpd process. Clear each table + // before the control plane reapplies its configuration. + tbl.clear(hdl)?; + Ok(tbl) } fn size(&self) -> usize { @@ -224,32 +203,13 @@ impl TableOps for Table { } fn clear(&self, hdl: &Handle) -> AsicResult<()> { - // TODO: the management protocol has no clear message (yet). - // - // For now, only the multicast replication entries tracked in `mc_data` - // get removed here, one `TableRemove` per keyset, and every other - // table retains its entries. - // - // Send a `TableClear` once softnpu and propolis support one (the - // pipeline side landed in oxidecomputer/p4#278). - if self.type_ != TableType::McastIpv6 { - return Ok(()); - } - let Some(table) = self.softnpu_table_name() else { return Ok(()); }; - - let mut mc_data = hdl.mc_data.lock().unwrap(); - for keyset_data in mc_data.repl_keysets() { - let remove = ManagementRequest::TableRemove(TableRemove { - table: table.to_string(), - keyset_data: keyset_data.to_vec(), - }); - crate::softnpu::mgmt::write(remove, &hdl.mgmt_config); - } - mc_data.clear_repl_entries(); - Ok(()) + let msg = ManagementRequest::TableClear(TableClear { + table: table.to_string(), + }); + crate::softnpu::mgmt::write(msg, &hdl.mgmt_config) } fn entry_add( @@ -270,13 +230,13 @@ impl TableOps for Table { trace!(hdl.log, "entry_add called"); trace!(hdl.log, "table: {name}"); trace!(hdl.log, "match_data: {fields:#?}"); - trace!(hdl.log, "action_data:\n{:#?}", action_data); + trace!(hdl.log, "action_data:\n{action_data:#?}"); let keyset_data = keyset_data(fields, self.type_); - // Multicast replication is handled ahead of the generic match so the - // replication tracking lock can be held from the bitmap snapshot through - // to the table write. + // Multicast replication entries get handled separately because + // sidecar-lite's `set_port_bitmap` op requires the port bitmaps + // for the group IDs that dpd sends in `configure_mcastv6`. if let (TableType::McastIpv6, "configure_mcastv6") = (self.type_, action_data.action.as_str()) { @@ -617,12 +577,7 @@ impl TableOps for Table { | (TableType::McastIpv6SrcFilter, "allow_source_mcastv6") => { ("allow_source", Vec::new()) } - // Multicast egress decap: pack 8x32-bit bitmap into 128 bits. - // - // DPD sends set_decap_ports with 8x32-bit bitmap fields - // keyed on the RID. Sidecar-lite expects a single 128-bit - // bitmap. We pack the low 4 chunks (ports 0-127) into a - // u128 for sidecar-lite's bit<128> decap_bitmap field. + // Multicast egress decap: see `pack_decap_bitmap`. (TableType::McastEgressDecapPorts, "set_decap_ports") => { ("set_decap_ports", pack_decap_bitmap(&action_data)) } @@ -651,7 +606,7 @@ impl TableOps for Table { (TableType::McastEgressPortMapping, "set_port_number") => { ("set_port_number", vec![port_number_param(&action_data)?]) } - // Sidecar-lite's resolvers support drop (by default). + // sidecar-lite's resolvers support drop (by default). (TableType::ArpIpv4, "drop") | (TableType::NeighborIpv6, "drop") => ("drop", Vec::new()), // The local_v4/local_v6 tables have no drop action and dpd programs @@ -668,13 +623,11 @@ impl TableOps for Table { let action = action.to_string(); trace!(hdl.log, "sending request to softnpu"); trace!(hdl.log, "table: {table}"); - trace!(hdl.log, "action: {:#?}", action); - trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); - trace!(hdl.log, "parameter_data:\n{:#?}", parameter_data); - - replace_entry(hdl, table, action, keyset_data, parameter_data); + trace!(hdl.log, "action: {action:#?}"); + trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); + trace!(hdl.log, "parameter_data:\n{parameter_data:#?}"); - Ok(()) + add_entry(hdl, table, action, keyset_data, parameter_data) } fn entry_update( @@ -683,12 +636,6 @@ impl TableOps for Table { key: &M, data: &A, ) -> AsicResult<()> { - // SoftNPU does not currently support in-place updates. `entry_add` - // removes the key first before adding it back with the new action data. - // - // Note: this is not an atomic op, as a lookup between the two messages - // could miss the entry, and mgmt IO failures panic in mgmt::write - // rather than surface errors. self.entry_add(hdl, key, data) } @@ -711,22 +658,14 @@ impl TableOps for Table { let keyset_data = keyset_data(fields, self.type_); - // Stop tracking a replication entry once it leaves the pipeline so a - // later membership change does not resurrect a stale bitmap. - if self.type_ == TableType::McastIpv6 { - hdl.mc_data.lock().unwrap().forget_repl_entry(&keyset_data); - } - trace!(hdl.log, "sending request to softnpu"); trace!(hdl.log, "table: {table}"); - trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data); + trace!(hdl.log, "keyset_data:\n{keyset_data:#?}"); let msg = ManagementRequest::TableRemove(TableRemove { keyset_data, table }); - crate::softnpu::mgmt::write(msg, &hdl.mgmt_config); - - Ok(()) + crate::softnpu::mgmt::write(msg, &hdl.mgmt_config) } fn get_entries( @@ -734,7 +673,7 @@ impl TableOps for Table { _hdl: &Handle, _from_hardware: bool, ) -> AsicResult> { - Err(aal::AsicError::OperationUnsupported) + Err(AsicError::OperationUnsupported) } fn get_counters( @@ -848,11 +787,11 @@ fn arg_ptr<'a>( } } -/// Pack the low 4 chunks (ports 0-127) of DPD's 8x32-bit decap port -/// bitmap into the 16 bytes of sidecar-lite's bit<128> decap_bitmap +/// Pack the low 4 chunks (ports 0-127) of dpd's 8x32-bit decap port +/// bitmap into the 16 bytes of sidecar-lite's `bit<128>` `decap_bitmap` /// field (in little-endian). /// -/// Caveats: The high chunks are always zero on this backend and ports >= 128 +/// NOTE: The high chunks are always zero on this backend and ports >= 128 /// are rejected when adding members. Emitting exactly 16 bytes keeps /// any following action parameter (the tagged variant's vlan_id) at /// the offset sidecar-lite reads it from. diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 6be56e65..15190c81 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -2254,14 +2254,6 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); - let ctr_baseline_mcast_egress1 = switch - .get_counter(&port_label_egress1, Some("multicast")) - .await - .unwrap(); - let ctr_baseline_mcast_egress2 = switch - .get_counter(&port_label_egress2, Some("multicast")) - .await - .unwrap(); let ctr_baseline_external_egress1 = switch .get_counter(&port_label_egress1, Some("multicast_external")) .await @@ -2291,26 +2283,6 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() .await .unwrap(); - // One decapped replica should be counted per external member port. - check_counter_incremented( - switch, - &port_label_egress1, - ctr_baseline_mcast_egress1, - 1, - Some("multicast"), - ) - .await - .unwrap(); - check_counter_incremented( - switch, - &port_label_egress2, - ctr_baseline_mcast_egress2, - 1, - Some("multicast"), - ) - .await - .unwrap(); - // Decapped replicas to external members must be attributed to the // external counter, not the underlay one. check_counter_incremented( @@ -2660,22 +2632,6 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); - let ctr_baseline_mcast_egress1 = switch - .get_counter(&port_label_egress1, Some("multicast")) - .await - .unwrap(); - let ctr_baseline_mcast_egress2 = switch - .get_counter(&port_label_egress2, Some("multicast")) - .await - .unwrap(); - let ctr_baseline_mcast_egress3 = switch - .get_counter(&port_label_egress3, Some("multicast")) - .await - .unwrap(); - let ctr_baseline_mcast_egress4 = switch - .get_counter(&port_label_egress4, Some("multicast")) - .await - .unwrap(); let ctr_baseline_underlay_egress1 = switch .get_counter(&port_label_egress1, Some("multicast_underlay")) .await @@ -2721,24 +2677,6 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe .await .unwrap(); - // One replica should be counted per member port, external & underlay alike. - for (label, baseline) in [ - (&port_label_egress1, ctr_baseline_mcast_egress1), - (&port_label_egress2, ctr_baseline_mcast_egress2), - (&port_label_egress3, ctr_baseline_mcast_egress3), - (&port_label_egress4, ctr_baseline_mcast_egress4), - ] { - check_counter_incremented( - switch, - label, - baseline, - 1, - Some("multicast"), - ) - .await - .unwrap(); - } - // Still-encapsulated replicas to underlay members count as underlay, // not external, for UNDERLAY_EXTERNAL tagged groups. check_counter_incremented( diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 3682a318..9409b51e 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -414,12 +414,77 @@ impl From<&MulticastGroup> for MulticastGroupResponse { } } +/// Multicast groups awaiting repair, with source-filter keys whose removal +/// can't be confirmed. +/// +/// A failed rollback of an external update can leave source filters in the +/// ASIC that are absent from the stored group. Repair and deletion clear these +/// unobserved sources first. +/// +/// NOTE: On backends with per-table dumps (e.g., Tofino), the dump in +/// `del_entries_where` already handles this. We track them here for +/// backends without per-table dumps (e.g., SoftNPU), where a leftover +/// `/0` could admit every source. +#[derive(Debug, Default)] +struct PendingRepairs(BTreeMap>); + +impl PendingRepairs { + fn contains(&self, key: &Q) -> bool + where + GroupKey: Borrow, + Q: Ord + ?Sized, + { + self.0.contains_key(key) + } + + fn insert(&mut self, key: GroupKey) { + self.0.entry(key).or_default(); + } + + fn extend(&mut self, keys: impl IntoIterator) { + keys.into_iter().for_each(|key| self.insert(key)); + } + + fn add_sources( + &mut self, + key: GroupKey, + srcs: impl IntoIterator, + ) { + self.0.entry(key).or_default().extend(srcs); + } + + fn sources(&self, key: &Q) -> BTreeSet + where + GroupKey: Borrow, + Q: Ord + ?Sized, + { + self.0.get(key).cloned().unwrap_or_default() + } + + fn remove(&mut self, key: &Q) -> bool + where + GroupKey: Borrow, + Q: Ord + ?Sized, + { + self.0.remove(key).is_some() + } + + #[cfg(all(test, feature = "chaos"))] + fn is_empty(&self) -> bool { + self.0.is_empty() + } + + fn clear(&mut self) { + self.0.clear(); + } +} + /// Stores multicast group configurations. #[derive(Debug)] pub struct MulticastGroupData { /// Multicast group configurations keyed by group IP. groups: BTreeMap, - pending_repairs: BTreeSet, + pending_repairs: PendingRepairs, /// The stack of available group IDs for O(1) allocation. /// Pre-populated with all IDs from GENERATOR_START to u16::MAX-1. free_group_ids: Arc>>, @@ -444,7 +509,7 @@ impl MulticastGroupData { Self { groups: BTreeMap::new(), - pending_repairs: BTreeSet::new(), + pending_repairs: PendingRepairs::default(), free_group_ids, nat_target_refs: BTreeMap::new(), } @@ -805,6 +870,22 @@ fn del_group_locked( debug!(s.log, "deleting multicast group for IP {group_ip}"); + let stored_srcs = match &group.kind { + GroupKind::External { sources: srcs, .. } => Some(srcs), + GroupKind::Underlay { .. } => None, + }; + clear_unobserved_sources( + s, + group_ip, + &mcast.pending_repairs.sources(&group_ip), + stored_srcs, + ) + .map_err(|_| { + DpdError::McastRepairPending(format!( + "multicast group {group_ip} state is uncertain; retry the request" + )) + })?; + let rollback_ctx = GroupDeleteRollbackContext::new(s, &group); let mut deleted = Vec::new(); @@ -969,7 +1050,9 @@ fn repair_pending_external_group( external_forwarding: ExternalForwarding { vlan_id: *vlan_id }, sources: None, }; + let unobserved = mcast.pending_repairs.sources(&group_ip); let repaired = (|| { + clear_unobserved_sources(s, group_ip, &unobserved, Some(sources))?; repair_nat_target(s, mcast, *nat_target)?; update_external_tables(s, &group, &stored, sources, &[])?; let underlay = mcast.get_nat_target_group(group_ip, *nat_target)?; @@ -992,6 +1075,37 @@ fn repair_pending_external_group( Ok(()) } +/// Clear unobserved source filters for `group_ip` that aren't in `stored`. +fn clear_unobserved_sources( + s: &Switch, + group_ip: IpAddr, + unobserved: &BTreeSet, + stored: Option<&SourceFilter>, +) -> DpdResult<()> { + apply_each( + unobserved + .iter() + .filter(|source| { + !stored + .is_some_and(|srcs| srcs.iter().any(|src| src == **source)) + }) + .cloned(), + |source| clear_source_filter(s, group_ip, source), + ) + .map_err(|failures| { + for (source, e) in failures.iter() { + error!( + s.log, + "failed to clear unobserved source filter"; + "group_ip" => %group_ip, + "source" => ?source, + "error" => ?e, + ); + } + failures.head.1 + }) +} + fn repair_nat_target( s: &Switch, mcast: &mut MulticastGroupData, @@ -1095,8 +1209,13 @@ pub(crate) fn modify_group_external( ) { let err = rollback_ctx.rollback_external(e, &new_sources); if matches!(err, DpdError::Unwind { .. }) { - mcast.pending_repairs.extend([ + mcast.pending_repairs.add_sources( group_key, + new_sources + .iter() + .filter(|new| !old_sources.iter().any(|old| old == *new)), + ); + mcast.pending_repairs.extend([ GroupKey::underlay(old_underlay_ip), GroupKey::underlay(new_underlay_ip), ]); @@ -1180,6 +1299,12 @@ pub(crate) fn modify_group_underlay( MulticastReplicationInfo::new(original.external_group_id()) }); + // The decap bitmap must cover external ports before they are added to + // ASIC groups. + // + // Normal updates write the bitmap before port changes only when widening + // it. A group pending repair may have lost its bitmap during rollback; + // restore coverage for its stored external members as well. let widened = (!members.is_empty()) .then(|| { original.widened_external_bitmap(&members).or_else(|| { @@ -1189,6 +1314,7 @@ pub(crate) fn modify_group_underlay( }) }) .flatten(); + let mut added_members = Vec::new(); let mut removed_members = Vec::new(); let programmed = (|| { @@ -1222,27 +1348,33 @@ pub(crate) fn modify_group_underlay( .map_err(|e| (UpdateStep::Tables, e)); } - // Ensure the bitmap exists before the replication entry. The VLAN - // comes from the referencing external group. The bitmap covers - // overlay members only because decapsulation applies only to overlay - // traffic. - table::mcast::mcast_egress::set_bitmap_entry( - s, - original.external_group_id(), - &create_port_bitmap(&members, Direction::External), - external_group_vlan_id, - ) - .map_err(|e| (UpdateStep::Tables, e))?; - - // Ensure the replication entry on every update with members. - // - // An entry lost to a partial delete can then come back. It's inserted - // after the bitmap, where a rollback deletes a newly added bitmap on - // failure, and if that delete also fails, then the leftover is a - // bitmap that nothing can replicate toward (instead of a replication - // entry with no decap entry). + let union_bitmap = widened.clone().unwrap_or_else(|| { + create_port_bitmap(original.members(), Direction::External) + }); + + // Write the replication entry on membership updates. Backends + // whose replication entry persists a write-time port bitmap (e.g., + // SoftNPU) only see membership changes when the bitmap is rewritten. set_replication_entry(s, &original, &info) - .map_err(|e| (UpdateStep::Tables, e)) + .map_err(|e| (UpdateStep::Tables, e))?; + + // Only narrow the decap port bitmap after the replication entry is + // rewritten, keeping decap at a superset of the ports replication can + // still send to. Backends whose replication entry holds a write-time + // port bitmap (e.g., SoftNPU) keep sending to removed ports until the + // replication entry is rewritten, and those packet copies must still + // have their Geneve header stripped. + let new_bitmap = create_port_bitmap(&members, Direction::External); + if new_bitmap != union_bitmap { + table::mcast::mcast_egress::set_bitmap_entry( + s, + original.external_group_id(), + &new_bitmap, + external_group_vlan_id, + ) + .map_err(|e| (UpdateStep::Tables, e))?; + } + Ok(()) })(); if let Err((step, e)) = programmed { @@ -1380,7 +1512,12 @@ fn resync_after_failed_unwind( } } - match reprogram_underlay_entries(s, &resynced, vlan_id) { + match reprogram_underlay_entries( + s, + &resynced, + vlan_id, + group_entry.members(), + ) { Ok(()) => Some(resynced), Err(e) => { error!( @@ -1407,10 +1544,11 @@ fn clear_group_after_failed_resync( ) } -fn reprogram_underlay_entries( +fn reprogram_underlay_entries<'a>( s: &Switch, - group: &MulticastGroup, + group: &'a MulticastGroup, vlan_id: Option, + previous_members: impl IntoIterator, ) -> DpdResult<()> { let GroupKind::Underlay { group_ip, replication: repl } = &group.kind else { @@ -1419,8 +1557,22 @@ fn reprogram_underlay_entries( match repl { Replication::Active { info, .. } => { - set_group_bitmap_entries(s, group, vlan_id)?; + let union_bitmap = create_port_bitmap( + group.members().chain(previous_members), + Direction::External, + ); + table::mcast::mcast_egress::set_bitmap_entry( + s, + group.external_group_id(), + &union_bitmap, + vlan_id, + )?; set_replication_entry(s, group, info)?; + let bitmap = + create_port_bitmap(group.members(), Direction::External); + if bitmap != union_bitmap { + set_group_bitmap_entries(s, group, vlan_id)?; + } } Replication::Empty => cleanup_empty_group_replication(s, group)?, } @@ -1447,9 +1599,6 @@ fn resync_members_from_asic( let asic_ids = match s.asic_hdl.mc_port_list(group_id) { Ok(asic_ids) => asic_ids, Err(AsicError::Missing(_)) => Vec::new(), - Err(AsicError::OperationUnsupported) => { - return Some(group_entry.clone()); - } Err(e) => { error!( s.log, @@ -1570,18 +1719,6 @@ pub(crate) fn reset(s: &Switch) -> DpdResult<()> { let keys = mcast.groups.keys().copied().collect::>(); mcast.pending_repairs.extend(keys); - // Remove each known group's entries before the table clears below, as - // SoftNPU's `clear` cannot empty its tables. - for group in mcast.groups.values() { - if let Err(e) = delete_group_tables(s, group, &mut Vec::new()) { - error!( - s.log, - "failed to delete table entries for multicast group {}: {e:?}", - group.ip() - ); - } - } - // Destroy ASIC groups let group_ids = s.asic_hdl.mc_domains(); for group_id in group_ids { @@ -1683,7 +1820,7 @@ fn ensure_bitmap_vlan( res => return res, } - reprogram_underlay_entries(s, group, vlan_id) + reprogram_underlay_entries(s, group, vlan_id, group.members()) } /// Remove source filters for a multicast group. @@ -1838,11 +1975,11 @@ fn clear_source_filter( /// Remove ASIC source-filter entries for `group` that are not in `sources`. /// -/// The group's stored sources missing from what `sources` contains are cleared -/// first, then any other entry for the group address is swept from what the -/// table lists. +/// Stored sources not in `sources` are cleared first, and then any other +/// listed entry for the group address is removed. /// -/// Backends that don't list entries (e.g., SoftNPU) skip the periodic sweep. +/// Backends that don't list entries (e.g., SoftNPU) only clear the stored +/// sources. fn remove_unwanted_source_filters( s: &Switch, group: &MulticastGroup, @@ -2248,15 +2385,6 @@ fn remove_unwanted_group_members( ); continue; } - Err(AsicError::OperationUnsupported) => { - debug!( - s.log, - "multicast group readback unsupported, skipping unwanted port removal"; - "group_ip" => %group_ip, - "group_id" => group_id, - ); - continue; - } Err(e) => return Err(e.into()), }; diff --git a/dpd/src/mcast/rollback.rs b/dpd/src/mcast/rollback.rs index a4abc7aa..28b06783 100644 --- a/dpd/src/mcast/rollback.rs +++ b/dpd/src/mcast/rollback.rs @@ -20,9 +20,9 @@ use super::{ Direction, ExternalMulticastIp, GroupKind, IpSrc, MulticastGroup, MulticastGroupId, MulticastGroupMember, MulticastReplicationInfo, Replication, SourceFilter, UnderlayMulticastIpv6, clear_source_filter, - mc_group_clear, mc_group_set, mc_port_clear, mc_port_set, - restore_source_filters, set_group_bitmap_entries, set_replication_entry, - set_source_filter, update_fwding_tables, + create_port_bitmap, mc_group_clear, mc_group_set, mc_port_clear, + mc_port_set, restore_source_filters, set_group_bitmap_entries, + set_replication_entry, set_source_filter, update_fwding_tables, }; use crate::{ Switch, table, @@ -536,15 +536,58 @@ impl<'a> GroupUpdateRollbackContext<'a> { Self { switch, original_group, table_vlan_id } } - /// Restore table entries to original state. - fn restore_tables(&self) -> DpdResult<()> { + fn restore_union_bitmap( + &self, + added_ports: &[MulticastGroupMember], + ) -> DpdResult<()> { + if !matches!( + self.original_group.kind, + GroupKind::Underlay { replication: Replication::Active { .. }, .. } + ) { + return Ok(()); + } + + // Port rollback can immediately enable replicas on Tofino for example. + // + // This restores decapsulation for the original and added members before + // modifying any ports. + let union_bitmap = create_port_bitmap( + self.original_group.members().chain(added_ports), + Direction::External, + ); + self.log_rollback_error( + "restore decap bitmap", + &format!("for group {}", self.group_ip()), + table::mcast::mcast_egress::set_bitmap_entry( + self.switch, + self.external_group_id(), + &union_bitmap, + self.table_vlan_id, + ), + ) + } + + fn restore_tables( + &self, + added_ports: &[MulticastGroupMember], + ports_restored: bool, + ) -> DpdResult<()> { match &self.original_group.kind { GroupKind::Underlay { replication: Replication::Active { .. }, .. - } => self - .restore_fwding_tables() - .and_then(|()| self.restore_replication()), + } => { + self.restore_replication()?; + if ports_restored + && added_ports + .iter() + .any(|member| member.direction == Direction::External) + { + self.restore_fwding_tables() + } else { + Ok(()) + } + } GroupKind::Underlay { replication: Replication::Empty, .. } => { unwind_outcome([ self.remove_added_bitmap(), @@ -644,6 +687,10 @@ impl<'a> GroupUpdateRollbackContext<'a> { /// Undo an underlay group's port changes, restore its tables, and report /// the result with the failure that originally triggered the rollback. + /// + /// If the decap bitmap can't be restored to cover both original + /// and added members, rollback removes the added ports and avoids re-adding + /// removed ports. pub(crate) fn rollback_underlay_and_restore( &self, err: DpdError, @@ -651,11 +698,23 @@ impl<'a> GroupUpdateRollbackContext<'a> { added_ports: &[MulticastGroupMember], removed_ports: &[MulticastGroupMember], ) -> DpdError { - after_unwind( - err, - self.rollback_underlay_update(added_ports, removed_ports, repl) - .and(self.restore_tables()), - ) + if let Err(unwind) = self.restore_union_bitmap(added_ports) { + if !removed_ports.is_empty() { + warn!( + self.switch.log, + "decap bitmap restore failed, leaving removed ports out \ + of the group"; + "group" => %self.group_ip(), + "skipped_ports" => removed_ports.len(), + ); + } + let ports = self.rollback_underlay_update(added_ports, &[], repl); + return after_unwind(err, unwind_outcome([Err(unwind), ports])); + } + let ports = + self.rollback_underlay_update(added_ports, removed_ports, repl); + let tables = self.restore_tables(added_ports, ports.is_ok()); + after_unwind(err, ports.and(tables)) } } @@ -1746,6 +1805,7 @@ mod chaos_tests { source_table, TableOp::EntryDel, CallRange::starting_at(1), + AsicError::Synthetic, ); let update = || MulticastGroupUpdateExternalEntry { internal_forwarding: ExternalInternalForwarding { @@ -1801,6 +1861,132 @@ mod chaos_tests { } } + #[test] + fn test_external_cleanup_without_listing() { + for (group_ip, source_ip, del) in [ + ("224.1.2.3", "192.0.2.1", false), + ("ff0e::1", "2001:db8::1", false), + ("224.1.2.3", "192.0.2.1", true), + ("ff0e::1", "2001:db8::1", true), + ] { + let external_ip: IpAddr = group_ip.parse().unwrap(); + let (_, source_table, _) = external_tables(external_ip); + let (switch, _logs) = chaos_switch(Config::default()); + let (underlay_ip, _) = install_underlay_group(&switch); + let nat_target = external_nat_target(external_ip, underlay_ip); + let tag = "external-unlisted-cleanup-test"; + add_group_external( + &switch, + external_entry( + external_ip, + tag, + nat_target, + None, + serde_json::json!([{ "Exact": source_ip }]), + ), + ) + .unwrap(); + let stored = + serde_json::to_value(get_group(&switch, external_ip).unwrap()) + .unwrap(); + let stored_sources = source_filter_prefixes(&switch, external_ip); + + switch.asic_hdl.inject_table_faults( + source_table, + TableOp::EntryList, + CallRange::starting_at(1), + |_| AsicError::OperationUnsupported, + ); + switch.asic_hdl.inject_table_faults( + source_table, + TableOp::EntryDel, + CallRange::starting_at(1), + AsicError::Synthetic, + ); + let err = modify_group_external( + &switch, + ExternalMulticastIp::new(external_ip).unwrap(), + tag, + MulticastGroupUpdateExternalEntry { + internal_forwarding: ExternalInternalForwarding { + nat_target: nat_target.try_into().unwrap(), + }, + external_forwarding: ExternalForwarding { vlan_id: None }, + sources: Some(vec![SourceEntry::Any]), + }, + ) + .unwrap_err(); + assert!(matches!(err, DpdError::Unwind { .. }), "{err:?}"); + assert_eq!(table_usage(&switch, source_table).0, 2); + + let res = if del { + del_group(&switch, external_ip, tag) + } else { + get_group(&switch, external_ip).map(|_| ()) + }; + assert!( + matches!(res, Err(DpdError::McastRepairPending(_))), + "{res:?}" + ); + assert_eq!(table_usage(&switch, source_table).0, 2); + { + let mcast = switch.mcast.lock().unwrap(); + assert!(mcast.pending_repairs.contains(&external_ip)); + assert_eq!( + mcast.pending_repairs.sources(&external_ip), + BTreeSet::from([IpSrc::Any]) + ); + assert_eq!( + serde_json::to_value(MulticastGroupResponse::from( + &mcast.groups[&external_ip] + )) + .unwrap(), + stored, + ); + assert_eq!( + mcast.nat_target_refs.get(&underlay_ip).map(GroupKey::ip), + Some(external_ip) + ); + } + + switch.asic_hdl.clear_table_faults(source_table, TableOp::EntryDel); + if del { + del_group(&switch, external_ip, tag).unwrap(); + assert_eq!(table_usage(&switch, source_table).0, 0); + assert!(route_action(&switch, external_ip).is_none()); + assert!(nat_target_addr(&switch, external_ip).is_none()); + let mcast = switch.mcast.lock().unwrap(); + assert!(!mcast.groups.contains_key(&external_ip)); + assert!(!mcast.nat_target_refs.contains_key(&underlay_ip)); + } else { + assert_eq!( + serde_json::to_value( + get_group(&switch, external_ip).unwrap() + ) + .unwrap(), + stored, + ); + assert_eq!(table_usage(&switch, source_table).0, 1); + } + assert!( + !switch + .mcast + .lock() + .unwrap() + .pending_repairs + .contains(&external_ip) + ); + + switch + .asic_hdl + .clear_table_faults(source_table, TableOp::EntryList); + assert_eq!( + source_filter_prefixes(&switch, external_ip), + if del { BTreeSet::new() } else { stored_sources } + ); + } + } + #[test] fn test_external_delete_removes_untracked_source_filters() { for (group_ip, source_ip, stray_ip) in [ @@ -1835,126 +2021,57 @@ mod chaos_tests { } } - const UNLISTED_SOURCE_CASES: [(&str, &str, &str); 2] = [ - ("224.1.2.3", "192.0.2.1", "192.0.2.2"), - ("ff0e::1", "2001:db8::1", "2001:db8::2"), - ]; - - fn unlisted_source_filter_group( - external_ip: IpAddr, - tag: &str, - source_ips: &[IpAddr], - ) -> (Switch, NatTarget) { - let (_, source_table, _) = external_tables(external_ip); - let mut config = Config::default(); - config.asic_config.entries_unsupported.insert(source_table); - let (switch, _logs) = chaos_switch(config); - let (underlay_ip, _) = install_underlay_group(&switch); - let nat_target = external_nat_target(external_ip, underlay_ip); - let sources: Vec = source_ips - .iter() - .map(|source_ip| serde_json::json!({ "Exact": source_ip })) - .collect(); - add_group_external( - &switch, - external_entry( - external_ip, - tag, - nat_target, - None, - serde_json::Value::Array(sources), - ), - ) - .unwrap(); - (switch, nat_target) - } - - fn take_source_filter( - switch: &Switch, - external_ip: IpAddr, - source_ip: IpAddr, - ) -> bool { - let (_, source_table, _) = external_tables(external_ip); - let (_, _, deletes_before, _) = table_usage(switch, source_table); - clear_source_filter(switch, external_ip, IpSrc::Exact(source_ip)) - .unwrap(); - let (_, _, deletes_after, _) = table_usage(switch, source_table); - deletes_after > deletes_before - } - - #[test] - fn test_external_create_with_unlisted_source_filters() { - for (group_ip, first_source, second_source) in UNLISTED_SOURCE_CASES { - let external_ip: IpAddr = group_ip.parse().unwrap(); - let source_ips: [IpAddr; 2] = - [first_source.parse().unwrap(), second_source.parse().unwrap()]; - let (switch, _) = unlisted_source_filter_group( - external_ip, - "external-unlisted-create-test", - &source_ips, - ); - - for source_ip in source_ips { - assert!( - take_source_filter(&switch, external_ip, source_ip), - "{source_ip} -> {external_ip}" - ); - } - } - } - + // Without per-table dumps (e.g., SoftNPU), only the group's stored + // sources get removed; the untracked entries remain in place. #[test] - fn test_external_update_removes_unlisted_source_filters() { - for (group_ip, kept_source, removed_source) in UNLISTED_SOURCE_CASES { + fn test_delete_unlisted_source_filters() { + for (group_ip, first_source, second_source, stray_ip) in [ + ("224.1.2.3", "192.0.2.1", "192.0.2.2", "192.0.2.3"), + ("ff0e::1", "2001:db8::1", "2001:db8::2", "2001:db8::3"), + ] { let external_ip: IpAddr = group_ip.parse().unwrap(); - let kept_source: IpAddr = kept_source.parse().unwrap(); - let removed_source: IpAddr = removed_source.parse().unwrap(); - let tag = "external-unlisted-update-test"; - let (switch, nat_target) = unlisted_source_filter_group( - external_ip, - tag, - &[kept_source, removed_source], - ); - - modify_group_external( + let (_, source_table, _) = external_tables(external_ip); + let (switch, _logs) = chaos_switch(Config::default()); + let (underlay_ip, _) = install_underlay_group(&switch); + let tag = "external-delete-unlisted-test"; + add_group_external( &switch, - ExternalMulticastIp::new(external_ip).unwrap(), - tag, - MulticastGroupUpdateExternalEntry { - internal_forwarding: ExternalInternalForwarding { - nat_target: nat_target.try_into().unwrap(), - }, - external_forwarding: ExternalForwarding { vlan_id: None }, - sources: Some(vec![SourceEntry::Exact( - ExactSource::new(kept_source).unwrap(), - )]), - }, + external_entry( + external_ip, + tag, + external_nat_target(external_ip, underlay_ip), + None, + serde_json::json!([ + { "Exact": first_source }, + { "Exact": second_source }, + ]), + ), ) .unwrap(); + set_source_filter( + &switch, + external_ip, + IpSrc::Exact(stray_ip.parse().unwrap()), + ) + .unwrap(); + assert_eq!(source_filter_prefixes(&switch, external_ip).len(), 3); - assert!(!take_source_filter(&switch, external_ip, removed_source)); - assert!(take_source_filter(&switch, external_ip, kept_source)); - } - } - - #[test] - fn test_external_delete_removes_unlisted_source_filters() { - for (group_ip, first_source, second_source) in UNLISTED_SOURCE_CASES { - let external_ip: IpAddr = group_ip.parse().unwrap(); - let source_ips: [IpAddr; 2] = - [first_source.parse().unwrap(), second_source.parse().unwrap()]; - let tag = "external-unlisted-delete-test"; - let (switch, _) = - unlisted_source_filter_group(external_ip, tag, &source_ips); - + switch.asic_hdl.inject_table_faults( + source_table, + TableOp::EntryList, + CallRange::starting_at(1), + |_| AsicError::OperationUnsupported, + ); del_group(&switch, external_ip, tag).unwrap(); + switch + .asic_hdl + .clear_table_faults(source_table, TableOp::EntryList); - for source_ip in source_ips { - assert!( - !take_source_filter(&switch, external_ip, source_ip), - "{source_ip} -> {external_ip}" - ); - } + let prefix_len = if external_ip.is_ipv4() { 32 } else { 128 }; + assert_eq!( + source_filter_prefixes(&switch, external_ip), + BTreeSet::from([format!("{stray_ip}/{prefix_len}")]) + ); } } @@ -1984,6 +2101,7 @@ mod chaos_tests { TableType::McastEgressDecapPorts, TableOp::EntryUpdate, CallRange::starting_at(1), + AsicError::Synthetic, ); let err = del_group(&switch, external_ip, tag).unwrap_err(); assert!(matches!(err, DpdError::Unwind { .. }), "{err:?}"); @@ -2025,11 +2143,13 @@ mod chaos_tests { TableType::RouteIpv6Mcast, TableOp::EntryAdd, CallRange::nth(1), + AsicError::Synthetic, ); switch.asic_hdl.inject_table_faults( TableType::McastIpv6, TableOp::EntryDel, CallRange::nth(1), + AsicError::Synthetic, ); let err = add_group_underlay(&switch, create()).unwrap_err(); @@ -2063,6 +2183,7 @@ mod chaos_tests { TableType::RouteIpv6Mcast, TableOp::EntryAdd, CallRange::starting_at(1), + AsicError::Synthetic, ); let err = get_group_underlay(&switch, membership_group_ip()).unwrap_err(); @@ -2218,11 +2339,13 @@ mod chaos_tests { route_table, TableOp::EntryAdd, CallRange::nth(1), + AsicError::Synthetic, ); switch.asic_hdl.inject_table_faults( nat_table, TableOp::EntryDel, CallRange::nth(1), + AsicError::Synthetic, ); let err = add_group_external(&switch, create()).unwrap_err(); @@ -2275,6 +2398,7 @@ mod chaos_tests { route_table, TableOp::EntryAdd, CallRange::starting_at(1), + AsicError::Synthetic, ); let err = get_group(&switch, external_ip).unwrap_err(); assert!(matches!(err, DpdError::McastRepairPending(_)), "{err:?}"); @@ -2335,11 +2459,13 @@ mod chaos_tests { TableType::McastIpv6, TableOp::EntryUpdate, CallRange::nth(1), + AsicError::Synthetic, ); switch.asic_hdl.inject_table_faults( TableType::McastEgressDecapPorts, TableOp::EntryUpdate, CallRange::nth(1), + AsicError::Synthetic, ); let tag = "external-create-vlan-test"; @@ -2918,6 +3044,12 @@ mod chaos_tests { u32::from(!bitmap_missing), "{context}" ); + switch + .mcast + .lock() + .unwrap() + .pending_repairs + .insert(underlay_key); let requested_members = if membership_changed { let kept: Vec<_> = members @@ -2955,6 +3087,10 @@ mod chaos_tests { let groups = switch.mcast.lock().unwrap(); assert!(groups.groups.contains_key(&underlay_key)); + assert!( + !groups.pending_repairs.contains(&underlay_key), + "{context}" + ); assert_eq!( groups.free_group_ids.lock().unwrap().len(), free_ids - 2, @@ -3137,6 +3273,7 @@ mod chaos_tests { TableType::McastEgressDecapPorts, TableOp::EntryDel, CallRange::nth(1), + AsicError::Synthetic, ); } } @@ -3916,9 +4053,10 @@ mod chaos_tests { let (tag, members) = underlay_tag_and_members(&switch, underlay_key); switch.asic_hdl.inject_table_faults( - TableType::McastEgressDecapPorts, - TableOp::EntryUpdate, - CallRange::between(2, 3), + TableType::McastIpv6, + TableOp::EntryAdd, + CallRange::between(1, 2), + AsicError::Synthetic, ); table::mcast::mcast_replication::clear_ipv6_entry( @@ -3949,7 +4087,7 @@ mod chaos_tests { let rollback_op = if port_remove_fails { "mc_port_remove" } else { - "table_entry_update" + "table_entry_add" }; assert!( matches!( @@ -3959,7 +4097,7 @@ mod chaos_tests { init.as_ref(), DpdError::Switch( aal::AsicError::Synthetic(op) - ) if op == "table_entry_update" + ) if op == "table_entry_add" ) && unwind.len() == 1 && matches!( &unwind.head, @@ -3980,10 +4118,10 @@ mod chaos_tests { let expected: &[(&str, &str)] = if port_remove_fails { &[ ("port changes", "mc_port_remove"), - ("restore VLAN settings", "table_entry_update"), + ("restore replication settings", "table_entry_add"), ] } else { - &[("restore VLAN settings", "table_entry_update")] + &[("restore replication settings", "table_entry_add")] }; { let errs = logs.entries.lock().unwrap(); @@ -4704,51 +4842,38 @@ mod chaos_tests { let mut with_dup = unsorted.to_vec(); with_dup.push(unsorted[1].clone()); - for readback in [true, false] { - let mut config = Config::default(); - if !readback { - config - .asic_config - .mc_config - .unsupported - .insert(McOp::McPortList); - } - let (switch, _logs) = chaos_switch(config); + let (switch, _logs) = chaos_switch(Config::default()); - let created = add_group_underlay( - &switch, - MulticastGroupCreateUnderlayEntry { - group_ip: membership_group_ip(), - tag: Some(MEMBERSHIP_TEST_TAG.to_string()), - members: unsorted.to_vec(), - }, - ) - .unwrap(); - assert_eq!(created.members, sorted, "readback={readback}"); - assert_consistent_membership(&switch, &sorted, readback); + let created = add_group_underlay( + &switch, + MulticastGroupCreateUnderlayEntry { + group_ip: membership_group_ip(), + tag: Some(MEMBERSHIP_TEST_TAG.to_string()), + members: unsorted.to_vec(), + }, + ) + .unwrap(); + assert_eq!(created.members, sorted); + assert_consistent_membership(&switch, &sorted); - let updated = put_membership(&switch, &with_dup).unwrap(); - assert_eq!(updated.members, sorted, "readback={readback}"); - assert_consistent_membership(&switch, &sorted, readback); - } + let updated = put_membership(&switch, &with_dup).unwrap(); + assert_eq!(updated.members, sorted); + assert_consistent_membership(&switch, &sorted); } fn assert_consistent_membership( switch: &Switch, expected: &[MulticastGroupMember], - readback: bool, ) { - assert_eq!(stored_membership(switch), expected, "readback={readback}"); + assert_eq!(stored_membership(switch), expected); for (reader, read) in READERS.into_iter().zip(read_memberships(switch)) { - assert_eq!(read, expected, "{reader:?} readback={readback}"); - } - if readback { - assert_eq!( - asic_membership(switch), - expected_membership(switch, expected) - ); + assert_eq!(read, expected, "{reader:?}"); } + assert_eq!( + asic_membership(switch), + expected_membership(switch, expected) + ); } #[test] @@ -4786,6 +4911,7 @@ mod chaos_tests { TableType::McastIpv6, TableOp::EntryUpdate, CallRange::starting_at(1), + AsicError::Synthetic, ); assert_uncertain_reads(&switch); @@ -4820,6 +4946,7 @@ mod chaos_tests { TableType::McastIpv6, TableOp::EntryUpdate, CallRange::starting_at(1), + AsicError::Synthetic, ); for read in read_memberships(&switch) { assert_eq!(read, members[..2]); @@ -4875,24 +5002,13 @@ mod chaos_tests { } fn underlay_read_restores_tables(reader: Reader) { - for (failed_table, readback) in [ - (TableType::McastIpv6, true), - (TableType::McastEgressDecapPorts, true), - (TableType::RouteIpv6Mcast, true), - (TableType::McastIpv6, false), - (TableType::McastEgressDecapPorts, false), - (TableType::RouteIpv6Mcast, false), + for failed_table in [ + TableType::McastIpv6, + TableType::McastEgressDecapPorts, + TableType::RouteIpv6Mcast, ] { let members = membership_members(); - let mut config = Config::default(); - if !readback { - config - .asic_config - .mc_config - .unsupported - .insert(McOp::McPortList); - } - let (switch, _logs) = chaos_switch(config); + let (switch, _logs) = chaos_switch(Config::default()); create_membership_group(&switch, &members); let external_ip: IpAddr = "239.1.2.3".parse().unwrap(); add_group_external( @@ -4941,6 +5057,7 @@ mod chaos_tests { failed_table, TableOp::EntryAdd, CallRange::starting_at(1), + AsicError::Synthetic, ); assert_uncertain_reads(&switch); @@ -4952,15 +5069,13 @@ mod chaos_tests { .pending_repairs .contains(&GroupKey::underlay(membership_group_ip())) ); - if readback { - assert_eq!( - asic_membership(&switch), - expected_membership(&switch, &members), - ); - } + assert_eq!( + asic_membership(&switch), + expected_membership(&switch, &members), + ); switch.asic_hdl.clear_table_faults(failed_table, TableOp::EntryAdd); - let context = format!("{reader:?}, {failed_table}, {readback}"); + let context = format!("{reader:?}, {failed_table}"); let expected = members.iter().cloned().collect::>(); assert_eq!( read_membership(&switch, reader) @@ -5171,6 +5286,272 @@ mod chaos_tests { } } + #[test] + fn test_underlay_narrow_failure_restores_group() { + let members = membership_members(); + for replacement in [None, Some(rear_member(3, Direction::External))] { + let context = format!("replacement={replacement:?}"); + let narrow_call = 1 + u64::from(replacement.is_some()); + let mut new_members = members[..2].to_vec(); + new_members.extend(replacement); + let (switch, logs) = chaos_switch(Config::default()); + create_membership_group(&switch, &members); + + let external_id = membership_group_id(&switch, Direction::External); + let bitmap_before = bitmap_entry(&switch, external_id); + let repl_before = + forwarding_table_snapshot(&switch, TableType::McastIpv6); + let repl_updates = table_usage(&switch, TableType::McastIpv6).3; + let group_before = { + let groups = switch.mcast.lock().unwrap(); + format!( + "{:?}", + groups.groups[&GroupKey::underlay(membership_group_ip())] + ) + }; + + switch.asic_hdl.inject_table_faults( + TableType::McastEgressDecapPorts, + TableOp::EntryUpdate, + CallRange::nth(narrow_call), + AsicError::Synthetic, + ); + let err = put_membership(&switch, &new_members).unwrap_err(); + assert!( + matches!( + &err, + DpdError::Switch(AsicError::Synthetic(op)) + if op == "table_entry_update" + ), + "{context}: {err:?}" + ); + switch.asic_hdl.clear_table_faults( + TableType::McastEgressDecapPorts, + TableOp::EntryUpdate, + ); + + assert_eq!( + bitmap_entry(&switch, external_id), + bitmap_before, + "{context}" + ); + assert_eq!( + forwarding_table_snapshot(&switch, TableType::McastIpv6), + repl_before, + "{context}" + ); + assert_eq!( + table_usage(&switch, TableType::McastIpv6).3, + repl_updates + 2, + "{context}" + ); + assert_eq!( + asic_membership(&switch), + expected_membership(&switch, &members), + "{context}" + ); + let groups = switch.mcast.lock().unwrap(); + let key = GroupKey::underlay(membership_group_ip()); + assert_eq!( + format!("{:?}", groups.groups[&key]), + group_before, + "{context}" + ); + assert!(!groups.pending_repairs.contains(&key), "{context}"); + assert!(logs.entries.lock().unwrap().is_empty(), "{context}"); + } + } + + #[test] + fn test_underlay_rollback_keeps_decap() { + #[derive(Clone, Copy, Debug)] + enum RestoreFailure { + Bitmap, + Ports, + Replication, + } + + for (narrow_applied, failure) in [ + (false, RestoreFailure::Replication), + (true, RestoreFailure::Replication), + (true, RestoreFailure::Ports), + (true, RestoreFailure::Bitmap), + ] { + let context = + format!("narrow_applied={narrow_applied}, failure={failure:?}"); + let [removed, underlay, added] = membership_members(); + let members = [removed, underlay.clone()]; + let new_members = [underlay, added]; + let (switch, logs) = chaos_switch(Config::default()); + create_membership_group(&switch, &members); + let original = switch.mcast.lock().unwrap().groups + [&GroupKey::underlay(membership_group_ip())] + .clone(); + let repl = original.replication_info().unwrap(); + let external_id = original.external_group_id(); + let union_bitmap = create_port_bitmap( + members.iter().chain(&new_members), + Direction::External, + ); + let new_bitmap = + create_port_bitmap(&new_members, Direction::External); + + table::mcast::mcast_egress::set_bitmap_entry( + &switch, + external_id, + &union_bitmap, + None, + ) + .unwrap(); + let mut added_ports = Vec::new(); + let mut removed_ports = Vec::new(); + process_membership_changes( + &switch, + &new_members, + &original, + repl, + &mut added_ports, + &mut removed_ports, + ) + .unwrap(); + + set_replication_entry(&switch, &original, repl).unwrap(); + if narrow_applied { + table::mcast::mcast_egress::set_bitmap_entry( + &switch, + external_id, + &new_bitmap, + None, + ) + .unwrap(); + } + + let repl_before = table_usage(&switch, TableType::McastIpv6); + let (op, failed_call) = match failure { + RestoreFailure::Bitmap => { + switch.asic_hdl.inject_table_faults( + TableType::McastEgressDecapPorts, + TableOp::EntryUpdate, + CallRange::nth(1), + AsicError::Synthetic, + ); + ("restore decap bitmap", "table_entry_update") + } + RestoreFailure::Ports => { + switch.asic_hdl.inject_mc_faults( + McOp::McPortRemove, + CallRange::nth(1), + ); + ("port changes", "mc_port_remove") + } + RestoreFailure::Replication => { + switch.asic_hdl.inject_table_faults( + TableType::McastIpv6, + TableOp::EntryUpdate, + CallRange::nth(1), + AsicError::Synthetic, + ); + ("restore replication settings", "table_entry_update") + } + }; + + let err = GroupUpdateRollbackContext::new(&switch, &original, None) + .rollback_underlay_and_restore( + DpdError::Switch(AsicError::Synthetic( + "table_entry_update".to_string(), + )), + repl, + &added_ports, + &removed_ports, + ); + let DpdError::Unwind { initial, unwind } = &err else { + panic!("{context}: expected a failed unwind: {err:?}"); + }; + assert!( + matches!( + initial.as_ref(), + DpdError::Switch(AsicError::Synthetic(op)) + if op == "table_entry_update" + ), + "{context}: {err:?}" + ); + assert_eq!(unwind.len(), 1, "{context}: {err:?}"); + assert!( + matches!( + &unwind.head, + DpdError::Switch(AsicError::Synthetic(op)) + if op == failed_call + ), + "{context}: {err:?}" + ); + + let expected_bitmap = match failure { + RestoreFailure::Bitmap => new_bitmap, + _ => union_bitmap.clone(), + }; + assert_eq!( + bitmap_entry(&switch, external_id), + Some((expected_bitmap, None)), + "{context}" + ); + let expected_members: Vec<_> = match failure { + RestoreFailure::Bitmap => new_members[..1].to_vec(), + RestoreFailure::Ports => { + members.iter().chain(&new_members[1..]).cloned().collect() + } + RestoreFailure::Replication => members.to_vec(), + }; + assert_eq!( + asic_membership(&switch), + expected_membership(&switch, &expected_members), + "{context}" + ); + let repl_updates = + u64::from(matches!(failure, RestoreFailure::Ports)); + assert_eq!( + table_usage(&switch, TableType::McastIpv6), + ( + repl_before.0, + repl_before.1, + repl_before.2, + repl_before.3 + repl_updates, + ), + "{context}" + ); + { + let errors = logs.entries.lock().unwrap(); + assert_eq!(errors.len(), 1, "{context}: {errors:?}"); + assert_eq!(errors[0]["operation"], op, "{context}"); + assert!( + errors[0]["error"].contains(failed_call), + "{context}: {errors:?}" + ); + } + + let mut pending = original.clone(); + let GroupKind::Underlay { replication, .. } = &mut pending.kind + else { + panic!("expected an underlay group"); + }; + replication.add_members(repl.clone(), &new_members); + switch.asic_hdl.inject_table_faults( + TableType::McastIpv6, + TableOp::EntryUpdate, + CallRange::nth(1), + AsicError::Synthetic, + ); + assert!( + resync_after_failed_unwind(&switch, &pending, None).is_none(), + "{context}" + ); + assert_eq!( + bitmap_entry(&switch, external_id), + Some((union_bitmap, None)), + "{context}" + ); + } + } + #[test] fn test_underlay_write_removes_member_left_by_failed_unwind() { let [kept_member, added_member, failing_member] = membership_members(); @@ -5437,6 +5818,7 @@ mod chaos_tests { failed_table, TableOp::EntryDel, CallRange::starting_at(1), + AsicError::Synthetic, ); fail_unwind_to_empty_group(&switch); @@ -5477,16 +5859,17 @@ mod chaos_tests { fn external_member_put_with_failed_unwind( init_members: &[MulticastGroupMember], tgt_members: &[MulticastGroupMember], - decap_update_failures: CallRange, + update_failures: CallRange, before_put: impl FnOnce(&Switch), ) -> (Switch, RollbackLog) { let (switch, logs) = chaos_switch(Config::default()); create_membership_group(&switch, init_members); before_put(&switch); switch.asic_hdl.inject_table_faults( - TableType::McastEgressDecapPorts, + TableType::McastIpv6, TableOp::EntryUpdate, - decap_update_failures, + update_failures, + AsicError::Synthetic, ); let removals = init_members .iter() @@ -5531,7 +5914,7 @@ mod chaos_tests { let (switch, logs) = external_member_put_with_failed_unwind( &init_members, &tgt_members, - CallRange::nth(2), + CallRange::nth(1), |_| {}, ); let external_id = membership_group_id(&switch, Direction::External); @@ -5555,10 +5938,9 @@ mod chaos_tests { } fn clear_external_member_put_faults(switch: &Switch) { - switch.asic_hdl.clear_table_faults( - TableType::McastEgressDecapPorts, - TableOp::EntryUpdate, - ); + switch + .asic_hdl + .clear_table_faults(TableType::McastIpv6, TableOp::EntryUpdate); clear_membership_faults(switch); } @@ -5568,7 +5950,7 @@ mod chaos_tests { let (switch, logs) = external_member_put_with_failed_unwind( &init_members, &tgt_members, - CallRange::starting_at(2), + CallRange::starting_at(1), |_| {}, ); @@ -5587,7 +5969,7 @@ mod chaos_tests { let (switch, logs) = external_member_put_with_failed_unwind( &init_members, &tgt_members, - CallRange::starting_at(2), + CallRange::starting_at(1), |switch| { add_group_external( switch, @@ -5633,7 +6015,7 @@ mod chaos_tests { let (switch, logs) = external_member_put_with_failed_unwind( &init_members, &tgt_members, - CallRange::starting_at(2), + CallRange::starting_at(1), |switch| { if group_destroy_fails { switch.asic_hdl.inject_mc_faults( @@ -5645,6 +6027,7 @@ mod chaos_tests { TableType::RouteIpv6Mcast, TableOp::EntryDel, CallRange::nth(1), + AsicError::Synthetic, ); } }, @@ -5689,73 +6072,45 @@ mod chaos_tests { let [kept_member, _, _] = membership_members(); let tgt_members = [kept_member]; - for readback in [true, false] { - let mut config = Config::default(); - if !readback { - config - .asic_config - .mc_config - .unsupported - .insert(McOp::McPortList); - } - let (switch, logs) = chaos_switch(config); - create_membership_group(&switch, &[]); - let external_id = membership_group_id(&switch, Direction::External); - assert_eq!(table_usage(&switch, TableType::McastIpv6).0, 0); + let (switch, logs) = chaos_switch(Config::default()); + create_membership_group(&switch, &[]); + let external_id = membership_group_id(&switch, Direction::External); + assert_eq!(table_usage(&switch, TableType::McastIpv6).0, 0); - switch.asic_hdl.inject_table_faults( - TableType::McastIpv6, - TableOp::EntryAdd, - CallRange::nth(1), - ); - switch - .asic_hdl - .inject_mc_faults(McOp::McPortRemove, CallRange::nth(1)); - let err = put_membership(&switch, &tgt_members).unwrap_err(); - let DpdError::Unwind { initial, unwind } = &err else { - panic!("expected a failed unwind: {err:?}"); - }; - assert!( - matches!( - initial.as_ref(), - DpdError::Switch(AsicError::Synthetic(op)) - if op == "table_entry_add" - ), - "{err:?}" - ); - assert!(is_synthetic(&unwind.head, McOp::McPortRemove), "{err:?}"); + switch.asic_hdl.inject_table_faults( + TableType::McastIpv6, + TableOp::EntryAdd, + CallRange::nth(1), + AsicError::Synthetic, + ); + switch.asic_hdl.inject_mc_faults(McOp::McPortRemove, CallRange::nth(1)); + let err = put_membership(&switch, &tgt_members).unwrap_err(); - if readback { - assert_eq!( - asic_membership(&switch), - expected_membership(&switch, &tgt_members), - ); - } else { - assert_eq!( - [Direction::External, Direction::Underlay].map( - |direction| switch - .asic_hdl - .mc_port_count(membership_group_id( - &switch, direction, - )) - .unwrap(), - ), - expected_membership(&switch, &tgt_members) - .map(|members| members.len()), - ); - } - assert_eq!(stored_membership(&switch), tgt_members.to_vec()); - assert!(switch.mcast.lock().unwrap().pending_repairs.is_empty()); - assert_eq!(table_usage(&switch, TableType::McastIpv6).0, 1); - assert_eq!( - bitmap_entry(&switch, external_id), - Some(( - create_port_bitmap(&tgt_members, Direction::External), - None, - )), - ); - assert_eq!(logs.entries.lock().unwrap().len(), 1); - } + assert_eq!( + asic_membership(&switch), + expected_membership(&switch, &tgt_members), + ); + assert_eq!(stored_membership(&switch), tgt_members.to_vec()); + assert!(switch.mcast.lock().unwrap().pending_repairs.is_empty()); + assert_eq!(table_usage(&switch, TableType::McastIpv6).0, 1); + assert_eq!( + bitmap_entry(&switch, external_id), + Some((create_port_bitmap(&tgt_members, Direction::External), None)), + ); + assert_eq!(logs.entries.lock().unwrap().len(), 1); + + let DpdError::Unwind { initial, unwind } = &err else { + panic!("expected a failed unwind: {err:?}"); + }; + assert!( + matches!( + initial.as_ref(), + DpdError::Switch(AsicError::Synthetic(op)) + if op == "table_entry_add" + ), + "{err:?}" + ); + assert!(is_synthetic(&unwind.head, McOp::McPortRemove), "{err:?}"); } #[test] diff --git a/dpd/src/table/mcast/mod.rs b/dpd/src/table/mcast/mod.rs index 3d940e35..9e9013e2 100644 --- a/dpd/src/table/mcast/mod.rs +++ b/dpd/src/table/mcast/mod.rs @@ -42,17 +42,24 @@ where M: MatchParse + Hash + fmt::Display, A: ActionParse, { - let entries = match s - .table_get(table)? - .get_entries::(&s.asic_hdl, false) - { - Ok(entries) => entries, - Err(DpdError::Switch(AsicError::OperationUnsupported)) => { - debug!(s.log, "{table} entry listing unsupported, skip clearing"); - return Ok(()); - } - Err(e) => return Err(e), - }; + let entries = + match s.table_get(table)?.get_entries::(&s.asic_hdl, false) { + Ok(entries) => entries, + // TODO: SoftNPU currently doesn't support per-table dumps. + // + // Its dump request returns all tables, and the UART reader can't + // reliably handle large responses. Add per-table dumps and read complete + // UART responses before removing the `OperationUnsupported` fallback. + Err(DpdError::Switch(AsicError::OperationUnsupported)) => { + debug!( + s.log, + "per-table dump unsupported, skip clearing"; + "table" => %table, + ); + return Ok(()); + } + Err(e) => return Err(e), + }; let keys: Vec = entries .into_iter() From f88550526a96b49c1fb0b48ee2e13195bdc4af08 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Mon, 9 Nov 2026 07:46:02 +0000 Subject: [PATCH 9/9] [deps] newly built softnpu --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a7353abf..c74840d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7302,7 +7302,7 @@ dependencies = [ [[package]] name = "softnpu" version = "0.2.0" -source = "git+https://github.com/oxidecomputer/softnpu?branch=zl%2Ftable-clear#618a149ff169ed8a9c1620a3b1a2410f5cf5d53b" +source = "git+https://github.com/oxidecomputer/softnpu?branch=zl%2Ftable-clear#63687602f0d26add71d4dae4ac62d46563ba23f5" dependencies = [ "p4rs 0.1.0 (git+https://github.com/oxidecomputer/p4?branch=zl%2Fupsert-table-entries)", "serde",