From e466bee28282b53a2181b3b4355e81bc67d3579d Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Tue, 6 Oct 2026 12:14:22 +0100 Subject: [PATCH 1/8] Set Geneve encap source port using flow hash This PR moves computation of any packet's flow hash (previously used for ECMP in the Router4/Router6 controls) to the head of packet processing. This allows us to use one flow hash to fill out the UDP source port for Geneve packets, and for ECMP as before. This is slightly different to what we had before in two ways. The first is that this hash includes the IpProtocol value, and the second is that we will use the outer flow hash rather than the inner when decapping and forwarding. Part of why this is still valid is that OPTE already sets the encap source port to the flow hash, so hashing early still includes the entropy from the inner flow hash. This ended up needing to be done because I couldn't convince p4c to let me place an extra two `Hash`es into `NatIngress`. This also fixes some of the external subnets & NAT unit ingress tests, which weren't *actually* generating the packet types we would have expected for each (we got all UDP, apparently!). Fixes #324. --- Cargo.lock | 17 ++- Cargo.toml | 1 + dpd-client/Cargo.toml | 2 +- .../integration_tests/attached_subnet.rs | 45 +++--- dpd-client/tests/integration_tests/common.rs | 73 +++++++++- dpd-client/tests/integration_tests/nat.rs | 128 +++++++----------- .../tests/integration_tests/route_ipv4.rs | 17 +-- .../tests/integration_tests/route_ipv6.rs | 17 +-- .../tests/integration_tests/table_tests.rs | 4 +- dpd/p4/metadata.p4 | 1 + dpd/p4/parser.p4 | 36 ++++- dpd/p4/sidecar.p4 | 96 +++++++------ packet/src/icmp.rs | 11 +- 13 files changed, 271 insertions(+), 177 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1ef80514..b2d7ad38 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1154,6 +1154,21 @@ dependencies = [ "thiserror 1.0.69", ] +[[package]] +name = "crc" +version = "3.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5eb8a2a1cd12ab0d987a5d5e825195d372001a4094a0376319d5a0ad71c1ba0d" +dependencies = [ + "crc-catalog", +] + +[[package]] +name = "crc-catalog" +version = "2.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "217698eaf96b4a3f0bc4f3662aaa55bdf913cd54d7204591faa790070c6d0853" + [[package]] name = "crc32fast" version = "1.5.0" @@ -1731,7 +1746,7 @@ dependencies = [ "async-trait", "chrono", "common 0.1.0", - "crc8", + "crc", "dpd-types", "futures", "http", diff --git a/Cargo.toml b/Cargo.toml index a8f11cfc..4629063d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -69,6 +69,7 @@ cfg-if = "1" chrono = "0.4" clap = { version = "4.6", features = [ "derive" ] } colored = "3" +crc = "3.4.0" csv = "1.3" curl = "0.4" dropshot = "0.17.1" diff --git a/dpd-client/Cargo.toml b/dpd-client/Cargo.toml index 7998da4d..a0901737 100644 --- a/dpd-client/Cargo.toml +++ b/dpd-client/Cargo.toml @@ -12,7 +12,6 @@ tofino_asic = ["asic/tofino_asic"] [dependencies] async-trait = "0.1.89" chrono.workspace = true -crc8 = "0.1.1" futures.workspace = true http.workspace = true oxnet.workspace = true @@ -34,6 +33,7 @@ packet = { path = "../packet" } pcap = { path = "../pcap" } asic = { path = "../asic" } anyhow.workspace = true +crc.workspace = true dpd-types.workspace = true lazy_static.workspace = true parking_lot.workspace = true diff --git a/dpd-client/tests/integration_tests/attached_subnet.rs b/dpd-client/tests/integration_tests/attached_subnet.rs index 19f7ff96..43a85180 100644 --- a/dpd-client/tests/integration_tests/attached_subnet.rs +++ b/dpd-client/tests/integration_tests/attached_subnet.rs @@ -63,13 +63,6 @@ async fn test_api() -> TestResult { Ok(()) } -#[derive(Debug)] -enum L4Protocol { - Tcp, - Udp, - Icmp, -} - #[derive(Debug)] struct ExternalTest { // packet source/destination from the vm client's perspective @@ -246,17 +239,25 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { // external subnet. let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; let switch_mac = switch.get_port_mac(test.uplink_port).unwrap().to_string(); - let ingress_pkt = common::gen_udp_packet_loaded( - Endpoint::parse( - &test.upstream_router_mac, - &test.pkt_src_ip, - test.pkt_src_port, - ) - .unwrap(), - Endpoint::parse(&switch_mac, &test.pkt_dst_ip, test.pkt_dst_port) - .unwrap(), - &load, - ); + let src = Endpoint::parse( + &test.upstream_router_mac, + &test.pkt_src_ip, + test.pkt_src_port, + ) + .unwrap(); + let dst = Endpoint::parse(&switch_mac, &test.pkt_dst_ip, test.pkt_dst_port) + .unwrap(); + let (ingress_pkt, csum_generated) = match test.l4_protocol { + L4Protocol::Tcp => { + (common::gen_tcp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Udp => { + (common::gen_udp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Icmp => { + (common::gen_icmp_packet_loaded(src, dst, &load), false) + } + }; // Convert the packet into a binary payload that will be encapsulated into a // geneve packet. We also rewrite the destination mac of the packet to @@ -283,7 +284,7 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { Endpoint::parse( &backplane_port_mac, switch_port_ip, - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash(src, dst, test.l4_protocol), ) .unwrap(), Endpoint::parse( @@ -299,7 +300,11 @@ async fn test_ingress(switch: &Switch, test: &ExternalTest) -> TestResult { /* Adjust for transition from switch port to gimlet port */ ipv6::Ipv6Hdr::adjust_hlim(&mut forward_pkt, -1); - udp::UdpHdr::update_checksum(&mut forward_pkt); + if csum_generated { + udp::UdpHdr::update_checksum(&mut forward_pkt); + } else { + forward_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; + } let send = vec![TestPacket { packet: Arc::new(ingress_pkt), diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 3d50f82a..4a25c1e8 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -5,6 +5,7 @@ // Copyright 2026 Oxide Computer Company use std::fmt::Write; +use std::io::Write as IoWrite; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -23,7 +24,6 @@ use dpd_client::Client; use dpd_client::ClientInfo; use dpd_client::ClientState; use dpd_client::types; -use packet::Endpoint; use packet::Packet; use packet::arp; use packet::eth; @@ -31,6 +31,7 @@ use packet::icmp; use packet::ipv4; use packet::ipv6; use packet::sidecar; +use packet::{Endpoint, L4Endpoint}; use types::PortId; const SHOW_VERBOSE: u8 = 0x01; @@ -50,6 +51,15 @@ const TEST_PCAP_TIMEOUT_MS: i32 = 1; pub struct PhysPort(pub u16); pub const NO_PORT: PhysPort = PhysPort(0xffff); +/// The type of an L4 packet carried over IP +#[repr(u8)] +#[derive(Copy, Clone, Debug, Eq, PartialEq)] +pub enum L4Protocol { + Tcp = ipv4::IPPROTO_TCP, + Udp = ipv4::IPPROTO_UDP, + Icmp = ipv4::IPPROTO_ICMP, +} + // On real hardware the "service port" is the PCI port, which doesn't have a // real physical port - it's just a collection of ringbufs in PCI space. On the // model, we have "physical port" 33 wired to carry the traffic for "tofino @@ -856,15 +866,23 @@ pub fn gen_udp_packet_loaded( Packet::generate(src, dst, udp_stack, Some(body)).unwrap() } -// Construct a single ICMP packet with an optional payload +// Construct a single ICMP echo request packet with an optional payload. pub fn gen_icmp_packet_loaded( src: Endpoint, dst: Endpoint, body: &[u8], ) -> Packet { let icmp_stack = match src.get_ip("src").unwrap() { - IpAddr::V4(_) => vec![ipv4::IPPROTO_ICMP.into(), eth::ETHER_IPV4], - IpAddr::V6(_) => vec![ipv6::IPPROTO_ICMPV6.into(), eth::ETHER_IPV6], + IpAddr::V4(_) => vec![ + u16::from(icmp::ICMP_ECHO) << 8, + ipv4::IPPROTO_ICMP.into(), + eth::ETHER_IPV4, + ], + IpAddr::V6(_) => vec![ + u16::from(icmp::ICMP6_ECHO_REQUEST) << 8, + ipv6::IPPROTO_ICMPV6.into(), + eth::ETHER_IPV6, + ], }; Packet::generate(src, dst, icmp_stack, Some(body)).unwrap() @@ -1441,8 +1459,55 @@ pub fn gen_arp_reply(src: Endpoint, tgt: Endpoint) -> Packet { .unwrap() } +pub fn tofino_flow_hash( + src: Endpoint, + dst: Endpoint, + ip_proto: L4Protocol, +) -> u16 { + let src = L4Endpoint::try_from(src).unwrap(); + let dst = L4Endpoint::try_from(dst).unwrap(); + + let mut data = [0u8; 2 * std::mem::size_of::<(Ipv6Addr, u16, u8)>()]; + let cap = data.len(); + let mut cursor = &mut data[..]; + let (src_port, dst_port, ip_proto_raw) = match (src, dst) { + ( + L4Endpoint { ip: IpAddr::V6(sip), port: src_port, .. }, + L4Endpoint { ip: IpAddr::V6(dip), port: dst_port, .. }, + ) => { + cursor.write_all(&dip.octets()).unwrap(); + cursor.write_all(&sip.octets()).unwrap(); + match ip_proto { + L4Protocol::Icmp => (0, dst_port, ipv6::IPPROTO_ICMPV6), + _ => (src_port, dst_port, ip_proto as u8), + } + } + ( + L4Endpoint { ip: IpAddr::V4(sip), port: src_port, .. }, + L4Endpoint { ip: IpAddr::V4(dip), port: dst_port, .. }, + ) => { + cursor.write_all(&dip.octets()).unwrap(); + cursor.write_all(&sip.octets()).unwrap(); + match ip_proto { + L4Protocol::Icmp => (0, dst_port, ipv4::IPPROTO_ICMP), + _ => (src_port, dst_port, ip_proto as u8), + } + } + _ => panic!("mismatched src/dst address families"), + }; + + cursor.write_all(&[ip_proto_raw]).unwrap(); + cursor.write_all(&dst_port.to_be_bytes()).unwrap(); + cursor.write_all(&src_port.to_be_bytes()).unwrap(); + + let written = cap - cursor.len(); + let crc = crc::Crc::::new(&crc::CRC_16_ARC); + crc.checksum(&data[..written]) +} + pub mod prelude { pub use super::ADMIN_LOCAL_MULTICAST_PREFIX; + pub use super::L4Protocol; pub use super::NO_PORT; pub use super::PhysPort; pub use super::SERVICE_PORT; diff --git a/dpd-client/tests/integration_tests/nat.rs b/dpd-client/tests/integration_tests/nat.rs index 9d7372a1..f8fba25b 100644 --- a/dpd-client/tests/integration_tests/nat.rs +++ b/dpd-client/tests/integration_tests/nat.rs @@ -161,12 +161,6 @@ async fn test_api() -> TestResult { Ok(()) } -enum L4Protocol { - Tcp, - Udp, - Icmp, -} - struct NatTest { // uplink network info uplink_port: PhysPort, @@ -341,34 +335,30 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { switch.client.route_ipv6_set(&route).await.unwrap(); common::add_neighbor_ipv6(switch, &test.gimlet_ip, gimlet_mac).await?; - let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; // Build a packet coming from an external host via an upstream router, to an // uplinked switch port. The packet is addressed to a nat ip/port pair. + let load = vec![0xaau8, 0xbb, 0xcc, 0xdd, 0xee]; let switch_mac = switch.get_port_mac(test.uplink_port).unwrap().to_string(); - let ingress_pkt = common::gen_udp_packet_loaded( - Endpoint::parse(&test.router_mac, &test.vpc_src_ip, test.vpc_src_port) - .unwrap(), - Endpoint::parse( - &switch_mac, - &test.uplink_port_external, - test.nat_l4_port, - ) - .unwrap(), - &load, - ); - - let icmp_load = &[]; - let ingress_icmp_pkt = common::gen_icmp_packet_loaded( + let src = Endpoint::parse(&test.router_mac, &test.vpc_src_ip, test.vpc_src_port) - .unwrap(), - Endpoint::parse( - &switch_mac, - &test.uplink_port_external, - test.nat_l4_port, - ) - .unwrap(), - icmp_load, - ); + .unwrap(); + let dst = Endpoint::parse( + &switch_mac, + &test.uplink_port_external, + test.nat_l4_port, + ) + .unwrap(); + let (ingress_pkt, csum_generated) = match test.l4_protocol { + L4Protocol::Tcp => { + (common::gen_tcp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Udp => { + (common::gen_udp_packet_loaded(src, dst, &load), true) + } + L4Protocol::Icmp => { + (common::gen_icmp_packet_loaded(src, dst, &load), false) + } + }; // Deparse the incoming packet so we can copy it into the encapsulated // packet @@ -379,13 +369,6 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { eth.eth_dmac = test.vpc_dst_mac.parse().unwrap(); encapped.deparse().unwrap().to_vec() }; - let ingress_icmp_payload = { - let mut encapped = ingress_icmp_pkt.clone(); - let eth = encapped.hdrs.eth_hdr.as_mut().unwrap(); - eth.eth_smac = MacAddr::new(0, 0, 0, 0, 0, 0); - eth.eth_dmac = test.vpc_dst_mac.parse().unwrap(); - encapped.deparse().unwrap().to_vec() - }; // build the encapsulated packet for transporting the NAT packet from the // switch to OPTE @@ -400,7 +383,7 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { Endpoint::parse( &gimlet_port_mac, switch_port_ip, - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash(src, dst, test.l4_protocol), ) .unwrap(), Endpoint::parse( @@ -413,50 +396,23 @@ async fn test_nat_ingress(switch: &Switch, test: &NatTest) -> TestResult { test.geneve_vni, &ingress_payload, ); - let mut forward_icmp_pkt = common::gen_external_geneve_packet( - Endpoint::parse( - &gimlet_port_mac, - switch_port_ip, - geneve::GENEVE_UDP_PORT, - ) - .unwrap(), - Endpoint::parse( - &test.gimlet_mac, - &test.gimlet_ip, - geneve::GENEVE_UDP_PORT, - ) - .unwrap(), - eth::ETHER_ETHER, - test.geneve_vni, - &ingress_icmp_payload, - ); /* Adjust for transition from switch port to gimlet port */ ipv6::Ipv6Hdr::adjust_hlim(&mut forward_pkt, -1); - ipv6::Ipv6Hdr::adjust_hlim(&mut forward_icmp_pkt, -1); - - udp::UdpHdr::update_checksum(&mut forward_pkt); - // TODO: I cannot convince the tofino to compute this correctly. - // Conveniently, we dont actually need it, see RFC 6935. - // - // udp::UdpHdr::update_checksum(&mut forward_icmp_pkt); - // - forward_icmp_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; - - let send = vec![ - TestPacket { packet: Arc::new(ingress_pkt), port: test.uplink_port }, - TestPacket { - packet: Arc::new(ingress_icmp_pkt), - port: test.uplink_port, - }, - ]; - let expected = vec![ - TestPacket { packet: Arc::new(forward_pkt), port: test.gimlet_port }, - TestPacket { - packet: Arc::new(forward_icmp_pkt), - port: test.gimlet_port, - }, - ]; + if csum_generated { + udp::UdpHdr::update_checksum(&mut forward_pkt); + } else { + forward_pkt.hdrs.udp_hdr.as_mut().unwrap().udp_sum = 0; + } + + let send = vec![TestPacket { + packet: Arc::new(ingress_pkt), + port: test.uplink_port, + }]; + let expected = vec![TestPacket { + packet: Arc::new(forward_pkt), + port: test.gimlet_port, + }]; switch.packet_test(send, expected) } @@ -588,6 +544,13 @@ async fn test_ingress_ipv4_tcp() -> TestResult { test_ingress_ipv4(switch, L4Protocol::Tcp).await } +#[tokio::test] +#[ignore] +async fn test_ingress_ipv4_icmp() -> TestResult { + let switch = &*get_switch().await; + test_ingress_ipv4(switch, L4Protocol::Icmp).await +} + // packet to/from IPv6 addresses, async fn test_egress_ipv6( switch: &Switch, @@ -702,3 +665,10 @@ async fn test_ingress_ipv6_tcp() -> TestResult { let switch = &*get_switch().await; test_ingress_ipv6(switch, L4Protocol::Tcp).await } + +#[tokio::test] +#[ignore] +async fn test_ingress_ipv6_icmp() -> TestResult { + let switch = &*get_switch().await; + test_ingress_ipv6(switch, L4Protocol::Icmp).await +} diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 37cd8fcc..75f7eceb 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -793,23 +793,18 @@ async fn test_multipath(switch: &Switch, routers: &[Router]) -> TestResult { let dst_port: u16 = 4444; // Replicate the path-selection algorithm used in the sidecar p4 code - let mut data = [0u8; 12]; - data[0..4].copy_from_slice(&dst_ip.parse::().unwrap().octets()); - data[4..8].copy_from_slice(&src_ip.parse::().unwrap().octets()); - data[8..10].copy_from_slice(&dst_port.to_be_bytes()); - data[10..12].copy_from_slice(&src_port.to_be_bytes()); + let src = Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(); + let dst = Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(); - // The tofino CRC8 implementation uses the default polynomial value of 0x07 - let mut crc8 = crc8::Crc8::create_msb(0x07); - let hash = crc8.calc(&data, 12, 0); - let expected_egress = (hash & 0x3f) as usize % routers.len(); + let hash = common::tofino_flow_hash(src, dst, L4Protocol::Udp); + let expected_egress = usize::from(hash & 0x3f) % routers.len(); let (to_send, mut to_recv) = common::gen_udp_routed_pair( switch, PhysPort(routers[expected_egress].port), routers[expected_egress].mac, - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(), + src, + dst, ); let send = diff --git a/dpd-client/tests/integration_tests/route_ipv6.rs b/dpd-client/tests/integration_tests/route_ipv6.rs index dbdf6917..81932cac 100644 --- a/dpd-client/tests/integration_tests/route_ipv6.rs +++ b/dpd-client/tests/integration_tests/route_ipv6.rs @@ -853,23 +853,18 @@ async fn test_multipath(switch: &Switch, routers: &[Router]) -> TestResult { let dst_port: u16 = 4444; // Replicate the path-selection algorithm used in the sidecar p4 code - let mut data = [0u8; 36]; - data[0..16].copy_from_slice(&dst_ip.parse::().unwrap().octets()); - data[16..32].copy_from_slice(&src_ip.parse::().unwrap().octets()); - data[32..34].copy_from_slice(&dst_port.to_be_bytes()); - data[34..36].copy_from_slice(&src_port.to_be_bytes()); + let src = Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(); + let dst = Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(); - // The tofino CRC8 implementation uses the default polynomial value of 0x07 - let mut crc8 = crc8::Crc8::create_msb(0x07); - let hash = crc8.calc(&data, 36, 0); - let expected_egress = (hash & 0x3f) as usize % routers.len(); + let hash = common::tofino_flow_hash(src, dst, L4Protocol::Udp); + let expected_egress = usize::from(hash & 0x3f) % routers.len(); let (to_send, mut to_recv) = common::gen_udp_routed_pair( switch, PhysPort(routers[expected_egress].port), routers[expected_egress].mac, - Endpoint::parse("e0:d5:5e:67:89:ab", src_ip, src_port).unwrap(), - Endpoint::parse("e0:d5:5e:67:89:ac", dst_ip, dst_port).unwrap(), + src, + dst, ); let send = diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index f764ad15..9cb6bf59 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -46,9 +46,9 @@ use crate::integration_tests::common::prelude::*; // 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 +const IPV4_LPM_SIZE: usize = 8188; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] -const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8190; // ipv4 forwarding table #[cfg(feature = "multicast")] const IPV6_LPM_SIZE: usize = 1023; // ipv6 forwarding table diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index c609a269..8fcbda5f 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -31,6 +31,7 @@ struct sidecar_ingress_meta_t { bit<8> drop_reason; // reason a packet was dropped bit<16> l4_src_port; // tcp or udp destination port bit<16> l4_dst_port; // tcp or udp destination port + bit<16> flow_hash; // hash of a packet 5-tuple for ECMP/encap ipv6_addr_t nat_ingress_tgt; // target address for NAT ingress mac_addr_t nat_inner_mac; // inner mac address for NAT ingress geneve_vni_t nat_geneve_vni; // VNI for NAT ingress diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index 3a2618d0..07af7074 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -41,6 +41,7 @@ parser IngressParser( meta.icmp_csum = 0; meta.l4_src_port = 0; meta.l4_dst_port = 0; + meta.flow_hash = 0; meta.l4_length = 0; meta.body_checksum = 0; meta.nexthop_ipv4 = 0; @@ -317,7 +318,7 @@ parser IngressParser( state goto_proto_ipv6 { transition select(hdr.ipv6.next_hdr) { - IPPROTO_ICMPV6: parse_icmp; + IPPROTO_ICMPV6: parse_icmp6; IPPROTO_TCP: parse_tcp; IPPROTO_UDP: parse_udp; IPPROTO_SCTP: drop_sctp; @@ -338,6 +339,39 @@ parser IngressParser( nat_checksum.subtract(hdr.icmp); nat_checksum.subtract_all_and_deposit(meta.body_checksum); + meta.l4_dst_port = hdr.icmp.data[31:16]; + + transition select(hdr.icmp.type) { + ICMP_ECHO: accept; + ICMP_ECHOREPLY: accept; + default: icmp_no_dst_info; + } + } + + state parse_icmp6 { + pkt.extract(hdr.icmp); + meta.pkt_type = meta.pkt_type | PKT_ICMP; + + icmp_checksum.subtract({ + hdr.icmp.hdr_checksum, + hdr.icmp.type, hdr.icmp.code + }); + icmp_checksum.subtract_all_and_deposit(meta.icmp_csum); + + nat_checksum.subtract(hdr.icmp); + nat_checksum.subtract_all_and_deposit(meta.body_checksum); + + meta.l4_dst_port = hdr.icmp.data[31:16]; + + transition select(hdr.icmp.type) { + ICMP6_ECHO: accept; + ICMP6_ECHOREPLY: accept; + default: icmp_no_dst_info; + } + } + + state icmp_no_dst_info { + meta.l4_dst_port = 0; transition accept; } diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 07a694ae..e7ac5a04 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -511,8 +511,12 @@ control NatIngress ( hdr.inner_eth.ether_type = hdr.ethernet.ether_type; // 8 bytes + // The Geneve specification suggests that the UDP source port + // SHOULD be calculated as a hash of the inner 5-tuple + // (RFC 8926, ยง3.3). This is fairly crucial for NICs to split + // encapsulated flows across hardware rings. hdr.udp.setValid(); - hdr.udp.src_port = GENEVE_UDP_PORT; + hdr.udp.src_port = meta.flow_hash; hdr.udp.dst_port = GENEVE_UDP_PORT; hdr.udp.hdr_length = udp_len; hdr.udp.checksum = 0; @@ -648,30 +652,6 @@ control NatIngress ( } #endif /* MULTICAST */ - action set_icmp_dst_port() { - meta.l4_dst_port = hdr.icmp.data[31:16]; - } - - table icmp_dst_port { - key = { - hdr.icmp.isValid(): ternary; - hdr.icmp.type: ternary; - } - - actions = { - set_icmp_dst_port; - } - - const entries = { - ( true, ICMP_ECHO ) : set_icmp_dst_port; - ( true, ICMP_ECHOREPLY ) : set_icmp_dst_port; - ( true, ICMP6_ECHO ) : set_icmp_dst_port; - ( true, ICMP6_ECHOREPLY ) : set_icmp_dst_port; - } - - const size = 4; - } - action set_inner_tcp() { hdr.inner_tcp = hdr.tcp; hdr.inner_tcp.setValid(); @@ -714,8 +694,6 @@ control NatIngress ( } apply { - icmp_dst_port.apply(); - // Note: This whole conditional could be simpler as a set of */ // `const entries`, but apply (on tables) cannot be called from actions #ifdef MULTICAST @@ -1233,7 +1211,6 @@ control Router4 ( inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { RouterLookupIndex4() lookup_idx; - Hash>(HashAlgorithm_t.CRC8) index_hash; action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; @@ -1259,14 +1236,9 @@ control Router4 ( fwd.slot = 0; // Our route selection table is 11 bits wide, and we need 5 bits // of that for our "slot count" index. Thus, we only need 6 - // bits of the 8-bit hash calculated here to complete the 11-bit - // index. - fwd.ecmp_hash = index_hash.get({ - hdr.ipv4.dst_addr, - hdr.ipv4.src_addr, - meta.l4_dst_port, - meta.l4_src_port - }) & 0x3f; + // bits of the 16-bit hash calculated earlier to complete the + // 11-bit index. + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1380,7 +1352,6 @@ control Router6 ( inout ingress_intrinsic_metadata_for_tm_t ig_tm_md ) { RouterLookupIndex6() lookup_idx; - Hash>(HashAlgorithm_t.CRC8) index_hash; action icmp_error(bit<8> type, bit<8> code) { hdr.sidecar.sc_code = SC_ICMP_NEEDED; @@ -1404,14 +1375,9 @@ control Router6 ( fwd.slot = 0; // Our route selection table is 11 bits wide, and we need 5 bits // of that for our "slot count" index. Thus, we only need 6 - // bits of the 8-bit hash calculated here to complete the 11-bit - // index. - fwd.ecmp_hash = index_hash.get({ - hdr.ipv6.dst_addr, - hdr.ipv6.src_addr, - meta.l4_dst_port, - meta.l4_src_port - }) & 0x3f; + // bits of the 16-bit hash calculated earlier to complete the + // 11-bit index. + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -2112,6 +2078,43 @@ control MulticastEgress ( } #endif /* MULTICAST */ +/* + * Compute the flowhash of any (non-filtered) packet on ingress. + * + * Hashes are used for two purposes in the datapath: + * - ECMP nexthop selection in the Router4/Router6 tables. + * - Providing flow entropy in generated Geneve headers, so that + * destination NICs and transit switches can correctly fanout + * flows over rings and ports. + */ +control FlowHash( + in sidecar_headers_t hdr, + inout sidecar_ingress_meta_t meta) +{ + Hash>(HashAlgorithm_t.CRC16) v4_hash; + Hash>(HashAlgorithm_t.CRC16) v6_hash; + + apply { + if (hdr.ipv4.isValid()) { + meta.flow_hash = v4_hash.get({ + hdr.ipv4.dst_addr, + hdr.ipv4.src_addr, + hdr.ipv4.protocol, + meta.l4_dst_port, + meta.l4_src_port + }); + } else if (hdr.ipv6.isValid()) { + meta.flow_hash = v6_hash.get({ + hdr.ipv6.dst_addr, + hdr.ipv6.src_addr, + hdr.ipv6.next_hdr, + meta.l4_dst_port, + meta.l4_src_port + }); + } + } +} + control Ingress( inout sidecar_headers_t hdr, inout sidecar_ingress_meta_t meta, @@ -2131,6 +2134,7 @@ control Ingress( MulticastIngress() mcast_ingress; #endif /* MULTICAST */ MacRewrite() mac_rewrite; + FlowHash() flow_hash; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) ingress_ctr; Counter, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) egress_ctr; @@ -2152,6 +2156,8 @@ control Ingress( filter.apply(hdr, meta, ig_intr_md); } + flow_hash.apply(hdr, meta); + if (!meta.is_mcast || meta.is_link_local_mcastv6) { attached_subnet_ingress.apply(hdr, meta); } diff --git a/packet/src/icmp.rs b/packet/src/icmp.rs index 1591a1ce..8da4a56e 100644 --- a/packet/src/icmp.rs +++ b/packet/src/icmp.rs @@ -60,6 +60,8 @@ pub const ICMP6_DST_UNREACH: u8 = 1; pub const ICMP6_PACKET_TOO_BIG: u8 = 2; pub const ICMP6_TIME_EXCEEDED: u8 = 3; pub const ICMP6_PARAM_PROB: u8 = 4; +pub const ICMP6_ECHO_REQUEST: u8 = 128; +pub const ICMP6_ECHO_REPLY: u8 = 129; pub const ICMP6_DST_UNREACH_NOROUTE: u8 = 0; pub const ICMP6_DST_UNREACH_ADMIN: u8 = 1; @@ -132,8 +134,13 @@ impl Protocol for IcmpHdr { let op = protos.pop().unwrap_or_default(); let icmp_type = ((op >> 8) & 0xff) as u8; let icmp_code = (op & 0xff) as u8; - let icmp_id = match crate::L4Endpoint::try_from(dst) { - Ok(ep) => (ep.port as u32) << 16, + let is_v6 = crate::L3Endpoint::try_from(dst).unwrap().ip.is_ipv6(); + let icmp_id = match (is_v6, icmp_type, crate::L4Endpoint::try_from(dst)) + { + (false, ICMP_ECHO, Ok(ep)) + | (false, ICMP_ECHOREPLY, Ok(ep)) + | (true, ICMP6_ECHO_REQUEST, Ok(ep)) + | (true, ICMP6_ECHO_REPLY, Ok(ep)) => u32::from(ep.port) << 16, _ => 0, }; From 36aa564ee626f19cfa9e5c3d8e21f9bf68b624cf Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 7 Oct 2026 17:02:04 +0100 Subject: [PATCH 2/8] Fixup multicast. --- dpd-client/tests/integration_tests/mcast.rs | 6 +- packet/src/lib.rs | 87 ++++++++++++++++++--- 2 files changed, 83 insertions(+), 10 deletions(-) diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index b98b8e76..6c5ac347 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -434,7 +434,11 @@ fn prepare_expected_pkt( Endpoint::parse( &switch_port_mac, "::0", - geneve::GENEVE_UDP_PORT, + common::tofino_flow_hash( + send_pkt.src().unwrap(), + send_pkt.dst().unwrap(), + L4Protocol::Udp, + ), ) .unwrap(), Endpoint::parse( diff --git a/packet/src/lib.rs b/packet/src/lib.rs index a0262c10..f2399d0d 100644 --- a/packet/src/lib.rs +++ b/packet/src/lib.rs @@ -5,8 +5,7 @@ // Copyright 2026 Oxide Computer Company use std::convert::TryFrom; -use std::fmt::Debug; -use std::fmt::{self, Write}; +use std::fmt::{self, Debug, Write}; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::string::ToString; @@ -90,7 +89,7 @@ pub fn parse_ip(ip: &str) -> PacketResult { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L2Endpoint { pub mac: MacAddr, } @@ -107,7 +106,7 @@ impl fmt::Display for L2Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L3Endpoint { pub mac: MacAddr, pub ip: IpAddr, @@ -129,7 +128,7 @@ impl fmt::Display for L3Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub struct L4Endpoint { pub mac: MacAddr, pub ip: IpAddr, @@ -152,7 +151,7 @@ impl L4Endpoint { } } -#[derive(Copy, Clone)] +#[derive(Copy, Clone, Debug)] pub enum Endpoint { L2(L2Endpoint), L3(L3Endpoint), @@ -292,9 +291,9 @@ impl TryFrom for L4Endpoint { impl fmt::Display for Endpoint { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Endpoint::L2(e) => e.fmt(f), - Endpoint::L3(e) => e.fmt(f), - Endpoint::L4(e) => e.fmt(f), + Endpoint::L2(e) => fmt::Display::fmt(e, f), + Endpoint::L3(e) => fmt::Display::fmt(e, f), + Endpoint::L4(e) => fmt::Display::fmt(e, f), } } } @@ -462,6 +461,76 @@ impl Packet { Ok(data) } + + pub fn src(&self) -> Option { + let mac = self.hdrs.eth_hdr?.eth_smac; + + let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { + (Some(v4), None) => (v4.ipv4_src_ip.into(), false), + (None, Some(v6)) => (v6.ipv6_src_ip.into(), true), + _ => return Some(L2Endpoint { mac }.into()), + }; + + let port = + match (&self.hdrs.udp_hdr, &self.hdrs.tcp_hdr, &self.hdrs.icmp_hdr) + { + (Some(udp), None, None) => udp.udp_sport, + (None, Some(tcp), None) => tcp.tcp_sport, + (None, None, Some(icmp)) + if (!v6 + && matches!( + icmp.icmp_type, + icmp::ICMP_ECHO | icmp::ICMP_ECHOREPLY + )) + || (v6 + && matches!( + icmp.icmp_type, + icmp::ICMP6_ECHO_REQUEST + | icmp::ICMP6_ECHO_REPLY + )) => + { + 0 + } + _ => return Some(L3Endpoint { mac, ip }.into()), + }; + + Some(L4Endpoint { mac, ip, port }.into()) + } + + pub fn dst(&self) -> Option { + let mac = self.hdrs.eth_hdr?.eth_dmac; + + let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { + (Some(v4), None) => (v4.ipv4_dst_ip.into(), false), + (None, Some(v6)) => (v6.ipv6_dst_ip.into(), true), + _ => return Some(L2Endpoint { mac }.into()), + }; + + let port = + match (&self.hdrs.udp_hdr, &self.hdrs.tcp_hdr, &self.hdrs.icmp_hdr) + { + (Some(udp), None, None) => udp.udp_dport, + (None, Some(tcp), None) => tcp.tcp_dport, + (None, None, Some(icmp)) + if (!v6 + && matches!( + icmp.icmp_type, + icmp::ICMP_ECHO | icmp::ICMP_ECHOREPLY + )) + || (v6 + && matches!( + icmp.icmp_type, + icmp::ICMP6_ECHO_REQUEST + | icmp::ICMP6_ECHO_REPLY + )) => + { + (icmp.icmp_data >> 16) as u16 + } + _ => return Some(L3Endpoint { mac, ip }.into()), + }; + + Some(L4Endpoint { mac, ip, port }.into()) + } } fn collect( From e39d4aab91d7ed7731fc75fa71818d3f82a21117 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 7 Oct 2026 17:27:28 +0100 Subject: [PATCH 3/8] ... --- dpd-client/tests/integration_tests/table_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index 9cb6bf59..6d1be9bd 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -46,7 +46,7 @@ use crate::integration_tests::common::prelude::*; // ingress TCAM pressure. Investigate moving MulticastRouter4/6 into the // egress pipeline to reclaim capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 8188; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8177; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] const IPV4_LPM_SIZE: usize = 8190; // ipv4 forwarding table From cc884fdc1a674c51cbfacaa9e5645496ce71b41e Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 7 Oct 2026 18:00:55 +0100 Subject: [PATCH 4/8] Reduce/track the stage limit, again --- .github/buildomat/common.sh | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/.github/buildomat/common.sh b/.github/buildomat/common.sh index f8df2081..965e5b78 100644 --- a/.github/buildomat/common.sh +++ b/.github/buildomat/common.sh @@ -1,14 +1,16 @@ #!/bin/bash -# The tofino2 has 20 stages, and the current sidecar.p4 needs all 20 of them. +# The tofino2 has 20 stages, and the current sidecar.p4 needs at most 19 of them, +# depending on the features it's compiled with. +# # Specifying the number of stages isn't strictly necessary, but it allows us to # track when we exceed the current ceiling. The underlying intention is to grow # deliberately and thoughtfully, given the limited space on the ASIC. -# -# Note: this now seems silly since we have maxed out the number of stages, but -# we want to leave this check and note in place should we ever find a way to -# reduce our footprint below 20 stages. -TOFINO_STAGES=20 +if [ "$MULTICAST" == "" ]; then + TOFINO_STAGES=17 +else + TOFINO_STAGES=19 +fi # These describe which version of the SDE to download and where to find it SDE_COMMIT=2a6b33211c9675996dcb99fe939045506667ae94 From fad72186e97e71d10851b167619ba70042dcecbb Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Wed, 7 Oct 2026 18:19:22 +0100 Subject: [PATCH 5/8] Wiggle the table sizes for CI. --- dpd-client/tests/integration_tests/table_tests.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index 6d1be9bd..15bc2f29 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -46,9 +46,9 @@ use crate::integration_tests::common::prelude::*; // ingress TCAM pressure. Investigate moving MulticastRouter4/6 into the // egress pipeline to reclaim capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 8177; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8151; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] -const IPV4_LPM_SIZE: usize = 8190; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table #[cfg(feature = "multicast")] const IPV6_LPM_SIZE: usize = 1023; // ipv6 forwarding table From a59506b7dbc1853cfcf5bdfe605f4dd0bcbd3a7a Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 8 Oct 2026 12:09:45 +0100 Subject: [PATCH 6/8] Review feedback. --- dpd-client/tests/integration_tests/common.rs | 75 +++++++++------- .../tests/integration_tests/table_tests.rs | 12 +-- dpd/p4/metadata.p4 | 4 +- dpd/p4/sidecar.p4 | 33 +++---- packet/src/icmp.rs | 9 ++ packet/src/lib.rs | 88 +++++++++++-------- 6 files changed, 123 insertions(+), 98 deletions(-) diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 4a25c1e8..0fcebdc8 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -5,7 +5,6 @@ // Copyright 2026 Oxide Computer Company use std::fmt::Write; -use std::io::Write as IoWrite; use std::net::{IpAddr, Ipv4Addr, Ipv6Addr}; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -24,6 +23,7 @@ use dpd_client::Client; use dpd_client::ClientInfo; use dpd_client::ClientState; use dpd_client::types; +use packet::Endpoint; use packet::Packet; use packet::arp; use packet::eth; @@ -31,7 +31,6 @@ use packet::icmp; use packet::ipv4; use packet::ipv6; use packet::sidecar; -use packet::{Endpoint, L4Endpoint}; use types::PortId; const SHOW_VERBOSE: u8 = 0x01; @@ -1459,50 +1458,58 @@ pub fn gen_arp_reply(src: Endpoint, tgt: Endpoint) -> Packet { .unwrap() } +/// Compute an identical flow hash to the `FlowHash` control in ingress +/// processing. +/// +/// This value is currently used in encap header generation and ECMP nexthop +/// selection. pub fn tofino_flow_hash( src: Endpoint, dst: Endpoint, ip_proto: L4Protocol, ) -> u16 { - let src = L4Endpoint::try_from(src).unwrap(); - let dst = L4Endpoint::try_from(dst).unwrap(); - - let mut data = [0u8; 2 * std::mem::size_of::<(Ipv6Addr, u16, u8)>()]; - let cap = data.len(); - let mut cursor = &mut data[..]; - let (src_port, dst_port, ip_proto_raw) = match (src, dst) { - ( - L4Endpoint { ip: IpAddr::V6(sip), port: src_port, .. }, - L4Endpoint { ip: IpAddr::V6(dip), port: dst_port, .. }, - ) => { - cursor.write_all(&dip.octets()).unwrap(); - cursor.write_all(&sip.octets()).unwrap(); - match ip_proto { - L4Protocol::Icmp => (0, dst_port, ipv6::IPPROTO_ICMPV6), - _ => (src_port, dst_port, ip_proto as u8), + let Ok(src_ip) = src.get_ip("src") else { + return 0; + }; + let Ok(dst_ip) = dst.get_ip("dst") else { + return 0; + }; + + let crc = crc::Crc::::new(&crc::CRC_16_ARC); + let mut digest = crc.digest(); + + let proto = match (dst_ip, src_ip) { + (IpAddr::V6(dip), IpAddr::V6(sip)) => { + digest.update(&dip.octets()); + digest.update(&sip.octets()); + if ip_proto == L4Protocol::Icmp { + ipv6::IPPROTO_ICMPV6 + } else { + ip_proto as u8 } } - ( - L4Endpoint { ip: IpAddr::V4(sip), port: src_port, .. }, - L4Endpoint { ip: IpAddr::V4(dip), port: dst_port, .. }, - ) => { - cursor.write_all(&dip.octets()).unwrap(); - cursor.write_all(&sip.octets()).unwrap(); - match ip_proto { - L4Protocol::Icmp => (0, dst_port, ipv4::IPPROTO_ICMP), - _ => (src_port, dst_port, ip_proto as u8), - } + (IpAddr::V4(dip), IpAddr::V4(sip)) => { + digest.update(&dip.octets()); + digest.update(&sip.octets()); + ip_proto as u8 } _ => panic!("mismatched src/dst address families"), }; - cursor.write_all(&[ip_proto_raw]).unwrap(); - cursor.write_all(&dst_port.to_be_bytes()).unwrap(); - cursor.write_all(&src_port.to_be_bytes()).unwrap(); + // Ingress processing will compute an L3 hash when no ports + // are available. + let dst_port = dst.get_port("dst").unwrap_or(0); + let src_port = if ip_proto == L4Protocol::Icmp { + // The parser leaves l4_src_port at 0 for ICMP echo. + 0 + } else { + src.get_port("src").unwrap_or(0) + }; - let written = cap - cursor.len(); - let crc = crc::Crc::::new(&crc::CRC_16_ARC); - crc.checksum(&data[..written]) + digest.update(&[proto]); + digest.update(&dst_port.to_be_bytes()); + digest.update(&src_port.to_be_bytes()); + digest.finalize() } pub mod prelude { diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index 15bc2f29..cb6371ca 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -42,18 +42,18 @@ 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. +// TODO: Multicast loses some IPv4 LPM capacity due to ingress TCAM pressure. +// Investigate moving MulticastRouter4/6 into the egress pipeline to reclaim +// capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 8151; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8186; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] -const IPV4_LPM_SIZE: usize = 8191; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8187; // 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 IPV6_LPM_SIZE: usize = 8190; // ipv6 forwarding table const SWITCH_IPV4_ADDRS_SIZE: usize = 511; // ipv4 addrs assigned to our ports const SWITCH_IPV6_ADDRS_SIZE: usize = 511; // ipv6 addrs assigned to our ports diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index 8fcbda5f..c2ed198d 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -92,7 +92,7 @@ struct route4_result_t { * A hash of the (address,port) fields, which is used to choose between * multiple potential routes. */ - bit<8> ecmp_hash; + bit<6> ecmp_hash; /* Index into the target table of the first potential route */ bit<16> idx; @@ -117,7 +117,7 @@ struct route6_result_t { * A hash of the (address,port) fields, which is used to choose between * multiple potential routes. */ - bit<8> ecmp_hash; + bit<6> ecmp_hash; /* Index into the target table of the first potential route */ bit<16> idx; diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index e7ac5a04..c82f657c 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -1238,7 +1238,7 @@ control Router4 ( // of that for our "slot count" index. Thus, we only need 6 // bits of the 16-bit hash calculated earlier to complete the // 11-bit index. - fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; + fwd.ecmp_hash = meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1377,7 +1377,7 @@ control Router6 ( // of that for our "slot count" index. Thus, we only need 6 // bits of the 16-bit hash calculated earlier to complete the // 11-bit index. - fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; + fwd.ecmp_hash = meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1653,9 +1653,6 @@ control MulticastIngress ( DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv4_ssm_ctr; DirectCounter>(CounterType_t.PACKETS_AND_BYTES) mcast_ipv6_ssm_ctr; - Hash>(HashAlgorithm_t.CRC16) mcast_hashv6_level1; - Hash>(HashAlgorithm_t.CRC16) mcast_hashv6_level2; - // Drop action for IPv4 multicast packets with no group. // // At this point, We should only allow replication for IPv6 packets that @@ -1726,20 +1723,11 @@ control MulticastIngress ( ig_tm_md.level1_exclusion_id = level1_excl_id; ig_tm_md.level2_exclusion_id = level2_excl_id; - // Set multicast hash based on IPv6 packet fields - ig_tm_md.level1_mcast_hash = (bit<13>)mcast_hashv6_level1.get({ - hdr.ipv6.src_addr, - hdr.ipv6.dst_addr, - hdr.ipv6.next_hdr, - meta.l4_src_port, - meta.l4_dst_port - }); - - // Set secondary multicast hash based on IPv6 packet fields - ig_tm_md.level2_mcast_hash = (bit<13>)mcast_hashv6_level2.get({ - hdr.ipv6.flow_label, - ig_intr_md.ingress_port - }); + // Set L3/ECMP and L2/LAG multicast hashes using the packet's + // flow hash. This will include entroy from the inner flow, see + // the commentary on FlowHash. + ig_tm_md.level1_mcast_hash = meta.flow_hash[12:0]; + ig_tm_md.level2_mcast_hash = meta.flow_hash[15:3]; mcast_ipv6_ctr.count(); } @@ -2086,6 +2074,13 @@ control MulticastEgress ( * - Providing flow entropy in generated Geneve headers, so that * destination NICs and transit switches can correctly fanout * flows over rings and ports. + * + * While this is run at the head of packet processing, this is always + * derived from the inner flow 5-tuple, such that we don't need to re-hash + * the packet after NatEgress processing. OPTE (and other switches in a + * multirack world) store the inner flow hash in the UDP source port of + * encap headers, and as a result the outer flow hash will include that + * entropy. */ control FlowHash( in sidecar_headers_t hdr, diff --git a/packet/src/icmp.rs b/packet/src/icmp.rs index 8da4a56e..bd81c1fc 100644 --- a/packet/src/icmp.rs +++ b/packet/src/icmp.rs @@ -260,6 +260,15 @@ impl IcmpHdr { let hdr = pkt.hdrs.icmp_hdr.as_mut().unwrap(); hdr.icmp_sum = checksum; } + + /// Return whether this is an ICMP(v6) echo request/reply packet. + pub fn is_echo(&self, is_v6: bool) -> bool { + matches!( + (is_v6, self.icmp_type), + (false, ICMP_ECHO | ICMP_ECHOREPLY) + | (true, ICMP6_ECHO_REQUEST | ICMP6_ECHO_REPLY) + ) + } } #[test] diff --git a/packet/src/lib.rs b/packet/src/lib.rs index f2399d0d..689b8e6a 100644 --- a/packet/src/lib.rs +++ b/packet/src/lib.rs @@ -462,13 +462,28 @@ impl Packet { Ok(data) } - pub fn src(&self) -> Option { - let mac = self.hdrs.eth_hdr?.eth_smac; + /// Collect all source addresses from this packet's protocol stack. + /// + /// This function will return an error if the packet has no Ethernet + /// header, or if multiple protocols are present at the same layer. + pub fn src(&self) -> PacketResult { + let mac = self + .hdrs + .eth_hdr + .ok_or_else(|| { + PacketError::Invalid("packet has no L2 header".into()) + })? + .eth_smac; let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { (Some(v4), None) => (v4.ipv4_src_ip.into(), false), (None, Some(v6)) => (v6.ipv6_src_ip.into(), true), - _ => return Some(L2Endpoint { mac }.into()), + (None, None) => return Ok(L2Endpoint { mac }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L3 headers".into(), + )); + } }; let port = @@ -476,34 +491,40 @@ impl Packet { { (Some(udp), None, None) => udp.udp_sport, (None, Some(tcp), None) => tcp.tcp_sport, - (None, None, Some(icmp)) - if (!v6 - && matches!( - icmp.icmp_type, - icmp::ICMP_ECHO | icmp::ICMP_ECHOREPLY - )) - || (v6 - && matches!( - icmp.icmp_type, - icmp::ICMP6_ECHO_REQUEST - | icmp::ICMP6_ECHO_REPLY - )) => - { - 0 + (None, None, Some(icmp)) if icmp.is_echo(v6) => 0, + (None, None, _) => return Ok(L3Endpoint { mac, ip }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L4 headers".into(), + )); } - _ => return Some(L3Endpoint { mac, ip }.into()), }; - Some(L4Endpoint { mac, ip, port }.into()) + Ok(L4Endpoint { mac, ip, port }.into()) } - pub fn dst(&self) -> Option { - let mac = self.hdrs.eth_hdr?.eth_dmac; + /// Collect all destination addresses from this packet's protocol stack. + /// + /// This function will return an error if the packet has no Ethernet + /// header, or if multiple protocols are present at the same layer. + pub fn dst(&self) -> PacketResult { + let mac = self + .hdrs + .eth_hdr + .ok_or_else(|| { + PacketError::Invalid("packet has no L2 header".into()) + })? + .eth_dmac; let (ip, v6) = match (&self.hdrs.ipv4_hdr, &self.hdrs.ipv6_hdr) { (Some(v4), None) => (v4.ipv4_dst_ip.into(), false), (None, Some(v6)) => (v6.ipv6_dst_ip.into(), true), - _ => return Some(L2Endpoint { mac }.into()), + (None, None) => return Ok(L2Endpoint { mac }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L3 headers".into(), + )); + } }; let port = @@ -511,25 +532,18 @@ impl Packet { { (Some(udp), None, None) => udp.udp_dport, (None, Some(tcp), None) => tcp.tcp_dport, - (None, None, Some(icmp)) - if (!v6 - && matches!( - icmp.icmp_type, - icmp::ICMP_ECHO | icmp::ICMP_ECHOREPLY - )) - || (v6 - && matches!( - icmp.icmp_type, - icmp::ICMP6_ECHO_REQUEST - | icmp::ICMP6_ECHO_REPLY - )) => - { + (None, None, Some(icmp)) if icmp.is_echo(v6) => { (icmp.icmp_data >> 16) as u16 } - _ => return Some(L3Endpoint { mac, ip }.into()), + (None, None, _) => return Ok(L3Endpoint { mac, ip }.into()), + _ => { + return Err(PacketError::Invalid( + "packet has multiple L4 headers".into(), + )); + } }; - Some(L4Endpoint { mac, ip, port }.into()) + Ok(L4Endpoint { mac, ip, port }.into()) } } From 33a8d8832ca79e511e48c165471003bfb0f6b165 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 8 Oct 2026 12:18:07 +0100 Subject: [PATCH 7/8] Spelling... --- dpd/p4/sidecar.p4 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index c82f657c..29cce1b8 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -1724,7 +1724,7 @@ control MulticastIngress ( ig_tm_md.level2_exclusion_id = level2_excl_id; // Set L3/ECMP and L2/LAG multicast hashes using the packet's - // flow hash. This will include entroy from the inner flow, see + // flow hash. This will include entropy from the inner flow, see // the commentary on FlowHash. ig_tm_md.level1_mcast_hash = meta.flow_hash[12:0]; ig_tm_md.level2_mcast_hash = meta.flow_hash[15:3]; From f7a95486142658214ae6308518b585591abac8f2 Mon Sep 17 00:00:00 2001 From: Kyle Simpson Date: Thu, 8 Oct 2026 13:00:13 +0100 Subject: [PATCH 8/8] Undo `ecmp_hash` bitwidth changes --- dpd-client/tests/integration_tests/table_tests.rs | 6 +++--- dpd/p4/metadata.p4 | 4 ++-- dpd/p4/sidecar.p4 | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dpd-client/tests/integration_tests/table_tests.rs b/dpd-client/tests/integration_tests/table_tests.rs index cb6371ca..f0d4c4b8 100644 --- a/dpd-client/tests/integration_tests/table_tests.rs +++ b/dpd-client/tests/integration_tests/table_tests.rs @@ -46,14 +46,14 @@ use crate::integration_tests::common::prelude::*; // Investigate moving MulticastRouter4/6 into the egress pipeline to reclaim // capacity. #[cfg(feature = "multicast")] -const IPV4_LPM_SIZE: usize = 8186; // ipv4 forwarding table +const IPV4_LPM_SIZE: usize = 8145; // ipv4 forwarding table #[cfg(not(feature = "multicast"))] -const IPV4_LPM_SIZE: usize = 8187; // ipv4 forwarding table +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 = 8190; // ipv6 forwarding table +const IPV6_LPM_SIZE: usize = 8191; // ipv6 forwarding table const SWITCH_IPV4_ADDRS_SIZE: usize = 511; // ipv4 addrs assigned to our ports const SWITCH_IPV6_ADDRS_SIZE: usize = 511; // ipv6 addrs assigned to our ports diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index c2ed198d..8fcbda5f 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -92,7 +92,7 @@ struct route4_result_t { * A hash of the (address,port) fields, which is used to choose between * multiple potential routes. */ - bit<6> ecmp_hash; + bit<8> ecmp_hash; /* Index into the target table of the first potential route */ bit<16> idx; @@ -117,7 +117,7 @@ struct route6_result_t { * A hash of the (address,port) fields, which is used to choose between * multiple potential routes. */ - bit<6> ecmp_hash; + bit<8> ecmp_hash; /* Index into the target table of the first potential route */ bit<16> idx; diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 29cce1b8..b06a842a 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -1238,7 +1238,7 @@ control Router4 ( // of that for our "slot count" index. Thus, we only need 6 // bits of the 16-bit hash calculated earlier to complete the // 11-bit index. - fwd.ecmp_hash = meta.flow_hash[5:0]; + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd); @@ -1377,7 +1377,7 @@ control Router6 ( // of that for our "slot count" index. Thus, we only need 6 // bits of the 16-bit hash calculated earlier to complete the // 11-bit index. - fwd.ecmp_hash = meta.flow_hash[5:0]; + fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; lookup_idx.apply(hdr, fwd);