From 8c9c42be75223fce6c4428be96367a5af731e2c2 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 27 Feb 2026 01:18:21 +0000 Subject: [PATCH 1/6] [mcast, feature] remove multicast feature; make it truly default This removes the multicast feature gating altogether, making multicast inclusive to the work in dendrite/dpd/sidecar. --- .github/buildomat/jobs/multicast-test.sh | 34 -- .github/buildomat/packet-test-common.sh | 16 +- aal/Cargo.toml | 3 - aal/src/lib.rs | 1 - asic/Cargo.toml | 1 - asic/src/chaos/mod.rs | 9 - asic/src/softnpu/mod.rs | 2 - asic/src/tofino_asic/mod.rs | 15 - asic/src/tofino_common/mod.rs | 13 - asic/src/tofino_stub/mod.rs | 14 - common/Cargo.toml | 3 - common/src/counters.rs | 55 +-- common/src/table.rs | 67 +-- dpd-client/Cargo.toml | 1 - dpd-client/tests/integration_tests/common.rs | 5 - dpd-client/tests/integration_tests/mod.rs | 1 - .../tests/integration_tests/table_tests.rs | 16 - dpd/Cargo.toml | 1 - dpd/src/api_server.rs | 426 +++++++----------- dpd/src/counters.rs | 35 +- dpd/src/link.rs | 3 - dpd/src/macaddrs.rs | 2 - dpd/src/main.rs | 3 - dpd/src/mcast/mod.rs | 11 +- dpd/src/table/mac.rs | 5 - dpd/src/table/mod.rs | 21 - dpd/src/types.rs | 2 - swadm/Cargo.toml | 3 - swadm/tests/cli/counters.rs | 4 - swadm/tests/cli/expect/counters.txt | 2 +- swadm/tests/cli/expect/counters_multicast.txt | 1 - xtask/src/codegen.rs | 7 +- xtask/src/main.rs | 8 +- 33 files changed, 229 insertions(+), 561 deletions(-) delete mode 100755 .github/buildomat/jobs/multicast-test.sh delete mode 100644 swadm/tests/cli/expect/counters_multicast.txt diff --git a/.github/buildomat/jobs/multicast-test.sh b/.github/buildomat/jobs/multicast-test.sh deleted file mode 100755 index a6e1669f..00000000 --- a/.github/buildomat/jobs/multicast-test.sh +++ /dev/null @@ -1,34 +0,0 @@ -#!/bin/bash -#: -#: name = "multicast-test" -#: variety = "basic" -#: target = "ubuntu-22.04" -#: rust_toolchain = true -#: -#: output_rules = [ -#: "/work/simulator.log", -#: "/work/dpd.log", -#: ] -#: - -#### >>>>>>>>>>>>>>>>>>>>>>>>>>>> Local Usage >>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -#### -#### The following environment variables are useful. -#### -#### - JUST_TEST=1 Just runs the tests, skipping system prep. -#### - TESTNAME='$name' Will just run the specified test. -#### - STARTUP_TIMEOUT=n Seconds to wait for tofino-model/dpd to start. -#### Defaults to 15. -#### - NOBUILD=1 Don't build sidecar.p4 (in case you've already -#### built it) -#### -#### <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< - -export RUST_BACKTRACE=1 - -set -o errexit -set -o pipefail -set -o xtrace - -export MULTICAST=1 -source .github/buildomat/packet-test-common.sh diff --git a/.github/buildomat/packet-test-common.sh b/.github/buildomat/packet-test-common.sh index 44855110..e72d1fb0 100755 --- a/.github/buildomat/packet-test-common.sh +++ b/.github/buildomat/packet-test-common.sh @@ -10,15 +10,7 @@ STARTUP_TIMEOUT=${STARTUP_TIMEOUT:=120} DENDRITE_TEST_HOST=${DENDRITE_TEST_HOST:="[::1]"} DENDRITE_TEST_VERBOSITY=${DENDRITE_TEST_VERBOSITY:=3} -if [ "$MULTICAST" == "" ]; then - BUILD_FEATURES=tofino_asic - CODEGEN_FEATURES=() - SWADM_FEATURES=() -else - BUILD_FEATURES=tofino_asic,multicast - CODEGEN_FEATURES=(--multicast) - SWADM_FEATURES=(--features=multicast) -fi +BUILD_FEATURES=tofino_asic function cleanup { set +o errexit @@ -64,7 +56,7 @@ export SDE=/opt/oxide/tofino_sde banner "Build" if [[ $NOBUILD -ne 1 ]]; then cargo build --features=$BUILD_FEATURES --bin dpd --bin swadm - cargo xtask codegen --stages "$TOFINO_STAGES" "${CODEGEN_FEATURES[@]}" + cargo xtask codegen --stages "$TOFINO_STAGES" fi banner "Test" @@ -125,12 +117,10 @@ pushd swadm cargo test \ --no-fail-fast \ - "${SWADM_FEATURES[@]}" \ -- \ --ignored cargo test \ - --no-fail-fast \ - "${SWADM_FEATURES[@]}" + --no-fail-fast popd diff --git a/aal/Cargo.toml b/aal/Cargo.toml index 81ecdb6f..d2c6a656 100644 --- a/aal/Cargo.toml +++ b/aal/Cargo.toml @@ -3,9 +3,6 @@ name = "aal" version = "0.1.0" edition = "2024" -[features] -multicast = [] - [dependencies] common.workspace = true dpd-types.workspace = true diff --git a/aal/src/lib.rs b/aal/src/lib.rs index 3fa1ea56..36b0bf75 100644 --- a/aal/src/lib.rs +++ b/aal/src/lib.rs @@ -119,7 +119,6 @@ pub enum AsicError { /// The `AsicMulticastOps` trait contains the multicast-related ASIC operations /// that the dataplane daemon requires. -#[cfg(feature = "multicast")] pub trait AsicMulticastOps { /// Return a vector containing all of the defined multicast groups. fn mc_domains(&self) -> Vec; diff --git a/asic/Cargo.toml b/asic/Cargo.toml index 96d42164..9cb9e05b 100644 --- a/asic/Cargo.toml +++ b/asic/Cargo.toml @@ -13,7 +13,6 @@ tofino_asic = [ tofino_stub = [] softnpu = ["softnpu-lib", "dep:propolis"] chaos = [] -multicast = ["aal/multicast", "common/multicast"] interrupts = [] [lib] diff --git a/asic/src/chaos/mod.rs b/asic/src/chaos/mod.rs index e5d58831..ccf2deed 100644 --- a/asic/src/chaos/mod.rs +++ b/asic/src/chaos/mod.rs @@ -11,7 +11,6 @@ use std::collections::HashMap; use std::sync::Mutex; use tokio::sync::mpsc; -#[cfg(feature = "multicast")] use aal::AsicMulticastOps; use aal::{ AsicError, AsicId, AsicOps, AsicResult, Connector, PortHdl, PortUpdate, @@ -117,7 +116,6 @@ impl TableChaos { ], v, ); - #[cfg(feature = "multicast")] tc.add_tables( vec![ TableType::RouteIpv4Mcast, @@ -154,7 +152,6 @@ impl TableChaos { } } -#[cfg(feature = "multicast")] #[derive(Debug, Default, Serialize, Deserialize, Clone)] pub struct AsicMulticastConfig { pub mc_port_count: Chaos, @@ -194,7 +191,6 @@ pub struct AsicConfig { pub table_entry_add: TableChaos, pub table_entry_update: TableChaos, pub table_entry_del: TableChaos, - #[cfg(feature = "multicast")] pub mc_config: AsicMulticastConfig, } @@ -228,7 +224,6 @@ impl AsicConfig { table_entry_add: TableChaos::uniform(v), table_entry_update: TableChaos::uniform(v), table_entry_del: TableChaos::uniform(v), - #[cfg(feature = "multicast")] mc_config: AsicMulticastConfig { mc_port_count: Chaos::new(v), mc_port_add: Chaos::new(v), @@ -257,7 +252,6 @@ impl AsicConfig { port_enable_get: Chaos::new(v), connector_avail_channels: Chaos::new(v), get_sidecar_identifiers: Chaos::new(v), - #[cfg(feature = "multicast")] mc_config: AsicMulticastConfig { mc_port_count: Chaos::new(v), mc_groups_count: Chaos::new(v), @@ -286,7 +280,6 @@ impl AsicConfig { table_entry_add: TableChaos::uniform(v), table_entry_update: TableChaos::uniform(v), table_entry_del: TableChaos::uniform(v), - #[cfg(feature = "multicast")] mc_config: AsicMulticastConfig { mc_port_add: Chaos::new(v), mc_port_remove: Chaos::new(v), @@ -376,7 +369,6 @@ pub(crate) use unfurl; /// A convenience macro for unfurling multicast chaos. The $name should be a /// regular `Chaos` member of [`AsicMulticastConfigConfig`]. The `handle` is /// a [`Handle`] object. -#[cfg(feature = "multicast")] macro_rules! unfurl_mc { ($handle:ident, $name:ident) => { $handle @@ -396,7 +388,6 @@ macro_rules! table_unfurl { } pub(crate) use table_unfurl; -#[cfg(feature = "multicast")] impl AsicMulticastOps for Handle { fn mc_domains(&self) -> Vec { let len = self.ports.lock().unwrap().len() as u16; diff --git a/asic/src/softnpu/mod.rs b/asic/src/softnpu/mod.rs index cfb497ad..94246790 100644 --- a/asic/src/softnpu/mod.rs +++ b/asic/src/softnpu/mod.rs @@ -16,7 +16,6 @@ pub use crate::faux_fsm::FsmState; pub use crate::faux_fsm::FsmType; pub use crate::faux_fsm::PortFsmState; -#[cfg(feature = "multicast")] use aal::AsicMulticastOps; use aal::{ AsicError, AsicId, AsicOps, AsicResult, Connector, PortHdl, PortUpdate, @@ -145,7 +144,6 @@ impl Handle { pub fn fini(&self) {} } -#[cfg(feature = "multicast")] impl AsicMulticastOps for Handle { fn mc_domains(&self) -> Vec { let len = self.ports.lock().unwrap().len() as u16; diff --git a/asic/src/tofino_asic/mod.rs b/asic/src/tofino_asic/mod.rs index 18eb3eed..7953e22b 100644 --- a/asic/src/tofino_asic/mod.rs +++ b/asic/src/tofino_asic/mod.rs @@ -20,7 +20,6 @@ mod genpd; #[cfg(feature = "interrupts")] pub mod interrupt_monitor; mod link_fsm; -#[cfg(feature = "multicast")] pub mod mcast; pub mod ports; pub mod qsfp; @@ -30,7 +29,6 @@ pub mod snapshot; pub mod stats; pub mod table; -#[cfg(feature = "multicast")] use aal::AsicMulticastOps; use aal::{ AsicError, AsicOps, AsicResult, Connector, PortHdl, SidecarIdentifiers, @@ -73,7 +71,6 @@ impl Default for AsicConfig { } } -#[cfg(feature = "multicast")] impl AsicMulticastOps for Handle { fn mc_domains(&self) -> Vec { mcast::domains(self) @@ -330,7 +327,6 @@ pub struct Handle { rt: tofino_common::BfRt, log: slog::Logger, phys_ports: Mutex, - #[cfg(feature = "multicast")] domains: Mutex>, eth_connector_id: Option, } @@ -395,28 +391,18 @@ impl Handle { let p4_dir = tofino_common::get_p4_dir()?; - #[cfg(feature = "multicast")] let mut bf = bf_wrapper::bf_init( &log, &config.devpath, &p4_dir, &config.board_rev, )?; - #[cfg(not(feature = "multicast"))] - let bf = bf_wrapper::bf_init( - &log, - &config.devpath, - &p4_dir, - &config.board_rev, - )?; - let rt = tofino_common::BfRt::init(&p4_dir)?; let phys_ports = ports::init(dev_id)?; let eth_connector_id = phys_ports.eth_connector_id; // Note: we assume that bf_mc_init() has been called as part of the // bf_switch_init() operation. - #[cfg(feature = "multicast")] { bf.mcast_hdl = mcast::create_session()?; } @@ -429,7 +415,6 @@ impl Handle { rt, log: log.new(o!("unit" => "tofino_asic")), phys_ports: Mutex::new(phys_ports), - #[cfg(feature = "multicast")] domains: Mutex::new(std::collections::HashMap::new()), eth_connector_id, }) diff --git a/asic/src/tofino_common/mod.rs b/asic/src/tofino_common/mod.rs index 7e4ff064..0002c233 100644 --- a/asic/src/tofino_common/mod.rs +++ b/asic/src/tofino_common/mod.rs @@ -15,7 +15,6 @@ use aal::AsicError; use aal::AsicResult; use aal::MatchType; use common::counters::CounterId; -#[cfg(feature = "multicast")] use common::counters::MulticastCounterId; use common::table::TableType; @@ -35,11 +34,9 @@ fn table_name(type_: TableType) -> &'static str { TableType::RouteFwdIpv6 => { "pipe.Ingress.l3_router.Router6.lookup_idx.route" } - #[cfg(feature = "multicast")] TableType::RouteIpv4Mcast => { "pipe.Ingress.l3_router.MulticastRouter4.tbl" } - #[cfg(feature = "multicast")] TableType::RouteIpv6Mcast => { "pipe.Ingress.l3_router.MulticastRouter6.tbl" } @@ -58,33 +55,25 @@ fn table_name(type_: TableType) -> &'static str { TableType::AttachedSubnetIpv6 => { "pipe.Ingress.attached_subnet_ingress.attached_subnets_v6" } - #[cfg(feature = "multicast")] TableType::McastIpv6 => { "pipe.Ingress.mcast_ingress.mcast_replication_ipv6" } - #[cfg(feature = "multicast")] TableType::McastIpv4SrcFilter => { "pipe.Ingress.mcast_ingress.mcast_source_filter_ipv4" } - #[cfg(feature = "multicast")] TableType::McastIpv6SrcFilter => { "pipe.Ingress.mcast_ingress.mcast_source_filter_ipv6" } - #[cfg(feature = "multicast")] TableType::NatIngressIpv4Mcast => { "pipe.Ingress.nat_ingress.ingress_ipv4_mcast" } - #[cfg(feature = "multicast")] TableType::NatIngressIpv6Mcast => { "pipe.Ingress.nat_ingress.ingress_ipv6_mcast" } - #[cfg(feature = "multicast")] TableType::PortMacAddressMcast => "pipe.Egress.mac_rewrite.mac_rewrite", - #[cfg(feature = "multicast")] TableType::McastEgressDecapPorts => { "pipe.Egress.mcast_egress.tbl_decap_ports" } - #[cfg(feature = "multicast")] TableType::McastEgressPortMapping => { "pipe.Egress.mcast_egress.asic_id_to_port" } @@ -100,12 +89,10 @@ fn counter_table_name(id: CounterId) -> &'static str { CounterId::Egress => "pipe.Ingress.egress_ctr", CounterId::DropPort => "pipe.Ingress.drop_port_ctr", CounterId::DropReason => "pipe.Ingress.drop_reason_ctr", - #[cfg(feature = "multicast")] CounterId::Multicast(id) => mulitcast_counter_table_name(id), } } -#[cfg(feature = "multicast")] fn mulitcast_counter_table_name(id: MulticastCounterId) -> &'static str { match id { MulticastCounterId::EgressDropPort => "pipe.Egress.drop_port_ctr", diff --git a/asic/src/tofino_stub/mod.rs b/asic/src/tofino_stub/mod.rs index 8f4224c0..42beec94 100644 --- a/asic/src/tofino_stub/mod.rs +++ b/asic/src/tofino_stub/mod.rs @@ -12,7 +12,6 @@ use tokio::sync::mpsc; use crate::Identifiers; use crate::tofino_common::*; -#[cfg(feature = "multicast")] use aal::AsicMulticastOps; use aal::{ AsicError, AsicOps, AsicResult, Connector, PortHdl, PortUpdate, @@ -24,7 +23,6 @@ pub use crate::faux_fsm::FsmState; pub use crate::faux_fsm::FsmType; pub use crate::faux_fsm::PortFsmState; -#[cfg(feature = "multicast")] pub mod mcast; pub mod ports; pub mod table; @@ -68,19 +66,15 @@ impl AsicLinkStats { } } -#[cfg(feature = "multicast")] impl AsicMulticastOps for StubHandle { - #[cfg(feature = "multicast")] fn mc_domains(&self) -> Vec { let mc_data = self.mc_data.lock().unwrap(); mc_data.domains() } - #[cfg(feature = "multicast")] fn mc_port_count(&self, group_id: u16) -> AsicResult { let mc_data = self.mc_data.lock().unwrap(); mc_data.domain_port_count(group_id) } - #[cfg(feature = "multicast")] fn mc_port_add( &self, group_id: u16, @@ -92,7 +86,6 @@ impl AsicMulticastOps for StubHandle { let mut mc_data = self.mc_data.lock().unwrap(); mc_data.domain_port_add(group_id, port, rid, level1_excl_id) } - #[cfg(feature = "multicast")] fn mc_port_remove(&self, group_id: u16, port: u16) -> AsicResult<()> { info!( self.log, @@ -101,28 +94,24 @@ impl AsicMulticastOps for StubHandle { let mut mc_data = self.mc_data.lock().unwrap(); mc_data.domain_port_remove(group_id, port) } - #[cfg(feature = "multicast")] fn mc_group_create(&self, group_id: u16) -> AsicResult<()> { info!(self.log, "creating multicast group {}", group_id); let mut mc_data = self.mc_data.lock().unwrap(); mc_data.domain_create(group_id) } - #[cfg(feature = "multicast")] fn mc_group_destroy(&self, group_id: u16) -> AsicResult<()> { info!(self.log, "destroying multicast group {}", group_id); let mut mc_data = self.mc_data.lock().unwrap(); mc_data.domain_destroy(group_id) } - #[cfg(feature = "multicast")] fn mc_groups_count(&self) -> AsicResult { info!(self.log, "number of multicast groups"); let mc_data = self.mc_data.lock().unwrap(); Ok(mc_data.domains().len()) } - #[cfg(feature = "multicast")] fn mc_set_max_nodes( &self, max_nodes: u32, @@ -270,7 +259,6 @@ pub struct StubHandle { log: slog::Logger, phys_ports: Mutex, port_state: Mutex>, - #[cfg(feature = "multicast")] mc_data: Mutex, update_tx: Mutex>>, } @@ -281,7 +269,6 @@ impl StubHandle { let rt = BfRt::init(&p4_dir)?; let phys_ports = Mutex::new(ports::init()?); let port_state = Mutex::new(BTreeMap::new()); - #[cfg(feature = "multicast")] let mc_data = Mutex::new(mcast::init()); let log = log.new(o!()); @@ -290,7 +277,6 @@ impl StubHandle { log, phys_ports, port_state, - #[cfg(feature = "multicast")] mc_data, update_tx: Mutex::new(None), }) diff --git a/common/Cargo.toml b/common/Cargo.toml index 7a062bb4..ed505213 100644 --- a/common/Cargo.toml +++ b/common/Cargo.toml @@ -4,9 +4,6 @@ version = "0.1.0" authors = ["Nils Nieuwejaar "] edition = "2024" -[features] -multicast = [] - [dependencies] anyhow.workspace = true chrono.workspace = true diff --git a/common/src/counters.rs b/common/src/counters.rs index 9d61e528..eb89eb6d 100644 --- a/common/src/counters.rs +++ b/common/src/counters.rs @@ -33,7 +33,6 @@ pub enum CounterId { Packet, DropPort, DropReason, - #[cfg(feature = "multicast")] Multicast(MulticastCounterId), } @@ -49,7 +48,6 @@ pub enum CounterId { Serialize, Deserialize, )] -#[cfg(feature = "multicast")] pub enum MulticastCounterId { EgressDropPort, EgressDropReason, @@ -73,7 +71,6 @@ impl fmt::Display for CounterId { CounterId::Packet => "Packet".to_string(), CounterId::DropPort => "Ingress_Drop_Port".to_string(), CounterId::DropReason => "Ingress_Drop_Reason".to_string(), - #[cfg(feature = "multicast")] CounterId::Multicast(id) => id.to_string(), } ) @@ -91,40 +88,32 @@ impl std::str::FromStr for CounterId { "packet" => Ok(CounterId::Packet), "ingressdropport" => Ok(CounterId::DropPort), "ingressdropreason" => Ok(CounterId::DropReason), - #[cfg(feature = "multicast")] - x => match x { - "egressdropport" => { - Ok(CounterId::Multicast(MulticastCounterId::EgressDropPort)) - } - "egressdropreason" => Ok(CounterId::Multicast( - MulticastCounterId::EgressDropReason, - )), - "unicast" => { - Ok(CounterId::Multicast(MulticastCounterId::Unicast)) - } - "multicast" => { - Ok(CounterId::Multicast(MulticastCounterId::Multicast)) - } - "multicastext" | "multicastexternal" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastExt)) - } - "multicastll" | "multicastlinklocal" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastLL)) - } - "multicastul" | "multicastunderlay" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastUL)) - } - "multicastdrop" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastDrop)) - } - x => Err(format!("No such counter: {x}")), - }, - #[cfg(not(feature = "multicast"))] + "egressdropport" => { + Ok(CounterId::Multicast(MulticastCounterId::EgressDropPort)) + } + "egressdropreason" => { + Ok(CounterId::Multicast(MulticastCounterId::EgressDropReason)) + } + "unicast" => Ok(CounterId::Multicast(MulticastCounterId::Unicast)), + "multicast" => { + Ok(CounterId::Multicast(MulticastCounterId::Multicast)) + } + "multicastext" | "multicastexternal" => { + Ok(CounterId::Multicast(MulticastCounterId::MulticastExt)) + } + "multicastll" | "multicastlinklocal" => { + Ok(CounterId::Multicast(MulticastCounterId::MulticastLL)) + } + "multicastul" | "multicastunderlay" => { + Ok(CounterId::Multicast(MulticastCounterId::MulticastUL)) + } + "multicastdrop" => { + Ok(CounterId::Multicast(MulticastCounterId::MulticastDrop)) + } x => Err(format!("No such counter: {x}")), } } } -#[cfg(feature = "multicast")] impl fmt::Display for MulticastCounterId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( diff --git a/common/src/table.rs b/common/src/table.rs index edafbb8a..a7ee364f 100644 --- a/common/src/table.rs +++ b/common/src/table.rs @@ -32,9 +32,7 @@ pub enum TableType { RouteFwdIpv4, RouteIdxIpv6, RouteFwdIpv6, - #[cfg(feature = "multicast")] RouteIpv4Mcast, - #[cfg(feature = "multicast")] RouteIpv6Mcast, ArpIpv4, NeighborIpv6, @@ -47,21 +45,13 @@ pub enum TableType { UplinkEgress, AttachedSubnetIpv4, AttachedSubnetIpv6, - #[cfg(feature = "multicast")] McastIpv6, - #[cfg(feature = "multicast")] McastIpv4SrcFilter, - #[cfg(feature = "multicast")] McastIpv6SrcFilter, - #[cfg(feature = "multicast")] NatIngressIpv4Mcast, - #[cfg(feature = "multicast")] NatIngressIpv6Mcast, - #[cfg(feature = "multicast")] PortMacAddressMcast, - #[cfg(feature = "multicast")] McastEgressDecapPorts, - #[cfg(feature = "multicast")] McastEgressPortMapping, Counter(crate::counters::CounterId), } @@ -69,7 +59,7 @@ pub enum TableType { /// Returns a vec of all the normal table types. This will not include the /// tables used to collect counter data, which are managed separately. pub fn get_table_types() -> Vec { - let mut base_tables = vec![ + vec![ TableType::RouteIdxIpv4, TableType::RouteFwdIpv4, TableType::RouteIdxIpv6, @@ -85,30 +75,17 @@ pub fn get_table_types() -> Vec { TableType::UplinkEgress, TableType::AttachedSubnetIpv4, TableType::AttachedSubnetIpv6, - ]; - - let mut multicast_tables; - #[cfg(feature = "multicast")] - { - multicast_tables = vec![ - TableType::RouteIpv4Mcast, - TableType::RouteIpv6Mcast, - TableType::McastIpv6, - TableType::McastIpv4SrcFilter, - TableType::McastIpv6SrcFilter, - TableType::NatIngressIpv4Mcast, - TableType::NatIngressIpv6Mcast, - TableType::PortMacAddressMcast, - TableType::McastEgressDecapPorts, - TableType::McastEgressPortMapping, - ]; - } - #[cfg(not(feature = "multicast"))] - { - multicast_tables = Vec::new(); - } - base_tables.append(&mut multicast_tables); - base_tables + TableType::RouteIpv4Mcast, + TableType::RouteIpv6Mcast, + TableType::McastIpv6, + TableType::McastIpv4SrcFilter, + TableType::McastIpv6SrcFilter, + TableType::NatIngressIpv4Mcast, + TableType::NatIngressIpv6Mcast, + TableType::PortMacAddressMcast, + TableType::McastEgressDecapPorts, + TableType::McastEgressPortMapping, + ] } // This is the name that will be displayed in the log and presented to the user. @@ -132,11 +109,9 @@ impl fmt::Display for TableType { TableType::RouteFwdIpv6 => { "Ingress.l3_router.Router6.lookup_idx.route".to_string() } - #[cfg(feature = "multicast")] TableType::RouteIpv4Mcast => { "Ingress.l3_router.MulticastRouter4.tbl".to_string() } - #[cfg(feature = "multicast")] TableType::RouteIpv6Mcast => { "Ingress.l3_router.MulticastRouter6.tbl".to_string() } @@ -165,34 +140,26 @@ impl fmt::Display for TableType { "Ingress.attached_subnet_ingress.attached_subnets_v6" .to_string() } - #[cfg(feature = "multicast")] TableType::McastIpv6 => { "Ingress.mcast_ingress.mcast_replication_ipv6".to_string() } - #[cfg(feature = "multicast")] TableType::McastIpv4SrcFilter => { "Ingress.mcast_ingress.mcast_source_filter_ipv4".to_string() } - #[cfg(feature = "multicast")] TableType::McastIpv6SrcFilter => { "Ingress.mcast_ingress.mcast_source_filter_ipv6".to_string() } - #[cfg(feature = "multicast")] TableType::NatIngressIpv4Mcast => { "Ingress.nat_ingress.ingress_ipv4_mcast".to_string() } - #[cfg(feature = "multicast")] TableType::NatIngressIpv6Mcast => { "Ingress.nat_ingress.ingress_ipv6_mcast".to_string() } - #[cfg(feature = "multicast")] TableType::PortMacAddressMcast => "Egress.mac_rewrite.mac_rewrite".to_string(), - #[cfg(feature = "multicast")] TableType::McastEgressDecapPorts => { "Egress.mcast_egress.tbl_decap_ports".to_string() } - #[cfg(feature = "multicast")] TableType::McastEgressPortMapping => { "Egress.mcast_egress.asic_id_to_port".to_string() } @@ -223,12 +190,10 @@ impl TryFrom<&str> for TableType { "ingress.l3_router.router6.lookup_idx.route" => { Ok(TableType::RouteFwdIpv6) } - #[cfg(feature = "multicast")] "ingress.l3_router.multicastrouter4.tbl" => { Ok(TableType::RouteIpv4Mcast) } - #[cfg(feature = "multicast")] "ingress.l3_router.multicastrouter6.tbl" => { Ok(TableType::RouteIpv6Mcast) } @@ -251,41 +216,33 @@ impl TryFrom<&str> for TableType { Ok(TableType::AttachedSubnetIpv6) } - #[cfg(feature = "multicast")] "ingress.mcast_ingress.mcast_replication_ipv6" => { Ok(TableType::McastIpv6) } - #[cfg(feature = "multicast")] "ingress.mcast_ingress.mcast_source_filter_ipv4" => { Ok(TableType::McastIpv4SrcFilter) } - #[cfg(feature = "multicast")] "ingress.mcast_ingress.mcast_source_filter_ipv6" => { Ok(TableType::McastIpv6SrcFilter) } - #[cfg(feature = "multicast")] "ingress.nat_ingress.ingress_ipv4_mcast" => { Ok(TableType::NatIngressIpv4Mcast) } - #[cfg(feature = "multicast")] "ingress.nat_ingress.ingress_ipv6_mcast" => { Ok(TableType::NatIngressIpv6Mcast) } - #[cfg(feature = "multicast")] "egress.mac_rewrite.mac_rewrite" => { Ok(TableType::PortMacAddressMcast) } - #[cfg(feature = "multicast")] "egress.mcast_egress.tbl_decap_ports" => { Ok(TableType::McastEgressDecapPorts) } - #[cfg(feature = "multicast")] "egress.mcast_egress.asic_id_to_port" => { Ok(TableType::McastEgressPortMapping) } diff --git a/dpd-client/Cargo.toml b/dpd-client/Cargo.toml index 7998da4d..1cba04cf 100644 --- a/dpd-client/Cargo.toml +++ b/dpd-client/Cargo.toml @@ -5,7 +5,6 @@ edition = "2024" description = "Client library for the Dendrite data plane daemon" [features] -multicast = ["asic/multicast"] chaos = ["asic/chaos"] tofino_asic = ["asic/tofino_asic"] diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 3d50f82a..39790a29 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -578,7 +578,6 @@ impl Switch { /// Return the port label for the given physical port, useful for /// counter information. - #[cfg(feature = "multicast")] pub fn port_label(&self, phys_port: PhysPort) -> Option { let idx: usize = phys_port.into(); if phys_port == NO_PORT { @@ -933,13 +932,11 @@ pub fn gen_udp_routed_pair( #[derive(Copy, Clone, Debug, Hash, Eq, PartialEq, Ord, PartialOrd)] pub enum OxideGeneveOption { External, - #[cfg(feature = "multicast")] Multicast(u8), Mss(u32), } /// Build a Geneve packet with a possible multicast tag. -#[cfg(feature = "multicast")] pub fn gen_geneve_packet_with_mcast_tag( src: Endpoint, dst: Endpoint, @@ -1014,7 +1011,6 @@ pub fn gen_geneve_packet( 0x00, ]); } - #[cfg(feature = "multicast")] OxideGeneveOption::Multicast(tag) if *tag < 3 => { #[rustfmt::skip] geneve.options.extend_from_slice(&[ @@ -1049,7 +1045,6 @@ pub fn gen_geneve_packet( ]); geneve.options.extend_from_slice(&mss.to_be_bytes()[..]); } - #[cfg(feature = "multicast")] _ => { panic!("illegal specification for option: {opt:?}") } diff --git a/dpd-client/tests/integration_tests/mod.rs b/dpd-client/tests/integration_tests/mod.rs index 29c5f2e6..7f1522a8 100644 --- a/dpd-client/tests/integration_tests/mod.rs +++ b/dpd-client/tests/integration_tests/mod.rs @@ -10,7 +10,6 @@ mod counters; mod geneve; mod icmp_ipv4; mod loopback; -#[cfg(feature = "multicast")] mod mcast; mod nat; mod port_api; diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index f764ad15..5b31ba45 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -4,7 +4,6 @@ // // Copyright 2026 Oxide Computer Company -#[cfg(feature = "multicast")] use std::net::IpAddr; use std::net::Ipv4Addr; use std::net::Ipv6Addr; @@ -42,17 +41,8 @@ use crate::integration_tests::common::prelude::*; // investigating. If it only changes by an entry or two, it's fine to just // adjust the constant below to match the observed result. // -// TODO: Multicast drops IPv4 LPM capacity to 7164 (from 8187) due to -// ingress TCAM pressure. Investigate moving MulticastRouter4/6 into the -// egress pipeline to reclaim capacity. -#[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 7164; // ipv4 forwarding table -#[cfg(not(feature = "multicast"))] const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table -#[cfg(feature = "multicast")] -const IPV6_LPM_SIZE: usize = 1023; // ipv6 forwarding table -#[cfg(not(feature = "multicast"))] const IPV6_LPM_SIZE: usize = 8191; // ipv6 forwarding table const SWITCH_IPV4_ADDRS_SIZE: usize = 511; // ipv4 addrs assigned to our ports @@ -63,9 +53,7 @@ const IPV4_ARP_SIZE: usize = 512; // arp cache const IPV6_NEIGHBOR_SIZE: usize = 512; // ipv6 neighbor cache /// The size of the multicast table related to replication on /// admin-local (internal) multicast groups. -#[cfg(feature = "multicast")] const MULTICAST_TABLE_SIZE: usize = 1024; -#[cfg(feature = "multicast")] const MCAST_TAG: &str = "mcast_table_test"; // multicast group tag // The result of a table insert or delete API operation. @@ -90,7 +78,6 @@ fn gen_ipv6_cidr(idx: usize) -> Ipv6Net { } // Generates valid IPv6 multicast addresses that are admin-local (scope 4). -#[cfg(feature = "multicast")] fn gen_ipv6_multicast_addr(idx: usize) -> Ipv6Addr { // Use admin-local multicast addresses (ff04::/16) // This ensures they will be created as internal groups @@ -480,9 +467,7 @@ impl TableTest for RouteV6 { async fn test_routev6_full() -> TestResult { test_table_capacity::(IPV6_LPM_SIZE).await } -#[cfg(feature = "multicast")] struct MulticastReplicationTableTest {} -#[cfg(feature = "multicast")] impl TableTest for MulticastReplicationTableTest { @@ -538,7 +523,6 @@ impl TableTest } } -#[cfg(feature = "multicast")] #[tokio::test] #[ignore] async fn test_multicast_replication_table_full() -> TestResult { diff --git a/dpd/Cargo.toml b/dpd/Cargo.toml index 05726534..e0dc4f90 100644 --- a/dpd/Cargo.toml +++ b/dpd/Cargo.toml @@ -5,7 +5,6 @@ authors = ["nils "] edition = "2024" [features] -multicast = ["aal/multicast", "asic/multicast"] tofino_asic = ["asic/tofino_asic"] tofino_stub = ["asic/tofino_stub"] softnpu = ["asic/softnpu"] diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index 5e108a04..e49a0198 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -30,7 +30,6 @@ use dpd_types::link::{ LinkIpv6Path, LinkPath, LinkUpCounter, LinkView, MsDuration, TfportData, }; use dpd_types::loopback::{LoopbackIpv4Path, LoopbackIpv6Path}; -#[cfg(feature = "multicast")] use dpd_types::mcast::UnderlayMulticastIpv6; use dpd_types::mcast::{ MulticastGroupCreateExternalEntry, MulticastGroupCreateUnderlayEntry, @@ -100,7 +99,6 @@ use common::ports::TxEqSwHw; use crate::attached_subnet; use crate::counters; -#[cfg(feature = "multicast")] use crate::mcast; use crate::nat; use crate::oxstats; @@ -127,33 +125,6 @@ fn client_error(message: impl ToString) -> HttpError { ) } -// Generate a 501 client error with the provided message. -#[cfg(not(feature = "multicast"))] -fn not_implemented(message: impl ToString) -> HttpError { - HttpError { - status_code: dropshot::ErrorStatusCode::NOT_IMPLEMENTED, - error_code: None, - external_message: message.to_string(), - internal_message: message.to_string(), - headers: None, - } -} - -// When the multicast feature is enabled, run the provided code. If -// multicast is no enabled, return 501 Not Implemented to the caller. -macro_rules! require_multicast { - ($body:expr) => {{ - #[cfg(feature = "multicast")] - { - $body - } - #[cfg(not(feature = "multicast"))] - { - Err(not_implemented("multicast feature disabled")) - } - }}; -} - pub enum DpdApiImpl {} impl DpdApi for DpdApiImpl { @@ -1746,7 +1717,6 @@ impl DpdApi for DpdApiImpl { error!(switch.log, "failed to reset ipv6 nat table: {:?}", e); err = Some(e); } - #[cfg(feature = "multicast")] if let Err(e) = mcast::reset(switch) { error!(switch.log, "failed to reset multicast state: {:?}", e); err = Some(e); @@ -1999,128 +1969,106 @@ impl DpdApi for DpdApiImpl { .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_create_external( rqctx: RequestContext>, group: TypedBody, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let entry = group.into_inner(); + let switch: &Switch = rqctx.context(); + let entry = group.into_inner(); - mcast::add_group_external(switch, entry) - .map(HttpResponseCreated) - .map_err(HttpError::from) - }) + mcast::add_group_external(switch, entry) + .map(HttpResponseCreated) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_create_underlay( rqctx: RequestContext>, group: TypedBody, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let entry = group.into_inner(); + let switch: &Switch = rqctx.context(); + let entry = group.into_inner(); - mcast::add_group_internal(switch, entry) - .map(HttpResponseCreated) - .map_err(HttpError::from) - }) + mcast::add_group_internal(switch, entry) + .map(HttpResponseCreated) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_delete( rqctx: RequestContext>, path: Path, query: Query, ) -> Result { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let ip = path.into_inner().group_ip; - let tag = query.into_inner().tag; + let switch: &Switch = rqctx.context(); + let ip = path.into_inner().group_ip; + let tag = query.into_inner().tag; - mcast::del_group(switch, ip, tag.as_ref()) - .map(|_| HttpResponseDeleted()) - .map_err(HttpError::from) - }) + mcast::del_group(switch, ip, tag.as_ref()) + .map(|_| HttpResponseDeleted()) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_delete_v1( rqctx: RequestContext>, path: Path, ) -> Result { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let ip = path.into_inner().group_ip; + let switch: &Switch = rqctx.context(); + let ip = path.into_inner().group_ip; - let existing_tag = mcast::get_group(switch, ip) - .map_err(HttpError::from)? - .tag() - .to_string(); + let existing_tag = mcast::get_group(switch, ip) + .map_err(HttpError::from)? + .tag() + .to_string(); - mcast::del_group(switch, ip, &existing_tag) - .map(|_| HttpResponseDeleted()) - .map_err(HttpError::from) - }) + mcast::del_group(switch, ip, &existing_tag) + .map(|_| HttpResponseDeleted()) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_reset( rqctx: RequestContext>, ) -> Result { - require_multicast!({ - let switch: &Switch = rqctx.context(); + let switch: &Switch = rqctx.context(); - mcast::reset(switch) - .map(|_| HttpResponseDeleted()) - .map_err(HttpError::from) - }) + mcast::reset(switch) + .map(|_| HttpResponseDeleted()) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_get( rqctx: RequestContext>, path: Path, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let ip = path.into_inner().group_ip; + let switch: &Switch = rqctx.context(); + let ip = path.into_inner().group_ip; - // Get the multicast group - mcast::get_group(switch, ip) - .map(HttpResponseOk) - .map_err(HttpError::from) - }) + // Get the multicast group + mcast::get_group(switch, ip) + .map(HttpResponseOk) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_update_underlay( rqctx: RequestContext>, path: Path, query: Query, group: TypedBody, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let admin_scoped = path.into_inner().group_ip; - let tag = query.into_inner().tag; - - mcast::modify_group_internal( - switch, - admin_scoped, - tag.as_ref(), - group.into_inner(), - ) - .map(HttpResponseOk) - .map_err(HttpError::from) - }) + let switch: &Switch = rqctx.context(); + let admin_scoped = path.into_inner().group_ip; + let tag = query.into_inner().tag; + + mcast::modify_group_internal( + switch, + admin_scoped, + tag.as_ref(), + group.into_inner(), + ) + .map(HttpResponseOk) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_update_underlay_v1( rqctx: RequestContext>, path: Path, @@ -2129,68 +2077,59 @@ impl DpdApi for DpdApiImpl { HttpResponseOk, HttpError, > { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let admin_scoped = path.into_inner().group_ip; - let underlay = UnderlayMulticastIpv6::try_from(admin_scoped) - .map_err(|e| { - HttpError::for_bad_request( - None, - format!("invalid group_ip: {e}"), - ) - })?; - let mut entry = group.into_inner(); - - let tag = match entry.tag.take() { - Some(t) => t, - None => { - mcast::get_group_internal(switch, underlay) - .map_err(HttpError::from)? - .tag - } - }; + let switch: &Switch = rqctx.context(); + let admin_scoped = path.into_inner().group_ip; + let underlay = + UnderlayMulticastIpv6::try_from(admin_scoped).map_err(|e| { + HttpError::for_bad_request( + None, + format!("invalid group_ip: {e}"), + ) + })?; + let mut entry = group.into_inner(); + + let tag = match entry.tag.take() { + Some(t) => t, + None => { + mcast::get_group_internal(switch, underlay) + .map_err(HttpError::from)? + .tag + } + }; - mcast::modify_group_internal(switch, underlay, &tag, entry.into()) - .map(|resp| HttpResponseOk(resp.into())) - .map_err(HttpError::from) - }) + mcast::modify_group_internal(switch, underlay, &tag, entry.into()) + .map(|resp| HttpResponseOk(resp.into())) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_get_underlay( rqctx: RequestContext>, path: Path, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let underlay = path.into_inner().group_ip; + let switch: &Switch = rqctx.context(); + let underlay = path.into_inner().group_ip; - mcast::get_group_internal(switch, underlay) - .map(HttpResponseOk) - .map_err(HttpError::from) - }) + mcast::get_group_internal(switch, underlay) + .map(HttpResponseOk) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_update_external( rqctx: RequestContext>, path: Path, query: Query, group: TypedBody, ) -> Result, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let entry = group.into_inner(); - let ip = path.into_inner().group_ip; - let tag = query.into_inner().tag; + let switch: &Switch = rqctx.context(); + let entry = group.into_inner(); + let ip = path.into_inner().group_ip; + let tag = query.into_inner().tag; - mcast::modify_group_external(switch, ip, tag.as_ref(), entry) - .map(HttpResponseOk) - .map_err(HttpError::from) - }) + mcast::modify_group_external(switch, ip, tag.as_ref(), entry) + .map(HttpResponseOk) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_update_external_v7( rqctx: RequestContext>, path: Path, @@ -2199,26 +2138,23 @@ impl DpdApi for DpdApiImpl { HttpResponseCreated, HttpError, > { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let ip = path.into_inner().group_ip; - let mut entry = group.into_inner(); - - let tag = match entry.tag.take() { - Some(t) => t, - None => mcast::get_group(switch, ip) - .map_err(HttpError::from)? - .tag() - .to_string(), - }; + let switch: &Switch = rqctx.context(); + let ip = path.into_inner().group_ip; + let mut entry = group.into_inner(); + + let tag = match entry.tag.take() { + Some(t) => t, + None => mcast::get_group(switch, ip) + .map_err(HttpError::from)? + .tag() + .to_string(), + }; - mcast::modify_group_external(switch, ip, &tag, entry.into()) - .map(|resp| HttpResponseCreated(resp.into())) - .map_err(HttpError::from) - }) + mcast::modify_group_external(switch, ip, &tag, entry.into()) + .map(|resp| HttpResponseCreated(resp.into())) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_group_update_external_v1( rqctx: RequestContext>, path: Path, @@ -2227,32 +2163,29 @@ impl DpdApi for DpdApiImpl { HttpResponseCreated, HttpError, > { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let ip = path.into_inner().group_ip; - let mut entry = group.into_inner(); - - let tag = match entry.tag.take() { - Some(t) => t, - None => mcast::get_group(switch, ip) - .map_err(HttpError::from)? - .tag() - .to_string(), - }; + let switch: &Switch = rqctx.context(); + let ip = path.into_inner().group_ip; + let mut entry = group.into_inner(); - let v7_entry = - v7::mcast::MulticastGroupUpdateExternalEntry::from(entry); - mcast::modify_group_external(switch, ip, &tag, v7_entry.into()) - .map(|resp| { - let v7_resp = - v7::mcast::MulticastGroupExternalResponse::from(resp); - HttpResponseCreated(v7_resp.into()) - }) - .map_err(HttpError::from) - }) + let tag = match entry.tag.take() { + Some(t) => t, + None => mcast::get_group(switch, ip) + .map_err(HttpError::from)? + .tag() + .to_string(), + }; + + let v7_entry = + v7::mcast::MulticastGroupUpdateExternalEntry::from(entry); + mcast::modify_group_external(switch, ip, &tag, v7_entry.into()) + .map(|resp| { + let v7_resp = + v7::mcast::MulticastGroupExternalResponse::from(resp); + HttpResponseCreated(v7_resp.into()) + }) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_groups_list( rqctx: RequestContext>, query_params: Query< @@ -2260,39 +2193,34 @@ impl DpdApi for DpdApiImpl { >, ) -> Result>, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - - let pag_params = query_params.into_inner(); - let Ok(limit) = - usize::try_from(rqctx.page_limit(&pag_params)?.get()) - else { - return Err(DpdError::Invalid( - "Invalid page limit".to_string(), - ) - .into()); - }; + let switch: &Switch = rqctx.context(); - let last_addr = match &pag_params.page { - WhichPage::First(..) => None, - WhichPage::Next(MulticastGroupIpParam { group_ip }) => { - Some(*group_ip) - } - }; + let pag_params = query_params.into_inner(); + let Ok(limit) = usize::try_from(rqctx.page_limit(&pag_params)?.get()) + else { + return Err( + DpdError::Invalid("Invalid page limit".to_string()).into() + ); + }; - let entries = mcast::get_range(switch, last_addr, limit, None); + let last_addr = match &pag_params.page { + WhichPage::First(..) => None, + WhichPage::Next(MulticastGroupIpParam { group_ip }) => { + Some(*group_ip) + } + }; - Ok(HttpResponseOk(ResultsPage::new( - entries, - &EmptyScanParams {}, - |e: &MulticastGroupResponse, _| MulticastGroupIpParam { - group_ip: e.ip(), - }, - )?)) - }) + let entries = mcast::get_range(switch, last_addr, limit, None); + + Ok(HttpResponseOk(ResultsPage::new( + entries, + &EmptyScanParams {}, + |e: &MulticastGroupResponse, _| MulticastGroupIpParam { + group_ip: e.ip(), + }, + )?)) } - #[allow(unused_variables)] async fn multicast_groups_list_by_tag( rqctx: RequestContext>, path: Path, @@ -2301,55 +2229,46 @@ impl DpdApi for DpdApiImpl { >, ) -> Result>, HttpError> { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let tag: String = path.into_inner().tag.into(); - - let pag_params = query_params.into_inner(); - let Ok(limit) = - usize::try_from(rqctx.page_limit(&pag_params)?.get()) - else { - return Err(DpdError::Invalid( - "Invalid page limit".to_string(), - ) - .into()); - }; + let switch: &Switch = rqctx.context(); + let tag: String = path.into_inner().tag.into(); - let last_addr = match &pag_params.page { - WhichPage::First(..) => None, - WhichPage::Next(MulticastGroupIpParam { group_ip }) => { - Some(*group_ip) - } - }; + let pag_params = query_params.into_inner(); + let Ok(limit) = usize::try_from(rqctx.page_limit(&pag_params)?.get()) + else { + return Err( + DpdError::Invalid("Invalid page limit".to_string()).into() + ); + }; - let entries = - mcast::get_range(switch, last_addr, limit, Some(&tag)); - Ok(HttpResponseOk(ResultsPage::new( - entries, - &EmptyScanParams {}, - |e: &MulticastGroupResponse, _| MulticastGroupIpParam { - group_ip: e.ip(), - }, - )?)) - }) + let last_addr = match &pag_params.page { + WhichPage::First(..) => None, + WhichPage::Next(MulticastGroupIpParam { group_ip }) => { + Some(*group_ip) + } + }; + + let entries = mcast::get_range(switch, last_addr, limit, Some(&tag)); + Ok(HttpResponseOk(ResultsPage::new( + entries, + &EmptyScanParams {}, + |e: &MulticastGroupResponse, _| MulticastGroupIpParam { + group_ip: e.ip(), + }, + )?)) } - #[allow(unused_variables)] async fn multicast_reset_by_tag( rqctx: RequestContext>, path: Path, ) -> Result { - require_multicast!({ - let switch: &Switch = rqctx.context(); - let tag: String = path.into_inner().tag.into(); + let switch: &Switch = rqctx.context(); + let tag: String = path.into_inner().tag.into(); - mcast::reset_tag(switch, &tag) - .map(|_| HttpResponseDeleted()) - .map_err(HttpError::from) - }) + mcast::reset_tag(switch, &tag) + .map(|_| HttpResponseDeleted()) + .map_err(HttpError::from) } - #[allow(unused_variables)] async fn multicast_reset_untagged( _rqctx: RequestContext>, ) -> Result { @@ -2366,17 +2285,14 @@ impl DpdApi for DpdApiImpl { )) } - #[allow(unused_variables)] async fn multicast_reset_untagged_v1( rqctx: RequestContext>, ) -> Result { - require_multicast!({ - let switch: &Switch = rqctx.context(); + let switch: &Switch = rqctx.context(); - mcast::reset_untagged(switch) - .map(|_| HttpResponseDeleted()) - .map_err(HttpError::from) - }) + mcast::reset_untagged(switch) + .map(|_| HttpResponseDeleted()) + .map_err(HttpError::from) } #[cfg(feature = "tofino_asic")] diff --git a/dpd/src/counters.rs b/dpd/src/counters.rs index c1c46dfc..9185e3e7 100644 --- a/dpd/src/counters.rs +++ b/dpd/src/counters.rs @@ -23,7 +23,6 @@ use aal::MatchParse; use aal_macros::*; use asic::Handle; use common::counters::CounterId; -#[cfg(feature = "multicast")] use common::counters::MulticastCounterId; use common::table::TableType; @@ -47,34 +46,22 @@ pub struct Counter { } pub fn get_counter_ids() -> Vec { - let mut base = vec![ + vec![ CounterId::Service, CounterId::Ingress, CounterId::Egress, CounterId::Packet, CounterId::DropPort, CounterId::DropReason, - ]; - let mut multicast; - #[cfg(feature = "multicast")] - { - multicast = vec![ - CounterId::Multicast(MulticastCounterId::EgressDropPort), - CounterId::Multicast(MulticastCounterId::EgressDropReason), - CounterId::Multicast(MulticastCounterId::Unicast), - CounterId::Multicast(MulticastCounterId::Multicast), - CounterId::Multicast(MulticastCounterId::MulticastExt), - CounterId::Multicast(MulticastCounterId::MulticastLL), - CounterId::Multicast(MulticastCounterId::MulticastUL), - CounterId::Multicast(MulticastCounterId::MulticastDrop), - ]; - } - #[cfg(not(feature = "multicast"))] - { - multicast = Vec::new(); - } - base.append(&mut multicast); - base + CounterId::Multicast(MulticastCounterId::EgressDropPort), + CounterId::Multicast(MulticastCounterId::EgressDropReason), + CounterId::Multicast(MulticastCounterId::Unicast), + CounterId::Multicast(MulticastCounterId::Multicast), + CounterId::Multicast(MulticastCounterId::MulticastExt), + CounterId::Multicast(MulticastCounterId::MulticastLL), + CounterId::Multicast(MulticastCounterId::MulticastUL), + CounterId::Multicast(MulticastCounterId::MulticastDrop), + ] } /// Fetch a counter by name from the switch's list of counters. This call @@ -329,7 +316,6 @@ pub async fn get_values( port_label(switch, idx.idx).await } CounterId::DropReason => reason_label(idx.idx as u8)?, - #[cfg(feature = "multicast")] CounterId::Multicast(MulticastCounterId::EgressDropPort) | CounterId::Multicast(MulticastCounterId::Unicast) | CounterId::Multicast(MulticastCounterId::Multicast) @@ -339,7 +325,6 @@ pub async fn get_values( | CounterId::Multicast(MulticastCounterId::MulticastDrop) => { port_label(switch, idx.idx).await } - #[cfg(feature = "multicast")] CounterId::Multicast(MulticastCounterId::EgressDropReason) => { reason_label(idx.idx as u8)? } diff --git a/dpd/src/link.rs b/dpd/src/link.rs index ae60baf4..383e18bc 100644 --- a/dpd/src/link.rs +++ b/dpd/src/link.rs @@ -14,7 +14,6 @@ use crate::fault::LinkUpTracker; use crate::ports::AdminEvent; use crate::ports::Event; use crate::table::mac; -#[cfg(feature = "multicast")] use crate::table::mcast; use crate::table::port_ip; use crate::table::uplink; @@ -1629,7 +1628,6 @@ fn set_mac_config( ) -> DpdResult<()> { mac::mac_set(switch, asic_id, mac)?; - #[cfg(feature = "multicast")] { mac::mcast_mac_set(switch, asic_id, mac)?; mcast::mcast_egress::add_port_mapping_entry(switch, asic_id)?; @@ -1640,7 +1638,6 @@ fn set_mac_config( fn clear_mac_config(switch: &Switch, asic_id: AsicId) -> DpdResult<()> { mac::mac_clear(switch, asic_id)?; - #[cfg(feature = "multicast")] { mac::mcast_mac_clear(switch, asic_id)?; mcast::mcast_egress::del_port_mapping_entry(switch, asic_id)?; diff --git a/dpd/src/macaddrs.rs b/dpd/src/macaddrs.rs index 52f850d4..10efb51f 100644 --- a/dpd/src/macaddrs.rs +++ b/dpd/src/macaddrs.rs @@ -24,7 +24,6 @@ use common::ports::PortId; cfg_if::cfg_if! { if #[cfg(feature = "tofino_asic")] { use std::convert::TryFrom; - #[cfg(feature = "multicast")] use crate::table::mcast; use common::ports::PortFec; use common::ports::PortSpeed; @@ -434,7 +433,6 @@ impl Switch { // Reset ingress and egress MAC tables and Port ID table(s). mac::reset(self)?; - #[cfg(feature = "multicast")] { mac::mcast_reset(self)?; mcast::mcast_egress::reset_bitmap_table(self)?; diff --git a/dpd/src/main.rs b/dpd/src/main.rs index 5fc08ec5..720b32dd 100644 --- a/dpd/src/main.rs +++ b/dpd/src/main.rs @@ -65,7 +65,6 @@ mod freemap; mod link; mod loopback; mod macaddrs; -#[cfg(feature = "multicast")] mod mcast; mod nat; mod oxstats; @@ -200,7 +199,6 @@ pub struct Switch { pub oximeter_producer: Mutex>, pub oximeter_meta: Mutex>, pub reconciler: link::LinkReconciler, - #[cfg(feature = "multicast")] pub mcast: Mutex, mac_mgmt: Mutex, @@ -316,7 +314,6 @@ impl Switch { oximeter_producer: Mutex::new(None), oximeter_meta: Mutex::new(None), reconciler: link::LinkReconciler::default(), - #[cfg(feature = "multicast")] mcast: Mutex::new(mcast::MulticastGroupData::new()), mac_mgmt, port_history: Mutex::new(BTreeMap::new()), diff --git a/dpd/src/mcast/mod.rs b/dpd/src/mcast/mod.rs index 5a685415..3ac32330 100644 --- a/dpd/src/mcast/mod.rs +++ b/dpd/src/mcast/mod.rs @@ -2393,15 +2393,16 @@ mod tests { None ); - // Vec with only Exact sources stays as-is + // Vec with only Exact sources is deduplicated and sorted let exact_sources = vec![ IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1))), IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))), ]; - assert_eq!( - canonicalize_sources(Some(exact_sources.clone())), - Some(exact_sources) - ); + let expected = vec![ + IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(10, 0, 0, 1))), + IpSrc::Exact(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 1))), + ]; + assert_eq!(canonicalize_sources(Some(exact_sources)), Some(expected)); // Single Exact source stays as-is let single_exact = vec![IpSrc::Exact(IpAddr::V6(Ipv6Addr::new( diff --git a/dpd/src/table/mac.rs b/dpd/src/table/mac.rs index 533577f5..652b94f4 100644 --- a/dpd/src/table/mac.rs +++ b/dpd/src/table/mac.rs @@ -102,18 +102,15 @@ pub fn reset(s: &Switch) -> DpdResult<()> { /// Add a new entry to the MAC table. /// /// An error is returned if the entry already exists. Use `mac_update` instead. -#[cfg(feature = "multicast")] pub fn mcast_mac_set(s: &Switch, port: u16, mac: MacAddr) -> DpdResult<()> { mac_set_common(s, TableType::PortMacAddressMcast, port, mac) } /// Remove an entry from the MAC table. -#[cfg(feature = "multicast")] pub fn mcast_mac_clear(s: &Switch, port: u16) -> DpdResult<()> { mac_clear_common(s, TableType::PortMacAddressMcast, port) } -#[cfg(feature = "multicast")] pub fn mcast_table_dump( s: &Switch, from_hardware: bool, @@ -124,7 +121,6 @@ pub fn mcast_table_dump( ) } -#[cfg(feature = "multicast")] pub fn mcast_counter_fetch( s: &Switch, force_sync: bool, @@ -133,7 +129,6 @@ pub fn mcast_counter_fetch( } /// Remove all entries from the MAC table. -#[cfg(feature = "multicast")] #[cfg(feature = "tofino_asic")] pub fn mcast_reset(s: &Switch) -> DpdResult<()> { s.table_clear(TableType::PortMacAddressMcast) diff --git a/dpd/src/table/mod.rs b/dpd/src/table/mod.rs index 5a98e7fc..a499c0ad 100644 --- a/dpd/src/table/mod.rs +++ b/dpd/src/table/mod.rs @@ -23,7 +23,6 @@ pub mod arp_ipv4; pub mod attached_subnet_v4; pub mod attached_subnet_v6; pub mod mac; -#[cfg(feature = "multicast")] pub mod mcast; pub mod nat; pub mod neighbor_ipv6; @@ -258,43 +257,33 @@ pub fn get_entries( TableType::UplinkIngress => { uplink::ingress_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::McastIpv6 => { mcast::mcast_replication::ipv6_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::McastIpv4SrcFilter => { mcast::mcast_src_filter::ipv4_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::McastIpv6SrcFilter => { mcast::mcast_src_filter::ipv6_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::NatIngressIpv4Mcast => { mcast::mcast_nat::ipv4_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::NatIngressIpv6Mcast => { mcast::mcast_nat::ipv6_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::RouteIpv4Mcast => { mcast::mcast_route::ipv4_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::RouteIpv6Mcast => { mcast::mcast_route::ipv6_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::PortMacAddressMcast => { mac::mcast_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::McastEgressDecapPorts => { mcast::mcast_egress::bitmap_table_dump(switch, from_hardware) } - #[cfg(feature = "multicast")] TableType::McastEgressPortMapping => { mcast::mcast_egress::port_mapping_table_dump(switch, from_hardware) } @@ -353,43 +342,33 @@ pub fn get_counters( TableType::UplinkIngress => { uplink::ingress_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::McastIpv6 => { mcast::mcast_replication::ipv6_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::McastIpv4SrcFilter => { mcast::mcast_src_filter::ipv4_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::McastIpv6SrcFilter => { mcast::mcast_src_filter::ipv6_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::NatIngressIpv4Mcast => { mcast::mcast_nat::ipv4_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::NatIngressIpv6Mcast => { mcast::mcast_nat::ipv6_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::RouteIpv4Mcast => { mcast::mcast_route::ipv4_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::RouteIpv6Mcast => { mcast::mcast_route::ipv6_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::McastEgressDecapPorts => { mcast::mcast_egress::bitmap_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::McastEgressPortMapping => { mcast::mcast_egress::port_mapping_counter_fetch(switch, force_sync) } - #[cfg(feature = "multicast")] TableType::PortMacAddressMcast => { mac::mcast_counter_fetch(switch, force_sync) } diff --git a/dpd/src/types.rs b/dpd/src/types.rs index 2626b8d7..c626f8a0 100644 --- a/dpd/src/types.rs +++ b/dpd/src/types.rs @@ -12,7 +12,6 @@ use common::SmfError; use common::ports::PortId; use common::ports::QsfpPort; use dpd_types::link::LinkId; -use slog::error; use std::{convert, net::IpAddr}; use transceiver_controller::Error as TransceiverError; @@ -315,7 +314,6 @@ impl convert::From for DpdError { } } -#[cfg(feature = "multicast")] impl convert::From for DpdError { fn from(err: dpd_types::mcast::Error) -> Self { DpdError::Invalid(err.to_string()) diff --git a/swadm/Cargo.toml b/swadm/Cargo.toml index 57ceccdf..0565ced7 100644 --- a/swadm/Cargo.toml +++ b/swadm/Cargo.toml @@ -5,9 +5,6 @@ version = "0.1.0" authors = ["nils "] edition = "2024" -[features] -multicast = [] - [dependencies] anyhow.workspace = true chrono.workspace = true diff --git a/swadm/tests/cli/counters.rs b/swadm/tests/cli/counters.rs index 5eaf9078..bd75ef16 100644 --- a/swadm/tests/cli/counters.rs +++ b/swadm/tests/cli/counters.rs @@ -12,11 +12,7 @@ use crate::cmd; #[serial] #[ignore] fn counters_list() -> anyhow::Result<()> { - #[cfg(not(feature = "multicast"))] const DIFF_FILE: &str = "counters.txt"; - #[cfg(feature = "multicast")] - const DIFF_FILE: &str = "counters_multicast.txt"; - cmd::swadm("counters list")?.try_expectorate(DIFF_FILE) } diff --git a/swadm/tests/cli/expect/counters.txt b/swadm/tests/cli/expect/counters.txt index 138d1f14..5cf15de9 100644 --- a/swadm/tests/cli/expect/counters.txt +++ b/swadm/tests/cli/expect/counters.txt @@ -1 +1 @@ -["Service", "Ingress", "Egress", "Packet", "Ingress_Drop_Port", "Ingress_Drop_Reason"] +["Service", "Ingress", "Egress", "Packet", "Ingress_Drop_Port", "Ingress_Drop_Reason", "Egress_Drop_Port", "Egress_Drop_Reason", "Unicast", "Multicast", "Multicast_External", "Multicast_Link_Local", "Multicast_Underlay", "Multicast_Drop"] diff --git a/swadm/tests/cli/expect/counters_multicast.txt b/swadm/tests/cli/expect/counters_multicast.txt deleted file mode 100644 index 5cf15de9..00000000 --- a/swadm/tests/cli/expect/counters_multicast.txt +++ /dev/null @@ -1 +0,0 @@ -["Service", "Ingress", "Egress", "Packet", "Ingress_Drop_Port", "Ingress_Drop_Reason", "Egress_Drop_Port", "Egress_Drop_Reason", "Unicast", "Multicast", "Multicast_External", "Multicast_Link_Local", "Multicast_Underlay", "Multicast_Drop"] diff --git a/xtask/src/codegen.rs b/xtask/src/codegen.rs index 87fedc8a..7a15bfdf 100644 --- a/xtask/src/codegen.rs +++ b/xtask/src/codegen.rs @@ -126,7 +126,6 @@ pub fn build( app_name: String, sde_location: String, stages: Option, - multicast: bool, ) -> Result<()> { let root = super::project_root()?; let src_dir = match app_name.as_str() { @@ -168,10 +167,8 @@ pub fn build( if let Some(s) = stages { args.push(format!("--num-stages-override={s}")); } - if multicast { - args.push("-D".to_string()); - args.push("MULTICAST".to_string()); - } + args.push("-D".to_string()); + args.push("MULTICAST".to_string()); args.push(app_path); println!("op: {args:?}"); diff --git a/xtask/src/main.rs b/xtask/src/main.rs index f7cdda5c..ed36dec4 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -79,10 +79,6 @@ enum XtaskCommands { /// pipeline stages to build for #[clap(long)] stages: Option, - - /// Include support for multicast - #[clap(long)] - multicast: bool, }, /// build an installable dataplane controller package Dist { @@ -251,8 +247,8 @@ async fn main() { if let Err(e) = match task.subcommand { XtaskCommands::Openapi(external) => external .exec_bin("dendrite-dropshot-apis", "dendrite-dropshot-apis"), - XtaskCommands::Codegen { name, sde, stages, multicast } => { - codegen::build(name, sde, stages, multicast) + XtaskCommands::Codegen { name, sde, stages } => { + codegen::build(name, sde, stages) } XtaskCommands::Dist { features, names, release, format } => { plat::dist(features, names, release, format).await From 7790240768b10b9926bd06f22f64e44b0a4dfcef Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Wed, 9 Sep 2026 04:03:10 +0000 Subject: [PATCH 2/6] [p4, sidecar] keep (IPV*_LPM_SIZE - 1) entries for routing tables w/ multicast Removing the multicast feature gate got us over Tofino's 20-stage pipeline limit. The table resizing work in #268 matched the v6 LPM table size to what we had in v4, putting us at 20 stages without multicast. In #208, I explored a few different approaches to refactor our pipeline, across ingress and egress, to give us more breathing room (stage-wise). However, we want to be efficient and targeted when making changes to the sidecar. Reducing mcast table sizes was my first attempt. Route and replication tables fit in SRAM; SSM is TCAM. The result, sadly, was fewer TCAM blocks, but the same stage count. Instead, I went the direction of fulfilling a Nils'ism: > XXX: this control could be moved to the Egress pipeline if we need more space > in the Ingress pipeline. Currently unicast packets are able to bypass that > pipeline, which is why we've tacked it on here. We could probably also merge > it with the MacRewrite control, as they are both per-port settings, but that > would present some weird semantics to the control plane daemon. I moved the `EgressFilter` control to the egress pipeline to save a stage, carrying (or bridging) the NAT egress bit in the bridge header. I *did not* move the `MacRewrite` control over to handle both uni+multi(cast) in egress, but that was demonstrated in #208 for a follow-up (post-testing). With the move, 20 stages is achieved! However, the v4/v6 route tests still stopped below 8191 entries with a deficit. The exact-match route-target tables were losing capacity under four-way cuckoo placement (default). [`@ways(8)`][ways] was the trick I found that gives each key in the table eight placement choices, which now makes table tests fill all 8191 entries. Includes: - Removal of the `#ifdef MULTICAST` codegen guards. - Counter updates to match our move of the NAT egress filter - Ordering of the NAT egress filter before the CPU-copy (`egress_rid`) check, making sure NAT unicast is not treated as a multicast CPU copy. [ways]: https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183 --- asic/src/tofino_common/mod.rs | 2 +- common/src/table.rs | 6 +- .../tests/integration_tests/table_tests.rs | 20 +-- dpd/p4/constants.p4 | 34 ++-- dpd/p4/metadata.p4 | 2 + dpd/p4/parser.p4 | 28 +--- dpd/p4/sidecar.p4 | 150 +++++++----------- dpd/src/table/uplink.rs | 2 +- xtask/src/codegen.rs | 2 - 9 files changed, 98 insertions(+), 148 deletions(-) diff --git a/asic/src/tofino_common/mod.rs b/asic/src/tofino_common/mod.rs index 0002c233..b6f56fc6 100644 --- a/asic/src/tofino_common/mod.rs +++ b/asic/src/tofino_common/mod.rs @@ -48,7 +48,7 @@ fn table_name(type_: TableType) -> &'static str { TableType::NatIngressIpv4 => "pipe.Ingress.nat_ingress.ingress_ipv4", TableType::NatIngressIpv6 => "pipe.Ingress.nat_ingress.ingress_ipv6", TableType::UplinkIngress => "pipe.Ingress.filter.uplink_ports", - TableType::UplinkEgress => "pipe.Ingress.egress_filter.egress_filter", + TableType::UplinkEgress => "pipe.Egress.egress_filter.egress_filter", TableType::AttachedSubnetIpv4 => { "pipe.Ingress.attached_subnet_ingress.attached_subnets_v4" } diff --git a/common/src/table.rs b/common/src/table.rs index a7ee364f..0fdb9aee 100644 --- a/common/src/table.rs +++ b/common/src/table.rs @@ -131,7 +131,7 @@ impl fmt::Display for TableType { TableType::UplinkIngress => "Ingress.filter.uplink_ports".to_string(), TableType::UplinkEgress => - "Ingress.egress_filter.egress_filter".to_string(), + "Egress.egress_filter.egress_filter".to_string(), TableType::AttachedSubnetIpv4 => { "Ingress.attached_subnet_ingress.attached_subnets_v4" .to_string() @@ -205,9 +205,7 @@ impl TryFrom<&str> for TableType { "ingress.nat_ingress.ingress_ipv4" => Ok(TableType::NatIngressIpv4), "ingress.nat_ingress.ingress_ipv6" => Ok(TableType::NatIngressIpv6), "ingress.filter.uplink_ports" => Ok(TableType::UplinkIngress), - "ingress.egress_filter.egress_filter" => { - Ok(TableType::UplinkEgress) - } + "egress.egress_filter.egress_filter" => Ok(TableType::UplinkEgress), "ingress.attached_subnet_ingress.attached_subnets_v4" => { Ok(TableType::AttachedSubnetIpv4) } diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index 5b31ba45..1407db41 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -32,17 +32,17 @@ use crate::integration_tests::common::prelude::*; // oximeter, which will let this test query dpd for the table size rather than // hardcoding it below. // -// The LPM tables are packed using a cuckoo hash algorithm. The conflict -// resolution approach can result in some slots not being available for some -// patterns of entries. Exactly which tables and which entries are unavailable -// can change depending on table layout on the ASIC. All of this means that it -// is normal for these table sizes to change slightly after updating the P4 -// code. If the table size appears to change dramatically, that's worth -// investigating. If it only changes by an entry or two, it's fine to just -// adjust the constant below to match the observed result. -// +// LPM lookups live in TCAM, placing entries without hash conflicts. +// The exact-match index->target tables behind them are cuckoo-managed SRAM +// (see pipe_mgr/cuckoo_move.h in p4lang/open-p4studio). Without enough +// hash ways, cuckoo conflicts can cap capacity a few % below what's +// declared. The @ways(8) annotation on those route tables helps us reach full +// occupancy on this build. It is normal for these sizes to change slightly +// after updating the P4 code. If the table size appears to change +// dramatically, that's worth investigating. If it only changes by an +// entry or two, it's fine to just adjust the constant below to match the +// observed result. const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table - const IPV6_LPM_SIZE: usize = 8191; // ipv6 forwarding table const SWITCH_IPV4_ADDRS_SIZE: usize = 511; // ipv4 addrs assigned to our ports diff --git a/dpd/p4/constants.p4 b/dpd/p4/constants.p4 index 7f6584ff..225301b6 100644 --- a/dpd/p4/constants.p4 +++ b/dpd/p4/constants.p4 @@ -8,22 +8,20 @@ const bit<16> L2_ISOLATED_FLAG = 0x8000; // TODO: these all need to be bigger. Early experimentation is showing that this // is going to need to come either through ATCAM/ALPM or code restructuring. -const int IPV4_NAT_TABLE_SIZE = 1024; // nat routing table -const int IPV6_NAT_TABLE_SIZE = 1024; // nat routing table -const int IPV4_LPM_SIZE = 8192; // ipv4 forwarding table -#ifdef MULTICAST -const int IPV6_LPM_SIZE = 1024; // ipv6 forwarding table -#else -const int IPV6_LPM_SIZE = 8192; // ipv6 forwarding table -#endif -const int IPV4_ARP_SIZE = 512; // arp cache -const int IPV6_NEIGHBOR_SIZE = 512; // ipv6 neighbor cache -const int SWITCH_IPV4_ADDRS_SIZE = 512; // ipv4 addrs assigned to our ports -const int SWITCH_IPV6_ADDRS_SIZE = 512; // ipv6 addrs assigned to our ports -const int IPV4_MULTICAST_TABLE_SIZE = 1024; // multicast routing table(s) for IPv4 -const int IPV6_MULTICAST_TABLE_SIZE = 1024; // multicast routing table(s) for IPv6 -const int ATTACHED_SUBNETS_V4_SIZE = 512; // external subnets mapped to instances -const int ATTACHED_SUBNETS_V6_SIZE = 512; // external subnets mapped to instances +const int IPV4_NAT_TABLE_SIZE = 1024; // nat routing table +const int IPV6_NAT_TABLE_SIZE = 1024; // nat routing table +const int IPV4_LPM_SIZE = 8192; // ipv4 forwarding table +const int IPV6_LPM_SIZE = 8192; // ipv6 forwarding table +const int IPV4_ARP_SIZE = 512; // arp cache +const int IPV6_NEIGHBOR_SIZE = 512; // ipv6 neighbor cache +const int SWITCH_IPV4_ADDRS_SIZE = 512; // ipv4 addrs assigned to our ports +const int SWITCH_IPV6_ADDRS_SIZE = 512; // ipv6 addrs assigned to our ports +const int IPV4_MULTICAST_TABLE_SIZE = 1024; // multicast routing table(s) for IPv4 +const int IPV6_MULTICAST_TABLE_SIZE = 1024; // multicast routing table(s) for IPv6 +const int MCAST_SOURCE_FILTER_IPV4_SIZE = 1024; // source filter size for IPv4 +const int MCAST_SOURCE_FILTER_IPV6_SIZE = 1024; // source filter size for IPv6 +const int ATTACHED_SUBNETS_V4_SIZE = 512; // external subnets mapped to instances +const int ATTACHED_SUBNETS_V6_SIZE = 512; // external subnets mapped to instances const bit<8> SC_FWD_FROM_USERSPACE = 0x00; const bit<8> SC_FWD_TO_USERSPACE = 0x01; @@ -60,6 +58,9 @@ const bit<2> MULTICAST_TAG_EXTERNAL = 0; const bit<2> MULTICAST_TAG_UNDERLAY = 1; const bit<2> MULTICAST_TAG_UNDERLAY_EXTERNAL = 2; +const bit<16> IPV6_INTERFACE_LOCAL_16 = 0xff01; // ff01::/16 +const bit<16> IPV6_LINK_LOCAL_16 = 0xff02; // ff02::/16 + /* IPv6 Address Mask and Pattern Constants */ // Reserved underlay multicast subnet (ff04::/64). This /64 within admin-local // scope is reserved for internal underlay multicast allocation. Customer @@ -97,4 +98,3 @@ const bit<8> DROP_GENEVE_OPTION_UNKNOWN = 0x1A; const bit<8> DROP_SCTP = 0x1B; // MAX(DROP_xxx) + 1 const bit<32> DROP_REASON_MAX = 0x1C; - diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index c609a269..0521a93e 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -10,6 +10,8 @@ @flexible header bridge_h { PortId_t ingress_port; + bool nat_egress_hit; + bit<6> reserved; } struct sidecar_ingress_meta_t { diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index 3a2618d0..f90668b6 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -55,6 +55,8 @@ parser IngressParser( meta.bridge_hdr.setValid(); meta.bridge_hdr.ingress_port = ig_intr_md.ingress_port; + meta.bridge_hdr.nat_egress_hit = false; + meta.bridge_hdr.reserved = 0; transition port_metadata; } @@ -246,8 +248,8 @@ parser IngressParser( }); transition select(hdr.ipv6.dst_addr[127:112]) { - 16w0xff01: drop_interface_local_mcast; - 16w0xff02: set_link_local_mcast; + IPV6_INTERFACE_LOCAL_16: drop_interface_local_mcast; + IPV6_LINK_LOCAL_16: set_link_local_mcast; default: check_ipv6_mcast; } } @@ -538,7 +540,6 @@ parser EgressParser( state meta_init { meta.drop_reason = 0; - meta.bridge_hdr.setInvalid(); meta.decap_ports_0 = 0; meta.decap_ports_1 = 0; @@ -561,9 +562,11 @@ parser EgressParser( state parse_bridge_hdr { pkt.extract(bridge_hdr); meta.bridge_hdr = bridge_hdr; - meta.bridge_hdr.setValid(); - transition parse_ethernet; + transition select(bridge_hdr.nat_egress_hit) { + true: accept; + default: parse_ethernet; + } } state parse_ethernet { @@ -591,8 +594,6 @@ parser EgressParser( pkt.extract(hdr.ipv4); transition select(hdr.ipv4.protocol) { - IPPROTO_ICMP: parse_icmp; - IPPROTO_TCP: parse_tcp; IPPROTO_UDP: parse_udp; default: accept; } @@ -602,24 +603,11 @@ parser EgressParser( pkt.extract(hdr.ipv6); transition select(hdr.ipv6.next_hdr) { - IPPROTO_TCP: parse_tcp; IPPROTO_UDP: parse_udp; default: accept; } } - state parse_icmp { - pkt.extract(hdr.icmp); - - transition accept; - } - - state parse_tcp { - pkt.extract(_); - - transition accept; - } - state parse_udp { pkt.extract(hdr.udp); diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 07a694ae..37a394cd 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -105,10 +105,7 @@ control Filter( ) { DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv4_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv6_ctr; -#ifdef MULTICAST Counter, PortId_t>(512, CounterType_t.PACKETS) drop_mcast_ctr; - bit<16> mcast_scope; -#endif /* MULTICAST */ action dropv4() { meta.drop_reason = DROP_IPV4_SWITCH_ADDR_MISS; @@ -179,7 +176,6 @@ control Filter( if (hdr.arp.isValid()) { switch_ipv4_addr.apply(); } else if (hdr.ipv4.isValid()) { -#ifdef MULTICAST if (meta.is_mcast) { // IPv4 Multicast Address Validation (RFC 1112, RFC 7042) // @@ -210,11 +206,7 @@ control Filter( } else { switch_ipv4_addr.apply(); } -#else /* MULTICAST */ - switch_ipv4_addr.apply(); -#endif /* MULTICAST */ } else if (hdr.ipv6.isValid()) { -#ifdef MULTICAST if (meta.is_mcast) { // Validate the IPv6 multicast MAC address format (RFC 2464, // RFC 7042). @@ -247,7 +239,6 @@ control Filter( return; } } -#endif /* MULTICAST */ if (!meta.is_mcast || meta.is_link_local_mcastv6 && !meta.encap_needed) { switch_ipv6_addr.apply(); @@ -479,10 +470,8 @@ control NatIngress ( ) { DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv4_ingress_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) ipv6_ingress_ctr; -#ifdef MULTICAST DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv4_ingress_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv6_ingress_ctr; -#endif /* MULTICAST */ action add_encap_headers(bit<16> udp_len) { // 8 bytes with a 4 byte option @@ -597,7 +586,6 @@ control NatIngress ( } -#ifdef MULTICAST action mcast_forward_ipv4_to(ipv6_addr_t target, mac_addr_t inner_mac, geneve_vni_t vni) { meta.nat_ingress_hit = true; meta.nat_ingress_tgt = target; @@ -646,7 +634,6 @@ control NatIngress ( const size = IPV6_MULTICAST_TABLE_SIZE; counters = mcast_ipv6_ingress_ctr; } -#endif /* MULTICAST */ action set_icmp_dst_port() { meta.l4_dst_port = hdr.icmp.data[31:16]; @@ -718,7 +705,6 @@ control NatIngress ( // Note: This whole conditional could be simpler as a set of */ // `const entries`, but apply (on tables) cannot be called from actions -#ifdef MULTICAST if (hdr.ipv4.isValid()) { if (meta.is_mcast) { ingress_ipv4_mcast.apply(); @@ -734,15 +720,6 @@ control NatIngress ( ingress_ipv6.apply(); } } -#else /* MULTICAST */ - if (hdr.ipv4.isValid()) { - if (!meta.encap_needed) { - ingress_ipv4.apply(); - } - } else if (hdr.ipv6.isValid()) { - ingress_ipv6.apply(); - } -#endif /* MULTICAST */ if (ingress_hit.apply().hit) { if (hdr.ipv4.isValid()) { @@ -945,7 +922,14 @@ control RouterLookupIndex6( * test to pass. We want the lookup and forward tables to have the same * capacity from dpd's perspective, and the "default" entry consumes a * slot in the lookup table. + * + * Note: we annotate @ways here, increasing cuckoo placement choices to 8 + * candidate buckets per key. Full occupancy of the route-target table + * is a measured result for this build, not a compiler guarantee. + * + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. */ + @ways(8) table route { key = { res.idx: exact; } actions = { forward; forward_vlan; } @@ -1066,7 +1050,14 @@ control RouterLookupIndex4( * test to pass. We want the lookup and forward tables to have the same * capacity from dpd's perspective, and the "default" entry consumes a * slot in the lookup table. + * + * Note: we annotate @ways here, increasing cuckoo placement choices to 8 + * candidate buckets per key. Full occupancy of the route-target table + * is a measured result for this build, not a compiler guarantee. + * + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. */ + @ways(8) table route { key = { res.idx: exact; } actions = { forward; forward_v6; forward_vlan; forward_vlan_v6; } @@ -1291,7 +1282,6 @@ control Router4 ( } } -#ifdef MULTICAST control MulticastRouter4( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, @@ -1371,7 +1361,6 @@ control MulticastRouter4( } } } -#endif /* MULTICAST */ control Router6 ( inout sidecar_headers_t hdr, @@ -1434,7 +1423,6 @@ control Router6 ( } } -#ifdef MULTICAST control MulticastRouter6 ( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, @@ -1511,7 +1499,6 @@ control MulticastRouter6 ( } } } -#endif /* MULTICAST */ control L3Router( inout sidecar_headers_t hdr, @@ -1520,7 +1507,6 @@ control L3Router( inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { apply { -#ifdef MULTICAST if (hdr.ipv4.isValid()) { if (meta.is_mcast && !meta.is_link_local_mcastv6) { MulticastRouter4.apply(hdr, meta, ig_intr_md, ig_tm_md); @@ -1534,13 +1520,6 @@ control L3Router( Router6.apply(hdr, meta, ig_intr_md, ig_tm_md); } } -#else /* MULTICAST */ - if (hdr.ipv4.isValid()) { - Router4.apply(hdr, meta, ig_intr_md, ig_tm_md); - } else if (hdr.ipv6.isValid()) { - Router6.apply(hdr, meta, ig_intr_md, ig_tm_md); - } -#endif /* MULTICAST */ if (meta.resolve_nexthop) { if (meta.nexthop_ipv4 != 0) { Arp.apply(hdr, meta, ig_intr_md, ig_tm_md); @@ -1551,38 +1530,6 @@ control L3Router( } } -/* - * XXX: this control could be moved to the Egress pipeline if we need more space - * in the Ingress pipeline. Currently unicast packets are able to bypass that - * pipeline, which is why we've tacked it on here. We could probably also merge - * it with the MacRewrite control, as they are both per-port settings, but that - * would present some weird semantics to the control plane daemon. - */ -control EgressFilter( - inout sidecar_ingress_meta_t meta, - in ingress_intrinsic_metadata_for_tm_t ig_tm_md -) { - action guest_traffic_not_allowed() { - meta.drop_reason = DROP_NAT_EGRESS_BLOCKED; - meta.dropped = true; - } - - action guest_traffic_allowed() { - } - - table egress_filter { - key = { ig_tm_md.ucast_egress_port : exact; } - actions = { guest_traffic_allowed; guest_traffic_not_allowed; } - - const size = 256; - default_action = guest_traffic_not_allowed; - } - - apply { - egress_filter.apply(); - } -} - control MacRewrite( inout sidecar_headers_t hdr, in PortId_t port @@ -1607,7 +1554,6 @@ control MacRewrite( } } -#ifdef MULTICAST /* This control is used to rewrite the source and destination MAC addresses * for multicast packets. The destination MAC address is derived from the * destination IP address, and the source MAC address is set based on the @@ -1788,7 +1734,7 @@ control MulticastIngress ( drop_mcastv4_filtered_source; } default_action = drop_mcastv4_filtered_source; - const size = IPV4_MULTICAST_TABLE_SIZE; + const size = MCAST_SOURCE_FILTER_IPV4_SIZE; counters = mcast_ipv4_ssm_ctr; } @@ -1813,7 +1759,7 @@ control MulticastIngress ( drop_mcastv6_filtered_source; } default_action = drop_mcastv6_filtered_source; - const size = IPV6_MULTICAST_TABLE_SIZE; + const size = MCAST_SOURCE_FILTER_IPV6_SIZE; counters = mcast_ipv6_ssm_ctr; } @@ -2110,7 +2056,6 @@ control MulticastEgress ( } } } -#endif /* MULTICAST */ control Ingress( inout sidecar_headers_t hdr, @@ -2126,10 +2071,7 @@ control Ingress( NatIngress() nat_ingress; NatEgress() nat_egress; L3Router() l3_router; - EgressFilter() egress_filter; -#ifdef MULTICAST MulticastIngress() mcast_ingress; -#endif /* MULTICAST */ MacRewrite() mac_rewrite; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) ingress_ctr; @@ -2166,10 +2108,8 @@ control Ingress( if (!meta.dropped) { if (!meta.is_mcast || meta.is_link_local_mcastv6) { services.apply(hdr, meta, ig_intr_md, ig_tm_md); -#ifdef MULTICAST } else if (meta.is_mcast && !meta.is_link_local_mcastv6) { mcast_ingress.apply(hdr, meta, ig_intr_md, ig_tm_md); -#endif /* MULTICAST */ } } @@ -2180,10 +2120,6 @@ control Ingress( if (!meta.dropped) { l3_router.apply(hdr, meta, ig_intr_md, ig_tm_md); } - if (!meta.dropped && meta.nat_egress_hit && - !meta.is_mcast && !meta.service_routed) { - egress_filter.apply(meta, ig_tm_md); - } } if (meta.dropped) { @@ -2196,8 +2132,12 @@ control Ingress( if (ig_tm_md.ucast_egress_port != USER_SPACE_SERVICE_PORT) { mac_rewrite.apply(hdr, ig_tm_md.ucast_egress_port); } - meta.bridge_hdr.setInvalid(); - ig_tm_md.bypass_egress = 1w1; + if (meta.nat_egress_hit && !meta.service_routed) { + meta.bridge_hdr.nat_egress_hit = true; + } else { + meta.bridge_hdr.setInvalid(); + ig_tm_md.bypass_egress = 1w1; + } } if (meta.encap_needed) { @@ -2319,6 +2259,37 @@ control IngressDeparser(packet_out pkt, } } +// Filter NAT egress traffic by port. +// +// Ports not explicitly marked as uplinks drop guest traffic to prevent +// NAT'd packets from egressing on non-uplink ports. +// +// This control is placed in the egress pipeline to avoid adding a stage to +// ingress. +control NatEgressFilter( + inout sidecar_egress_meta_t meta, + in egress_intrinsic_metadata_t eg_intr_md +) { + action guest_traffic_not_allowed() { + meta.drop_reason = DROP_NAT_EGRESS_BLOCKED; + } + + action guest_traffic_allowed() { + } + + table egress_filter { + key = { eg_intr_md.egress_port : exact; } + actions = { guest_traffic_allowed; guest_traffic_not_allowed; } + + const size = 256; + default_action = guest_traffic_not_allowed; + } + + apply { + egress_filter.apply(); + } +} + control Egress( inout sidecar_headers_t hdr, inout sidecar_egress_meta_t meta, @@ -2327,7 +2298,7 @@ control Egress( inout egress_intrinsic_metadata_for_deparser_t eg_dprsr_md, inout egress_intrinsic_metadata_for_output_port_t eg_oport_md ) { -#ifdef MULTICAST + NatEgressFilter() egress_filter; MulticastMacRewrite() mac_rewrite; MulticastEgress() mcast_egress; @@ -2346,7 +2317,7 @@ control Egress( bool is_link_local_ipv6_mcast = false; if (hdr.ipv6.isValid()) { bit<16> ipv6_prefix = (bit<16>)hdr.ipv6.dst_addr[127:112]; - is_link_local_ipv6_mcast = (ipv6_prefix == 16w0xff02); + is_link_local_ipv6_mcast = (ipv6_prefix == IPV6_LINK_LOCAL_16); } bool is_mcast = is_egress_rid_mcast || is_link_local_ipv6_mcast; @@ -2359,6 +2330,8 @@ control Egress( mcast_egress.apply(hdr, meta, eg_intr_md, eg_dprsr_md); mac_rewrite.apply(hdr, eg_intr_md.egress_port); } + } else if (meta.bridge_hdr.nat_egress_hit) { + egress_filter.apply(meta, eg_intr_md); } else if (eg_intr_md.egress_rid == 0 && eg_intr_md.egress_rid_first == 1) { // Drop CPU copies (RID=0) to prevent unwanted packets on port 0 @@ -2383,14 +2356,9 @@ control Egress( underlay_mcast_ctr.count(eg_intr_md.egress_port); } } else { - // non-multicast packets should bypass the egress - // pipeline, so we would expect this to be 0. unicast_ctr.count(eg_intr_md.egress_port); } } -#else /* MULTICAST */ - apply { } -#endif /* MULTICAST */ } control EgressDeparser( @@ -2399,7 +2367,6 @@ control EgressDeparser( in sidecar_egress_meta_t meta, in egress_intrinsic_metadata_for_deparser_t eg_dprsr_md ) { -#ifdef MULTICAST Checksum() ipv4_checksum; apply { @@ -2421,9 +2388,6 @@ control EgressDeparser( } pkt.emit(hdr); } -#else - apply { pkt.emit(hdr); } -#endif } Pipeline( diff --git a/dpd/src/table/uplink.rs b/dpd/src/table/uplink.rs index d8f04abd..9b47a557 100644 --- a/dpd/src/table/uplink.rs +++ b/dpd/src/table/uplink.rs @@ -28,7 +28,7 @@ enum IngressAction { #[derive(MatchParse, Debug, Hash)] struct EgressMatchKey { - #[match_xlate(name = "ucast_egress_port")] + #[match_xlate(name = "egress_port")] out_port: u16, } diff --git a/xtask/src/codegen.rs b/xtask/src/codegen.rs index 7a15bfdf..53279f4b 100644 --- a/xtask/src/codegen.rs +++ b/xtask/src/codegen.rs @@ -167,8 +167,6 @@ pub fn build( if let Some(s) = stages { args.push(format!("--num-stages-override={s}")); } - args.push("-D".to_string()); - args.push("MULTICAST".to_string()); args.push(app_path); println!("op: {args:?}"); From 4d4fc506e136f387ac647b1e866c15abf3c26b03 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 11 Sep 2026 02:57:07 +0000 Subject: [PATCH 3/6] [review] counter renaming, formatting, unskip fragmented tests --- .github/buildomat/packet-test-common.sh | 3 +- asic/src/tofino_common/mod.rs | 24 +++++------ common/src/counters.rs | 46 +++++++++++---------- dpd/p4/constants.p4 | 2 - dpd/p4/sidecar.p4 | 24 +++++------ dpd/src/api_server.rs | 5 +-- dpd/src/counters.rs | 54 ++++++++++++------------- 7 files changed, 75 insertions(+), 83 deletions(-) diff --git a/.github/buildomat/packet-test-common.sh b/.github/buildomat/packet-test-common.sh index e72d1fb0..660e03cd 100755 --- a/.github/buildomat/packet-test-common.sh +++ b/.github/buildomat/packet-test-common.sh @@ -106,8 +106,7 @@ cargo test \ --no-fail-fast \ $TESTNAME \ -- \ - --ignored \ - --skip succeeds_when_table_fragmented + --ignored popd diff --git a/asic/src/tofino_common/mod.rs b/asic/src/tofino_common/mod.rs index b6f56fc6..c17add4b 100644 --- a/asic/src/tofino_common/mod.rs +++ b/asic/src/tofino_common/mod.rs @@ -15,7 +15,7 @@ use aal::AsicError; use aal::AsicResult; use aal::MatchType; use common::counters::CounterId; -use common::counters::MulticastCounterId; +use common::counters::EgressCounterId; use common::table::TableType; pub mod ports; @@ -89,22 +89,20 @@ fn counter_table_name(id: CounterId) -> &'static str { CounterId::Egress => "pipe.Ingress.egress_ctr", CounterId::DropPort => "pipe.Ingress.drop_port_ctr", CounterId::DropReason => "pipe.Ingress.drop_reason_ctr", - CounterId::Multicast(id) => mulitcast_counter_table_name(id), + CounterId::EgressPipeline(id) => egress_counter_table_name(id), } } -fn mulitcast_counter_table_name(id: MulticastCounterId) -> &'static str { +fn egress_counter_table_name(id: EgressCounterId) -> &'static str { match id { - MulticastCounterId::EgressDropPort => "pipe.Egress.drop_port_ctr", - MulticastCounterId::EgressDropReason => "pipe.Egress.drop_reason_ctr", - MulticastCounterId::Unicast => "pipe.Egress.unicast_ctr", - MulticastCounterId::Multicast => "pipe.Egress.mcast_ctr", - MulticastCounterId::MulticastExt => "pipe.Egress.external_mcast_ctr", - MulticastCounterId::MulticastLL => "pipe.Egress.link_local_mcast_ctr", - MulticastCounterId::MulticastUL => "pipe.Egress.underlay_mcast_ctr", - MulticastCounterId::MulticastDrop => { - "pipe.Ingress.filter.drop_mcast_ctr" - } + EgressCounterId::DropPort => "pipe.Egress.drop_port_ctr", + EgressCounterId::DropReason => "pipe.Egress.drop_reason_ctr", + EgressCounterId::Unicast => "pipe.Egress.unicast_ctr", + EgressCounterId::Multicast => "pipe.Egress.mcast_ctr", + EgressCounterId::MulticastExt => "pipe.Egress.external_mcast_ctr", + EgressCounterId::MulticastLL => "pipe.Egress.link_local_mcast_ctr", + EgressCounterId::MulticastUL => "pipe.Egress.underlay_mcast_ctr", + EgressCounterId::MulticastDrop => "pipe.Ingress.filter.drop_mcast_ctr", } } diff --git a/common/src/counters.rs b/common/src/counters.rs index eb89eb6d..b5cfd127 100644 --- a/common/src/counters.rs +++ b/common/src/counters.rs @@ -33,7 +33,7 @@ pub enum CounterId { Packet, DropPort, DropReason, - Multicast(MulticastCounterId), + EgressPipeline(EgressCounterId), } #[derive( @@ -48,9 +48,9 @@ pub enum CounterId { Serialize, Deserialize, )] -pub enum MulticastCounterId { - EgressDropPort, - EgressDropReason, +pub enum EgressCounterId { + DropPort, + DropReason, Unicast, Multicast, MulticastExt, @@ -71,7 +71,7 @@ impl fmt::Display for CounterId { CounterId::Packet => "Packet".to_string(), CounterId::DropPort => "Ingress_Drop_Port".to_string(), CounterId::DropReason => "Ingress_Drop_Reason".to_string(), - CounterId::Multicast(id) => id.to_string(), + CounterId::EgressPipeline(id) => id.to_string(), } ) } @@ -89,45 +89,47 @@ impl std::str::FromStr for CounterId { "ingressdropport" => Ok(CounterId::DropPort), "ingressdropreason" => Ok(CounterId::DropReason), "egressdropport" => { - Ok(CounterId::Multicast(MulticastCounterId::EgressDropPort)) + Ok(CounterId::EgressPipeline(EgressCounterId::DropPort)) } "egressdropreason" => { - Ok(CounterId::Multicast(MulticastCounterId::EgressDropReason)) + Ok(CounterId::EgressPipeline(EgressCounterId::DropReason)) + } + "unicast" => { + Ok(CounterId::EgressPipeline(EgressCounterId::Unicast)) } - "unicast" => Ok(CounterId::Multicast(MulticastCounterId::Unicast)), "multicast" => { - Ok(CounterId::Multicast(MulticastCounterId::Multicast)) + Ok(CounterId::EgressPipeline(EgressCounterId::Multicast)) } "multicastext" | "multicastexternal" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastExt)) + Ok(CounterId::EgressPipeline(EgressCounterId::MulticastExt)) } "multicastll" | "multicastlinklocal" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastLL)) + Ok(CounterId::EgressPipeline(EgressCounterId::MulticastLL)) } "multicastul" | "multicastunderlay" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastUL)) + Ok(CounterId::EgressPipeline(EgressCounterId::MulticastUL)) } "multicastdrop" => { - Ok(CounterId::Multicast(MulticastCounterId::MulticastDrop)) + Ok(CounterId::EgressPipeline(EgressCounterId::MulticastDrop)) } x => Err(format!("No such counter: {x}")), } } } -impl fmt::Display for MulticastCounterId { +impl fmt::Display for EgressCounterId { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!( f, "{}", match self { - MulticastCounterId::EgressDropPort => "Egress_Drop_Port", - MulticastCounterId::EgressDropReason => "Egress_Drop_Reason", - MulticastCounterId::Unicast => "Unicast", - MulticastCounterId::Multicast => "Multicast", - MulticastCounterId::MulticastExt => "Multicast_External", - MulticastCounterId::MulticastLL => "Multicast_Link_Local", - MulticastCounterId::MulticastUL => "Multicast_Underlay", - MulticastCounterId::MulticastDrop => "Multicast_Drop", + EgressCounterId::DropPort => "Egress_Drop_Port", + EgressCounterId::DropReason => "Egress_Drop_Reason", + EgressCounterId::Unicast => "Unicast", + EgressCounterId::Multicast => "Multicast", + EgressCounterId::MulticastExt => "Multicast_External", + EgressCounterId::MulticastLL => "Multicast_Link_Local", + EgressCounterId::MulticastUL => "Multicast_Underlay", + EgressCounterId::MulticastDrop => "Multicast_Drop", } ) } diff --git a/dpd/p4/constants.p4 b/dpd/p4/constants.p4 index 225301b6..1f0b1cea 100644 --- a/dpd/p4/constants.p4 +++ b/dpd/p4/constants.p4 @@ -6,8 +6,6 @@ const bit<16> L2_ISOLATED_FLAG = 0x8000; -// TODO: these all need to be bigger. Early experimentation is showing that this -// is going to need to come either through ATCAM/ALPM or code restructuring. const int IPV4_NAT_TABLE_SIZE = 1024; // nat routing table const int IPV6_NAT_TABLE_SIZE = 1024; // nat routing table const int IPV4_LPM_SIZE = 8192; // ipv4 forwarding table diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 37a394cd..414a6ae3 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -922,12 +922,12 @@ control RouterLookupIndex6( * test to pass. We want the lookup and forward tables to have the same * capacity from dpd's perspective, and the "default" entry consumes a * slot in the lookup table. - * - * Note: we annotate @ways here, increasing cuckoo placement choices to 8 - * candidate buckets per key. Full occupancy of the route-target table - * is a measured result for this build, not a compiler guarantee. - * - * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. + * + * Note: we annotate @ways here, increasing cuckoo placement choices to 8 + * candidate buckets per key. Full occupancy of the route-target table + * is a measured result for this build, not a compiler guarantee. + * + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. */ @ways(8) table route { @@ -1050,12 +1050,12 @@ control RouterLookupIndex4( * test to pass. We want the lookup and forward tables to have the same * capacity from dpd's perspective, and the "default" entry consumes a * slot in the lookup table. - * - * Note: we annotate @ways here, increasing cuckoo placement choices to 8 - * candidate buckets per key. Full occupancy of the route-target table - * is a measured result for this build, not a compiler guarantee. - * - * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. + * + * Note: we annotate @ways here, increasing cuckoo placement choices to 8 + * candidate buckets per key. Full occupancy of the route-target table + * is a measured result for this build, not a compiler guarantee. + * + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. */ @ways(8) table route { diff --git a/dpd/src/api_server.rs b/dpd/src/api_server.rs index e49a0198..061563df 100644 --- a/dpd/src/api_server.rs +++ b/dpd/src/api_server.rs @@ -1934,10 +1934,7 @@ impl DpdApi for DpdApiImpl { _rqctx: RequestContext>, ) -> Result>, HttpError> { Ok(HttpResponseOk( - counters::get_counter_ids() - .iter() - .map(|id| id.to_string()) - .collect(), + counters::COUNTER_IDS.iter().map(|id| id.to_string()).collect(), )) } diff --git a/dpd/src/counters.rs b/dpd/src/counters.rs index 9185e3e7..52efccf4 100644 --- a/dpd/src/counters.rs +++ b/dpd/src/counters.rs @@ -23,7 +23,7 @@ use aal::MatchParse; use aal_macros::*; use asic::Handle; use common::counters::CounterId; -use common::counters::MulticastCounterId; +use common::counters::EgressCounterId; use common::table::TableType; use anyhow::Context; @@ -45,24 +45,22 @@ pub struct Counter { table: table::Table, } -pub fn get_counter_ids() -> Vec { - vec![ - CounterId::Service, - CounterId::Ingress, - CounterId::Egress, - CounterId::Packet, - CounterId::DropPort, - CounterId::DropReason, - CounterId::Multicast(MulticastCounterId::EgressDropPort), - CounterId::Multicast(MulticastCounterId::EgressDropReason), - CounterId::Multicast(MulticastCounterId::Unicast), - CounterId::Multicast(MulticastCounterId::Multicast), - CounterId::Multicast(MulticastCounterId::MulticastExt), - CounterId::Multicast(MulticastCounterId::MulticastLL), - CounterId::Multicast(MulticastCounterId::MulticastUL), - CounterId::Multicast(MulticastCounterId::MulticastDrop), - ] -} +pub const COUNTER_IDS: &[CounterId] = &[ + CounterId::Service, + CounterId::Ingress, + CounterId::Egress, + CounterId::Packet, + CounterId::DropPort, + CounterId::DropReason, + CounterId::EgressPipeline(EgressCounterId::DropPort), + CounterId::EgressPipeline(EgressCounterId::DropReason), + CounterId::EgressPipeline(EgressCounterId::Unicast), + CounterId::EgressPipeline(EgressCounterId::Multicast), + CounterId::EgressPipeline(EgressCounterId::MulticastExt), + CounterId::EgressPipeline(EgressCounterId::MulticastLL), + CounterId::EgressPipeline(EgressCounterId::MulticastUL), + CounterId::EgressPipeline(EgressCounterId::MulticastDrop), +]; /// Fetch a counter by name from the switch's list of counters. This call /// returns a pointer to the mutex that controls access to the counter, and the @@ -316,16 +314,16 @@ pub async fn get_values( port_label(switch, idx.idx).await } CounterId::DropReason => reason_label(idx.idx as u8)?, - CounterId::Multicast(MulticastCounterId::EgressDropPort) - | CounterId::Multicast(MulticastCounterId::Unicast) - | CounterId::Multicast(MulticastCounterId::Multicast) - | CounterId::Multicast(MulticastCounterId::MulticastExt) - | CounterId::Multicast(MulticastCounterId::MulticastLL) - | CounterId::Multicast(MulticastCounterId::MulticastUL) - | CounterId::Multicast(MulticastCounterId::MulticastDrop) => { + CounterId::EgressPipeline(EgressCounterId::DropPort) + | CounterId::EgressPipeline(EgressCounterId::Unicast) + | CounterId::EgressPipeline(EgressCounterId::Multicast) + | CounterId::EgressPipeline(EgressCounterId::MulticastExt) + | CounterId::EgressPipeline(EgressCounterId::MulticastLL) + | CounterId::EgressPipeline(EgressCounterId::MulticastUL) + | CounterId::EgressPipeline(EgressCounterId::MulticastDrop) => { port_label(switch, idx.idx).await } - CounterId::Multicast(MulticastCounterId::EgressDropReason) => { + CounterId::EgressPipeline(EgressCounterId::DropReason) => { reason_label(idx.idx as u8)? } }; @@ -363,7 +361,7 @@ pub fn init( ) -> anyhow::Result>> { let mut counters = BTreeMap::new(); - for id in get_counter_ids() { + for &id in COUNTER_IDS { let name = id.to_string(); counters.insert( id, From d285fdba5bdf562598becb51809162b2b259fceb Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Tue, 15 Sep 2026 00:12:52 +0000 Subject: [PATCH 4/6] [review] fixups + drop bypass_egress + count fwd'ed packets in egress This work follows up on Kyle's review, including adding documentation on `@ways` defaults (and now refers to a more informational link) and removing leftover, unnecessary `{}` blocks. The main change is that every packet copy now runs through to the egress pipeline, no longer relying on `bypass_egress` in the ingress pipeline. This follows from #208 and makes the counters more explicit: - `forwarded_ctr` records all packets leaving a port; - `unicast_ctr` and `mcast_ctr` now partition what gets forwarded by type; - drops are recorded separately with port and reason counters. --- asic/src/tofino_asic/mod.rs | 4 +- asic/src/tofino_common/mod.rs | 2 +- common/src/counters.rs | 6 +-- dpd/p4/constants.p4 | 2 + dpd/p4/sidecar.p4 | 58 ++++++++++++++++++----------- dpd/src/counters.rs | 4 +- dpd/src/link.rs | 14 ++----- dpd/src/macaddrs.rs | 6 +-- swadm/tests/cli/expect/counters.txt | 2 +- 9 files changed, 52 insertions(+), 46 deletions(-) diff --git a/asic/src/tofino_asic/mod.rs b/asic/src/tofino_asic/mod.rs index 7953e22b..c26a0fd2 100644 --- a/asic/src/tofino_asic/mod.rs +++ b/asic/src/tofino_asic/mod.rs @@ -403,9 +403,7 @@ impl Handle { // Note: we assume that bf_mc_init() has been called as part of the // bf_switch_init() operation. - { - bf.mcast_hdl = mcast::create_session()?; - } + bf.mcast_hdl = mcast::create_session()?; Ok(Handle { dev_id, diff --git a/asic/src/tofino_common/mod.rs b/asic/src/tofino_common/mod.rs index c17add4b..bd86d12d 100644 --- a/asic/src/tofino_common/mod.rs +++ b/asic/src/tofino_common/mod.rs @@ -86,7 +86,7 @@ fn counter_table_name(id: CounterId) -> &'static str { CounterId::Service => "pipe.Ingress.services.service_ctr", CounterId::Ingress => "pipe.Ingress.ingress_ctr", CounterId::Packet => "pipe.Ingress.packet_ctr", - CounterId::Egress => "pipe.Ingress.egress_ctr", + CounterId::Forwarded => "pipe.Egress.forwarded_ctr", CounterId::DropPort => "pipe.Ingress.drop_port_ctr", CounterId::DropReason => "pipe.Ingress.drop_reason_ctr", CounterId::EgressPipeline(id) => egress_counter_table_name(id), diff --git a/common/src/counters.rs b/common/src/counters.rs index b5cfd127..48bcb3e4 100644 --- a/common/src/counters.rs +++ b/common/src/counters.rs @@ -29,7 +29,7 @@ pub use dpd_types::counters::{ pub enum CounterId { Service, Ingress, - Egress, + Forwarded, Packet, DropPort, DropReason, @@ -67,7 +67,7 @@ impl fmt::Display for CounterId { match self { CounterId::Service => "Service".to_string(), CounterId::Ingress => "Ingress".to_string(), - CounterId::Egress => "Egress".to_string(), + CounterId::Forwarded => "Forwarded".to_string(), CounterId::Packet => "Packet".to_string(), CounterId::DropPort => "Ingress_Drop_Port".to_string(), CounterId::DropReason => "Ingress_Drop_Reason".to_string(), @@ -84,7 +84,7 @@ impl std::str::FromStr for CounterId { match s.to_lowercase().replace(['_'], "").as_str() { "service" => Ok(CounterId::Service), "ingress" => Ok(CounterId::Ingress), - "egress" => Ok(CounterId::Egress), + "forwarded" => Ok(CounterId::Forwarded), "packet" => Ok(CounterId::Packet), "ingressdropport" => Ok(CounterId::DropPort), "ingressdropreason" => Ok(CounterId::DropReason), diff --git a/dpd/p4/constants.p4 b/dpd/p4/constants.p4 index 1f0b1cea..225301b6 100644 --- a/dpd/p4/constants.p4 +++ b/dpd/p4/constants.p4 @@ -6,6 +6,8 @@ const bit<16> L2_ISOLATED_FLAG = 0x8000; +// TODO: these all need to be bigger. Early experimentation is showing that this +// is going to need to come either through ATCAM/ALPM or code restructuring. const int IPV4_NAT_TABLE_SIZE = 1024; // nat routing table const int IPV6_NAT_TABLE_SIZE = 1024; // nat routing table const int IPV4_LPM_SIZE = 8192; // ipv4 forwarding table diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 414a6ae3..ef7ccbc1 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -925,9 +925,12 @@ control RouterLookupIndex6( * * Note: we annotate @ways here, increasing cuckoo placement choices to 8 * candidate buckets per key. Full occupancy of the route-target table - * is a measured result for this build, not a compiler guarantee. + * was measured on this build; the compiler does not guarantee it. * - * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. + * By default, without this pragma, p4c computes the way count, bumping + * it up to 4 independent ways when it comes out lower. + * + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/mau/resource_estimate.cpp#L570-L620 */ @ways(8) table route { @@ -1053,9 +1056,12 @@ control RouterLookupIndex4( * * Note: we annotate @ways here, increasing cuckoo placement choices to 8 * candidate buckets per key. Full occupancy of the route-target table - * is a measured result for this build, not a compiler guarantee. + * was measured on this build; the compiler does not guarantee it. + * + * By default, without this pragma, p4c computes the way count, bumping + * it up to 4 independent ways when it comes out lower. * - * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183. + * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/mau/resource_estimate.cpp#L570-L620 */ @ways(8) table route { @@ -2075,7 +2081,6 @@ control Ingress( MacRewrite() mac_rewrite; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) ingress_ctr; - Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) egress_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS) drop_port_ctr; Counter, bit<8>>(DROP_REASON_MAX, CounterType_t.PACKETS) drop_reason_ctr; Counter, bit<10>>(1024, CounterType_t.PACKETS) packet_ctr; @@ -2128,15 +2133,11 @@ control Ingress( drop_port_ctr.count(ig_intr_md.ingress_port); drop_reason_ctr.count(meta.drop_reason); } else if (!meta.is_mcast) { - egress_ctr.count(ig_tm_md.ucast_egress_port); if (ig_tm_md.ucast_egress_port != USER_SPACE_SERVICE_PORT) { mac_rewrite.apply(hdr, ig_tm_md.ucast_egress_port); } if (meta.nat_egress_hit && !meta.service_routed) { meta.bridge_hdr.nat_egress_hit = true; - } else { - meta.bridge_hdr.setInvalid(); - ig_tm_md.bypass_egress = 1w1; } } @@ -2302,6 +2303,15 @@ control Egress( MulticastMacRewrite() mac_rewrite; MulticastEgress() mcast_egress; + // The Ingress pipeline never sets bypass_egress; every packet copy + // that is not dropped traverses this pipeline and is counted exactly + // once: + // + // - forwarded_ctr records all packets leaving a port + // - unicast_ctr and mcast_ctr partition what gets forwarded by type + // - drops are recorded separately in drop_port_ctr and + // drop_reason_ctr + Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) forwarded_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) unicast_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) mcast_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) link_local_mcast_ctr; @@ -2343,20 +2353,24 @@ control Egress( drop_port_ctr.count(eg_intr_md.egress_port); drop_reason_ctr.count(meta.drop_reason); eg_dprsr_md.drop_ctl = 1; - } else if (is_mcast == true) { - mcast_ctr.count(eg_intr_md.egress_port); - - if (is_link_local_ipv6_mcast) { - link_local_mcast_ctr.count(eg_intr_md.egress_port); - } else if (hdr.geneve.isValid()) { - external_mcast_ctr.count(eg_intr_md.egress_port); - } else if (hdr.geneve.isValid() && - hdr.geneve_opts.oxg_mcast.isValid() && - hdr.geneve_opts.oxg_mcast.mcast_tag == MULTICAST_TAG_UNDERLAY) { - underlay_mcast_ctr.count(eg_intr_md.egress_port); - } } else { - unicast_ctr.count(eg_intr_md.egress_port); + forwarded_ctr.count(eg_intr_md.egress_port); + + if (is_mcast == true) { + mcast_ctr.count(eg_intr_md.egress_port); + + if (is_link_local_ipv6_mcast) { + link_local_mcast_ctr.count(eg_intr_md.egress_port); + } else if (hdr.geneve.isValid()) { + external_mcast_ctr.count(eg_intr_md.egress_port); + } else if (hdr.geneve.isValid() && + hdr.geneve_opts.oxg_mcast.isValid() && + hdr.geneve_opts.oxg_mcast.mcast_tag == MULTICAST_TAG_UNDERLAY) { + underlay_mcast_ctr.count(eg_intr_md.egress_port); + } + } else { + unicast_ctr.count(eg_intr_md.egress_port); + } } } } diff --git a/dpd/src/counters.rs b/dpd/src/counters.rs index 52efccf4..b189cb6f 100644 --- a/dpd/src/counters.rs +++ b/dpd/src/counters.rs @@ -48,7 +48,7 @@ pub struct Counter { pub const COUNTER_IDS: &[CounterId] = &[ CounterId::Service, CounterId::Ingress, - CounterId::Egress, + CounterId::Forwarded, CounterId::Packet, CounterId::DropPort, CounterId::DropReason, @@ -310,7 +310,7 @@ pub async fn get_values( let key = match counter_id { CounterId::Packet => packet_label(idx.idx), CounterId::Service => service_label(idx.idx as u8), - CounterId::Ingress | CounterId::Egress | CounterId::DropPort => { + CounterId::Ingress | CounterId::Forwarded | CounterId::DropPort => { port_label(switch, idx.idx).await } CounterId::DropReason => reason_label(idx.idx as u8)?, diff --git a/dpd/src/link.rs b/dpd/src/link.rs index 383e18bc..61953b53 100644 --- a/dpd/src/link.rs +++ b/dpd/src/link.rs @@ -1627,21 +1627,15 @@ fn set_mac_config( mac: MacAddr, ) -> DpdResult<()> { mac::mac_set(switch, asic_id, mac)?; - - { - mac::mcast_mac_set(switch, asic_id, mac)?; - mcast::mcast_egress::add_port_mapping_entry(switch, asic_id)?; - } + mac::mcast_mac_set(switch, asic_id, mac)?; + mcast::mcast_egress::add_port_mapping_entry(switch, asic_id)?; Ok(()) } fn clear_mac_config(switch: &Switch, asic_id: AsicId) -> DpdResult<()> { mac::mac_clear(switch, asic_id)?; - - { - mac::mcast_mac_clear(switch, asic_id)?; - mcast::mcast_egress::del_port_mapping_entry(switch, asic_id)?; - } + mac::mcast_mac_clear(switch, asic_id)?; + mcast::mcast_egress::del_port_mapping_entry(switch, asic_id)?; Ok(()) } diff --git a/dpd/src/macaddrs.rs b/dpd/src/macaddrs.rs index 10efb51f..f75b28c3 100644 --- a/dpd/src/macaddrs.rs +++ b/dpd/src/macaddrs.rs @@ -433,10 +433,8 @@ impl Switch { // Reset ingress and egress MAC tables and Port ID table(s). mac::reset(self)?; - { - mac::mcast_reset(self)?; - mcast::mcast_egress::reset_bitmap_table(self)?; - } + mac::mcast_reset(self)?; + mcast::mcast_egress::reset_bitmap_table(self)?; // Create the link on the CPU port. let link_id = self.create_link(port_id, ¶ms)?; diff --git a/swadm/tests/cli/expect/counters.txt b/swadm/tests/cli/expect/counters.txt index 5cf15de9..b3afee85 100644 --- a/swadm/tests/cli/expect/counters.txt +++ b/swadm/tests/cli/expect/counters.txt @@ -1 +1 @@ -["Service", "Ingress", "Egress", "Packet", "Ingress_Drop_Port", "Ingress_Drop_Reason", "Egress_Drop_Port", "Egress_Drop_Reason", "Unicast", "Multicast", "Multicast_External", "Multicast_Link_Local", "Multicast_Underlay", "Multicast_Drop"] +["Service", "Ingress", "Forwarded", "Packet", "Ingress_Drop_Port", "Ingress_Drop_Reason", "Egress_Drop_Port", "Egress_Drop_Reason", "Unicast", "Multicast", "Multicast_External", "Multicast_Link_Local", "Multicast_Underlay", "Multicast_Drop"] From bc40ea7a55a38a3d23393632f62611c55da4969b Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sat, 17 Oct 2026 04:15:00 +0000 Subject: [PATCH 5/6] [review, p4] limit egress Geneve parsing to underlay multicast > In my understanding, we only parse the IPv4 header on egress to check > geneve options. And afaik an Oxide geneve header should only ever live > behind an outer IPv6 header. So can we just skip parsing an L4 header > on egress for IPv4 packets? Because our spec asserts there's never a > geneve header within an IPv4 packet? This was a good point. The egress pipeline only needs Geneve handling for underlay mcast replicas (outer hdr with `ff04::/64`, always IPv6), but we were walking `IPv4` -> `UDP` -> `Geneve` for UDP traffic to the Geneve port (6081), and similarly for IPv6, where decapped external replicas or customer traffic to port 6081 could end up in the Geneve parser. For IPv4, options or a later fragment's payload could have been read as UDP, with an apparent dst port of 6081 transitioning into the Geneve parser state. Now, we get: - IPv4: extract the header and accept - IPv6: only goes on to UDP for `ff04::/64` + UDP. --- dpd/p4/parser.p4 | 48 ++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 6 deletions(-) diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index f90668b6..3bb0f100 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -590,20 +590,56 @@ parser EgressParser( } } + // Egress only needs L4 and Geneve for underlay multicast replicas, + // which are always IPv6 packets. Stopping at the IPv4 header avoids + // misreading options or fragments as UDP. state parse_ipv4 { pkt.extract(hdr.ipv4); + transition accept; + } - transition select(hdr.ipv4.protocol) { - IPPROTO_UDP: parse_udp; + // Egress only needs Geneve parsed for underlay multicast (ff04::/64, our + // internal block). NAT ingress traffic gets encapsulated too, but nothing + // in the egress controls reads its Geneve header, and all other IPv6 + // stops after the IPv6 header. + // + // An egress reject doesn't drop the packet (TNA 5.2.1): + // https://raw.githubusercontent.com/barefootnetworks/Open-Tofino/2c83863fd6665cf29dcc64381ee2c6ce8e4552d8/PUBLIC_Tofino-Native-Arch.pdf#page=20 + state parse_ipv6 { + pkt.extract(hdr.ipv6); + + // Tofino 2 has four 8-bit parser match registers: + // https://github.com/p4lang/p4c/blob/279e939673457bc09a5ae9ed3337f6e8d4d93bf4/backends/tofino/bf-p4c/specs/parde_spec.h#L591-L600 + // + // This equates to 32 bits per state, which is too few for the 64-bit + // ff04::/64 prefix we rely on (plus the next_hdr). Instead, we check + // 16 bits per state, with next_hdr checked in the initial state. + // The later states could take 32 bits, but keeping at 16 gives them + // uniformity. + transition select(hdr.ipv6.dst_addr[127:112], hdr.ipv6.next_hdr) { + (IPV6_UNDERLAY_MULTICAST_PATTERN[127:112], IPPROTO_UDP): + check_underlay_dst_111_96; default: accept; } } - state parse_ipv6 { - pkt.extract(hdr.ipv6); + state check_underlay_dst_111_96 { + transition select(hdr.ipv6.dst_addr[111:96]) { + IPV6_UNDERLAY_MULTICAST_PATTERN[111:96]: check_underlay_dst_95_80; + default: accept; + } + } - transition select(hdr.ipv6.next_hdr) { - IPPROTO_UDP: parse_udp; + state check_underlay_dst_95_80 { + transition select(hdr.ipv6.dst_addr[95:80]) { + IPV6_UNDERLAY_MULTICAST_PATTERN[95:80]: check_underlay_dst_79_64; + default: accept; + } + } + + state check_underlay_dst_79_64 { + transition select(hdr.ipv6.dst_addr[79:64]) { + IPV6_UNDERLAY_MULTICAST_PATTERN[79:64]: parse_udp; default: accept; } } From f6163db0b5e77fa88ad964f78eb11ef45dd5a9e9 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Thu, 22 Oct 2026 15:34:45 +0000 Subject: [PATCH 6/6] [review] simplify branching --- dpd/p4/parser.p4 | 45 +++++++++------------------------------------ 1 file changed, 9 insertions(+), 36 deletions(-) diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index 7e291bb2..73bae827 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -583,48 +583,21 @@ parser EgressParser( transition accept; } - // Egress only needs Geneve parsed for underlay multicast (ff04::/64, our - // internal block). NAT ingress traffic gets encapsulated too, but nothing - // in the egress controls reads its Geneve header, and all other IPv6 - // stops after the IPv6 header. + // Egress only needs Geneve parsed for underlay multicast replicas. A + // non-zero egress_rid means the PRE replicated the packet. Ingress + // replicates only when the outer dst hits an internal group (ff04::/64) + // in `mcast_replication_ipv6`. NAT ingress traffic gets encapsulated + // as well, but the egress pipeline doesn't need to read its Geneve + // header, while all other IPv6 stops after the IPv6 header. // // An egress reject doesn't drop the packet (TNA 5.2.1): // https://raw.githubusercontent.com/barefootnetworks/Open-Tofino/2c83863fd6665cf29dcc64381ee2c6ce8e4552d8/PUBLIC_Tofino-Native-Arch.pdf#page=20 state parse_ipv6 { pkt.extract(hdr.ipv6); - // Tofino 2 has four 8-bit parser match registers: - // https://github.com/p4lang/p4c/blob/279e939673457bc09a5ae9ed3337f6e8d4d93bf4/backends/tofino/bf-p4c/specs/parde_spec.h#L591-L600 - // - // This equates to 32 bits per state, which is too few for the 64-bit - // ff04::/64 prefix we rely on (plus the next_hdr). Instead, we check - // 16 bits per state, with next_hdr checked in the initial state. - // The later states could take 32 bits, but keeping at 16 gives them - // uniformity. - transition select(hdr.ipv6.dst_addr[127:112], hdr.ipv6.next_hdr) { - (IPV6_UNDERLAY_MULTICAST_PATTERN[127:112], IPPROTO_UDP): - check_underlay_dst_111_96; - default: accept; - } - } - - state check_underlay_dst_111_96 { - transition select(hdr.ipv6.dst_addr[111:96]) { - IPV6_UNDERLAY_MULTICAST_PATTERN[111:96]: check_underlay_dst_95_80; - default: accept; - } - } - - state check_underlay_dst_95_80 { - transition select(hdr.ipv6.dst_addr[95:80]) { - IPV6_UNDERLAY_MULTICAST_PATTERN[95:80]: check_underlay_dst_79_64; - default: accept; - } - } - - state check_underlay_dst_79_64 { - transition select(hdr.ipv6.dst_addr[79:64]) { - IPV6_UNDERLAY_MULTICAST_PATTERN[79:64]: parse_udp; + transition select(eg_intr_md.egress_rid, hdr.ipv6.next_hdr) { + (0, _): accept; + (_, IPPROTO_UDP): parse_udp; default: accept; } }