From d203dc774ba5377c9f07d131727bdf491489a0da Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sat, 4 Jul 2026 00:40:08 +0000 Subject: [PATCH 1/4] [p4, sidecar] fix underlay/external mcast counter ops A replica still carrying a Geneve header at the egress counter block is bound for an underlay port, while external replicas are decapsulated earlier, at ingress, for external-only groups or in `mcast_egress` for bifurcated (external + underlay members) ones. Previously, we used a conditional: ```p4 else if (hdr.geneve.isValid()) { ``` ... on Geneve hdr validity too early in the chain to determine an external- vs underlay-destined packet, and we didn't consider using the `is_egress_rid_mcast` check at all. ## The Fix Now, we treat the three conditions for link-local, external mcast, and underlay mcast independently and more explicitly given the egress rid, which is 0 for link-local multicast packets and combines with Geneve hdr validity to determine the count for underlay vs external. The underlay and external arms key on `is_egress_rid_mcast` and not the negation of `is_link_local_ipv6_mcast`: the compiler doesn't carry the negation into the later gateways of the pipeline, which meant that a link-local packet incremented `external_mcast_ctr` alongside its own counter. This is now handled because link-local replicas arrive with `egress_rid` of 0, and scope 2 is refused at group creation, so no replicated packet ever carries a link-local outer destination. --- dpd-client/tests/integration_tests/mcast.rs | 304 +++++++++++++++++++- dpd/p4/sidecar.p4 | 35 ++- 2 files changed, 332 insertions(+), 7 deletions(-) diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index b98b8e76..c7997e94 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -17,7 +17,7 @@ use anyhow::anyhow; use dpd_client::{Error, types}; use futures::TryStreamExt; use oxnet::MulticastMac; -use packet::{Endpoint, eth, geneve, ipv4, ipv6, udp}; +use packet::{Endpoint, eth, geneve, ipv4, ipv6, sidecar, udp}; const MULTICAST_TEST_IPV4: Ipv4Addr = Ipv4Addr::new(224, 0, 1, 0); const MULTICAST_TEST_IPV6: Ipv6Addr = @@ -2147,9 +2147,27 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() ]; let port_label_ingress = switch.port_label(ingress).unwrap(); + let port_label_egress1 = switch.port_label(egress1).unwrap(); + let port_label_egress2 = switch.port_label(egress2).unwrap(); let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); + let ctr_baseline_external_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_underlay_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_underlay_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_underlay")) + .await + .unwrap(); switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); @@ -2163,6 +2181,44 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_external_members() .await .unwrap(); + // Decapped replicas to external members must be attributed to the + // external counter, not the underlay one. + check_counter_incremented( + switch, + &port_label_egress1, + ctr_baseline_external_egress1, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress2, + ctr_baseline_external_egress2, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + + let ctr_underlay_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_underlay_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_underlay")) + .await + .unwrap(); + assert_eq!( + ctr_underlay_egress1, ctr_baseline_underlay_egress1, + "decapped replicas must not hit the underlay counter" + ); + assert_eq!( + ctr_underlay_egress2, ctr_baseline_underlay_egress2, + "decapped replicas must not hit the underlay counter" + ); + cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG) .await .unwrap(); @@ -2277,9 +2333,27 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_members() ]; let port_label_ingress = switch.port_label(ingress).unwrap(); + let port_label_egress3 = switch.port_label(egress3).unwrap(); + let port_label_egress4 = switch.port_label(egress4).unwrap(); let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); + let ctr_baseline_underlay_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_underlay_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_external_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_external")) + .await + .unwrap(); switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); @@ -2293,6 +2367,43 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_members() .await .unwrap(); + // Encapped replicas to underlay members increment the underlay counter. + check_counter_incremented( + switch, + &port_label_egress3, + ctr_baseline_underlay_egress3, + 1, + Some("multicast_underlay"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress4, + ctr_baseline_underlay_egress4, + 1, + Some("multicast_underlay"), + ) + .await + .unwrap(); + + let ctr_external_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_external")) + .await + .unwrap(); + let ctr_external_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_external")) + .await + .unwrap(); + assert_eq!( + ctr_external_egress3, ctr_baseline_external_egress3, + "underlay-tagged replicas must not hit the external counter" + ); + assert_eq!( + ctr_external_egress4, ctr_baseline_external_egress4, + "underlay-tagged replicas must not hit the external counter" + ); + cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG) .await .unwrap(); @@ -2420,9 +2531,45 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe ]; let port_label_ingress = switch.port_label(ingress).unwrap(); + let port_label_egress1 = switch.port_label(egress1).unwrap(); + let port_label_egress2 = switch.port_label(egress2).unwrap(); + let port_label_egress3 = switch.port_label(egress3).unwrap(); + let port_label_egress4 = switch.port_label(egress4).unwrap(); let ctr_baseline_ingress = switch.get_counter(&port_label_ingress, Some("ingress")).await.unwrap(); + let ctr_baseline_underlay_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_underlay_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_underlay_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_underlay_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_baseline_external_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_external_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_external")) + .await + .unwrap(); switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); @@ -2436,12 +2583,167 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe .await .unwrap(); + // Still-encapsulated replicas to underlay members count as underlay, + // not external, for UNDERLAY_EXTERNAL tagged groups. + check_counter_incremented( + switch, + &port_label_egress3, + ctr_baseline_underlay_egress3, + 1, + Some("multicast_underlay"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress4, + ctr_baseline_underlay_egress4, + 1, + Some("multicast_underlay"), + ) + .await + .unwrap(); + + // Decapped replicas to external members count as external. + check_counter_incremented( + switch, + &port_label_egress1, + ctr_baseline_external_egress1, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + check_counter_incremented( + switch, + &port_label_egress2, + ctr_baseline_external_egress2, + 1, + Some("multicast_external"), + ) + .await + .unwrap(); + + let ctr_external_egress3 = switch + .get_counter(&port_label_egress3, Some("multicast_external")) + .await + .unwrap(); + let ctr_external_egress4 = switch + .get_counter(&port_label_egress4, Some("multicast_external")) + .await + .unwrap(); + assert_eq!( + ctr_external_egress3, ctr_baseline_external_egress3, + "underlay replicas must not hit the external counter" + ); + assert_eq!( + ctr_external_egress4, ctr_baseline_external_egress4, + "underlay replicas must not hit the external counter" + ); + + let ctr_underlay_egress1 = switch + .get_counter(&port_label_egress1, Some("multicast_underlay")) + .await + .unwrap(); + let ctr_underlay_egress2 = switch + .get_counter(&port_label_egress2, Some("multicast_underlay")) + .await + .unwrap(); + assert_eq!( + ctr_underlay_egress1, ctr_baseline_underlay_egress1, + "decapped replicas must not hit the underlay counter" + ); + assert_eq!( + ctr_underlay_egress2, ctr_baseline_underlay_egress2, + "decapped replicas must not hit the underlay counter" + ); + cleanup_test_group(switch, get_group_ip(&created_group), TEST_TAG) .await .unwrap(); cleanup_test_group(switch, MULTICAST_NAT_IP.into(), TEST_TAG).await } +/// Link-local replicas reach egress with a zero replication id, so the +/// counter block reaches them through `is_link_local_ipv6_mcast` rather +/// than `egress_rid`. Both the underlay and external conditions exclude +/// that case, and this checks the exclusion remains after compilation. +#[tokio::test] +#[ignore] +async fn test_link_local_multicast_counter_attribution() -> TestResult { + let switch = &*get_switch().await; + + let egress = PhysPort(10); + + let src = + Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:7788:0101::4", 3333) + .unwrap(); + let dst = Endpoint::parse("33:33:00:00:00:01", "ff02::1", 4444).unwrap(); + + // Sourced from userspace so the packet egresses without replication, + // mirroring test_link_local_multicast_outbound in route_ipv6. + let mut send = common::gen_udp_packet(src, dst); + common::add_sidecar_hdr( + switch, + &mut send, + sidecar::SC_FWD_FROM_USERSPACE, + NO_PORT, + egress, + None, + ); + let test_pkt = TestPacket { packet: Arc::new(send), port: SERVICE_PORT }; + let expected_pkts = vec![TestPacket { + packet: Arc::new(common::gen_udp_packet(src, dst)), + port: egress, + }]; + + let port_label_egress = switch.port_label(egress).unwrap(); + + let ctr_baseline_link_local = switch + .get_counter(&port_label_egress, Some("multicast_link_local")) + .await + .unwrap(); + let ctr_baseline_external = switch + .get_counter(&port_label_egress, Some("multicast_external")) + .await + .unwrap(); + let ctr_baseline_underlay = switch + .get_counter(&port_label_egress, Some("multicast_underlay")) + .await + .unwrap(); + + switch.packet_test(vec![test_pkt], expected_pkts).unwrap(); + + check_counter_incremented( + switch, + &port_label_egress, + ctr_baseline_link_local, + 1, + Some("multicast_link_local"), + ) + .await + .unwrap(); + + let ctr_external = switch + .get_counter(&port_label_egress, Some("multicast_external")) + .await + .unwrap(); + let ctr_underlay = switch + .get_counter(&port_label_egress, Some("multicast_underlay")) + .await + .unwrap(); + assert_eq!( + ctr_external, ctr_baseline_external, + "link-local replicas must not hit the external counter" + ); + assert_eq!( + ctr_underlay, ctr_baseline_underlay, + "link-local replicas must not hit the underlay counter" + ); + + Ok(()) +} + #[tokio::test] #[ignore] async fn test_ipv4_multicast_drops_ingress_is_egress_port() -> TestResult { diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 37a394cd..f8817d13 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -1823,7 +1823,7 @@ control MulticastIngress ( } else if (hdr.geneve.isValid() && hdr.inner_ipv6.isValid()) { // Check if the inner destination address is an IPv6 multicast // address (ff00::/8). Apply source filtering for both SSM - // (ff3x::/16) and ASM ranges. + // (ff3x::/32) and ASM ranges. if (hdr.inner_ipv6.dst_addr[127:120] == 8w0xff) { mcast_source_filter_ipv6.apply(); } else { @@ -2346,15 +2346,38 @@ control Egress( } else if (is_mcast == true) { mcast_ctr.count(eg_intr_md.egress_port); + // Count each multicast replica by type, one simple gateway + // per condition: + // + // - link-local (ff02::/16 outer dst) — never replicated by + // the PRE; it arrives with egress_rid == 0. + // - egress_rid > 0, Geneve hdr still valid — bound for an + // underlay port. + // - egress_rid > 0, no Geneve hdr — decapped at ingress + // (external-only groups) or by the mcast_egress control + // above (bifurcated, external + underlay members), and + // bound for an external member. + // + // The mcast_tag option is not read here: a replica headed + // for the underlay still carries its encapsulation whether + // or not the group is bifurcated. + // + // The rid arms don't test !is_link_local_ipv6_mcast: the + // compiler doesn't carry the negation across gateways. + // They don't need to either, since scope 2 is refused at + // group creation, so a replica never carries a link-local + // outer destination. + // + // The IngressDeparser above notes the same compiler limitation. 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) { + } + if (is_egress_rid_mcast && hdr.geneve.isValid()) { underlay_mcast_ctr.count(eg_intr_md.egress_port); } + if (is_egress_rid_mcast && !hdr.geneve.isValid()) { + external_mcast_ctr.count(eg_intr_md.egress_port); + } } else { unicast_ctr.count(eg_intr_md.egress_port); } From b866ac99a582ef8cae0f893d5670267e7a835a8e Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Fri, 11 Sep 2026 11:31:16 +0000 Subject: [PATCH 2/4] [review] doc fix on rid cases --- dpd/p4/sidecar.p4 | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 4212aeff..66b8bb6e 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -2346,29 +2346,33 @@ control Egress( } else if (is_mcast == true) { mcast_ctr.count(eg_intr_md.egress_port); - // Count each multicast replica by type, one simple gateway - // per condition: + // Per-type replica counters. There are three disjoint + // cases presented here, with one simple gateway each. + // The compiler cannot carry a negated condition across + // gateways, so each case gets its own gateway instead + // of an else branch (the IngressDeparser notes the same + // limitation): // - // - link-local (ff02::/16 outer dst) — never replicated by - // the PRE; it arrives with egress_rid == 0. - // - egress_rid > 0, Geneve hdr still valid — bound for an - // underlay port. - // - egress_rid > 0, no Geneve hdr — decapped at ingress - // (external-only groups) or by the mcast_egress control - // above (bifurcated, external + underlay members), and - // bound for an external member. + // - link-local (ff02::/16 outer dst): always forwarded, + // never PRE (Packet Replication Engine)-replicated; + // it arrives with egress_rid == 0. + // - egress_rid > 0 + a valid Geneve hdr: an underlay + // replica that's still encapsulated. + // - egress_rid > 0 + no Geneve hdr: an external replica + // that's decapped at ingress (external-only groups) + // or by mcast_egress above (bifurcated groups). // - // The mcast_tag option is not read here: a replica headed - // for the underlay still carries its encapsulation whether - // or not the group is bifurcated. + // These rid branch arms avoid checking + // !is_link_local_ipv6_mcast because scope 2 (link-local + // scope) groups are rejected at creation time and no + // PRE-replica ever carries a link-local outer + // destination. // - // The rid arms don't test !is_link_local_ipv6_mcast: the - // compiler doesn't carry the negation across gateways. - // They don't need to either, since scope 2 is refused at - // group creation, so a replica never carries a link-local - // outer destination. - // - // The IngressDeparser above notes the same compiler limitation. + // The mcast_tag option is not doing any work here: + // underlay replicas keep their encapsulation whether or + // not the group is bifurcated; Geneve header validity + // separates the two rid cases. + if (is_link_local_ipv6_mcast) { link_local_mcast_ctr.count(eg_intr_md.egress_port); } From ea470fae35d7439027c5c1d9dc97e2840ca21cc5 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Wed, 7 Oct 2026 00:03:44 +0000 Subject: [PATCH 3/4] [review] egress counters: rid-first check + fix link-local cond > IngressDeparser has the issue that writing it more cleanly uses too > many checksum engines, because the compiler doesn't recognise the > disjoint cases for some reason. Do we end up running out of stages if > we do a sensible if...? This reworks our counter logic per Kyle's comment (quoted above). Nesting the per-type checks (and, separately, an else-if chain with link-local checked first) dropped the underlay count to 0 during testing, even though stage usage didn't change. However, this led me to find an issue with: ```p4 bit<16> ipv6_prefix = (bit<16>)hdr.ipv6.dst_addr[127:112]; is_link_local_ipv6_mcast = (ipv6_prefix == IPV6_LINK_LOCAL_16); ``` This bool lands within a 16-bit container, and the conditionals on it (e.g., `is_mcast` and link_local checks) are built with *-only match rows, making them always true. So, every forwarded packet was counted as `Multicast`, and `Unicast` remained at 0. Now, the bool is set inside the `if` and the chain branches on `egress_rid` first. We don't see any significant issues with the added cycles, however. --- dpd-client/tests/integration_tests/common.rs | 37 ++++++++++++ dpd-client/tests/integration_tests/mcast.rs | 31 ---------- .../tests/integration_tests/route_ipv4.rs | 23 +++++++- dpd/p4/sidecar.p4 | 57 +++++++++---------- 4 files changed, 85 insertions(+), 63 deletions(-) diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index 39790a29..d52eb08e 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -45,6 +45,13 @@ pub const ADMIN_LOCAL_MULTICAST_PREFIX: u16 = 0xFF04; // packets before notifying us that any are ready. const TEST_PCAP_TIMEOUT_MS: i32 = 1; +// Interval and attempt count for polling a counter until it reaches an +// expected value. +// +// Note: counter updates from the model can lag the packet itself. +const COUNTER_POLL_INTERVAL: Duration = Duration::from_millis(100); +const COUNTER_POLL_ATTEMPTS: usize = 20; + // Physical port number #[derive(Clone, Copy, PartialOrd, Ord, Hash, PartialEq, Eq)] pub struct PhysPort(pub u16); @@ -827,6 +834,35 @@ impl Switch { } } +pub async fn check_counter_incremented( + switch: &Switch, + counter_name: &str, + baseline: u64, + expected_increment: u64, + client_name: Option<&str>, +) -> anyhow::Result { + let expected = baseline + expected_increment; + let mut new_value = 0; + + // Poll for the counter value (with timeout) + for _i in 0..COUNTER_POLL_ATTEMPTS { + thread::sleep(COUNTER_POLL_INTERVAL); + new_value = + switch.get_counter(counter_name, client_name).await.unwrap(); + + if new_value == expected { + return Ok(new_value); + } + } + + // Counter didn't increment as expected + Err(anyhow!( + "Counter '{counter_name}' expected to increase by \ + {expected_increment} (from {baseline} to {expected}), but only \ + reached {new_value}" + )) +} + // Construct a single TCP packet with an optional payload pub fn gen_tcp_packet_loaded( src: Endpoint, @@ -1444,5 +1480,6 @@ pub mod prelude { pub use super::Switch; pub use super::TestPacket; pub use super::TestResult; + pub use super::check_counter_incremented; pub use super::get_switch; } diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index c7997e94..1d8a003b 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -71,37 +71,6 @@ fn has_vlan_action_for_ip( }) } -async fn check_counter_incremented( - switch: &Switch, - counter_name: &str, - baseline: u64, - expected_increment: u64, - client_name: Option<&str>, -) -> anyhow::Result { - let mut new_value = 0; - - // Poll for the counter value (with timeout) - for _i in 0..20 { - std::thread::sleep(std::time::Duration::from_millis(100)); - new_value = - switch.get_counter(counter_name, client_name).await.unwrap(); - - if new_value == baseline + expected_increment { - return Ok(new_value); - } - } - - // Counter didn't increment as expected - Err(anyhow!( - "Counter '{}' expected to increase by {} (from {} to {}), but only reached {}", - counter_name, - expected_increment, - baseline, - baseline + expected_increment, - new_value - )) -} - async fn create_test_multicast_group( switch: &Switch, group_ip: IpAddr, diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 37cd8fcc..39f38ace 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -133,7 +133,28 @@ async fn test_route_ipv4_unicast_impl(switch: &Switch) -> TestResult { #[ignore] async fn test_route_ipv4_unicast() -> TestResult { let switch = &*get_switch().await; - test_route_ipv4_unicast_impl(switch).await + let label = switch.port_label(PhysPort(14)).unwrap(); + + let unicast = switch.get_counter(&label, Some("unicast")).await?; + let mcast = switch.get_counter(&label, Some("multicast")).await?; + let link_local = + switch.get_counter(&label, Some("multicast_link_local")).await?; + + test_route_ipv4_unicast_impl(switch).await?; + + check_counter_incremented(switch, &label, unicast, 1, Some("unicast")) + .await?; + check_counter_incremented(switch, &label, mcast, 0, Some("multicast")) + .await?; + check_counter_incremented( + switch, + &label, + link_local, + 0, + Some("multicast_link_local"), + ) + .await?; + Ok(()) } async fn test_deleted_route_ipv4_impl(switch: &Switch) -> TestResult { diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 57fcd2c7..9cd96b8d 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -2325,9 +2325,9 @@ control Egress( bool is_egress_rid_mcast = eg_intr_md.egress_rid > 0; // We track IPv6 multicast packets separately for counters. 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 == IPV6_LINK_LOCAL_16); + if (hdr.ipv6.isValid() && + hdr.ipv6.dst_addr[127:112] == IPV6_LINK_LOCAL_16) { + is_link_local_ipv6_mcast = true; } bool is_mcast = is_egress_rid_mcast || is_link_local_ipv6_mcast; @@ -2359,41 +2359,36 @@ control Egress( if (is_mcast == true) { mcast_ctr.count(eg_intr_md.egress_port); - // Per-type replica counters. There are three disjoint - // cases presented here, with one simple gateway each. - // The compiler cannot carry a negated condition across - // gateways, so each case gets its own gateway instead - // of an else branch (the IngressDeparser notes the same - // limitation): + // Per-type counters: // - // - link-local (ff02::/16 outer dst): always forwarded, - // never PRE (Packet Replication Engine)-replicated; - // it arrives with egress_rid == 0. - // - egress_rid > 0 + a valid Geneve hdr: an underlay - // replica that's still encapsulated. - // - egress_rid > 0 + no Geneve hdr: an external replica - // that's decapped at ingress (external-only groups) - // or by mcast_egress above (bifurcated groups). + // - link-local (ff02::/16 outer dst) -> always + // forwarded without PRE (Packet Replication Engine) + // replication, arriving with egress_rid == 0. + // - egress_rid > 0 with a valid Geneve header -> an + // underlay replica, still encapsulated. + // - egress_rid > 0 without a valid Geneve header -> an + // external replica, decapped at ingress (external-only + // groups) or by mcast_egress above (bifurcated groups). // - // These rid branch arms avoid checking - // !is_link_local_ipv6_mcast because scope 2 (link-local - // scope) groups are rejected at creation time and no - // PRE-replica ever carries a link-local outer - // destination. + // Geneve validity separates the two replica cases. + // Underlay replicas stay encapsulated whether or not the + // group is bifurcated, meaning that we can skip an + // mcast_tag check. // - // The mcast_tag option is not doing any work here: - // underlay replicas keep their encapsulation whether or - // not the group is bifurcated; Geneve header validity - // separates the two rid cases. + // This chain branches on egress_rid first. Within + // the scope of is_mcast, a zero rid already implies + // link-local, making the last branch avoid a test of + // is_link_local_ipv6_mcast. Scope 2 groups are rejected + // at the time of creation, meaning that no PRE replica + // would carry a link-local outer destination at this + // point. - if (is_link_local_ipv6_mcast) { - link_local_mcast_ctr.count(eg_intr_md.egress_port); - } if (is_egress_rid_mcast && hdr.geneve.isValid()) { underlay_mcast_ctr.count(eg_intr_md.egress_port); - } - if (is_egress_rid_mcast && !hdr.geneve.isValid()) { + } else if (is_egress_rid_mcast) { external_mcast_ctr.count(eg_intr_md.egress_port); + } else { + link_local_mcast_ctr.count(eg_intr_md.egress_port); } } else { unicast_ctr.count(eg_intr_md.egress_port); From 88a19dc889a6b92837fe072c43c39f3ded79f151 Mon Sep 17 00:00:00 2001 From: Zeeshan Lakhani Date: Sat, 17 Oct 2026 01:24:12 +0000 Subject: [PATCH 4/4] [review] address feedback + count link-local via bridge hdr --- common/src/counters.rs | 4 ++ dpd-client/tests/integration_tests/common.rs | 60 +++++++++---------- dpd-client/tests/integration_tests/mcast.rs | 57 ++++++++++++++++++ .../tests/integration_tests/route_ipv4.rs | 27 ++++----- dpd/p4/metadata.p4 | 3 +- dpd/p4/parser.p4 | 1 + dpd/p4/sidecar.p4 | 9 ++- dpd/src/counters.rs | 26 ++++++-- 8 files changed, 132 insertions(+), 55 deletions(-) diff --git a/common/src/counters.rs b/common/src/counters.rs index 48bcb3e4..6c132f1f 100644 --- a/common/src/counters.rs +++ b/common/src/counters.rs @@ -13,6 +13,10 @@ pub use dpd_types::counters::{ FecRSCounters, PcsCounters, RMonCounters, RMonCountersAll, }; +/// Per-port counter label for the userspace service port, which has no link +/// of its own on the current switch. +pub const SERVICE_PORT_LABEL: &str = "service"; + /// sidecar.p4 defines the following set of indirect counters. #[derive( Debug, diff --git a/dpd-client/tests/integration_tests/common.rs b/dpd-client/tests/integration_tests/common.rs index d52eb08e..fa2dc9fd 100644 --- a/dpd-client/tests/integration_tests/common.rs +++ b/dpd-client/tests/integration_tests/common.rs @@ -14,6 +14,7 @@ use std::{fmt, thread}; use anyhow::Context; use anyhow::anyhow; +use anyhow::bail; use oxnet::Ipv4Net; use oxnet::Ipv6Net; use slog::Drain; @@ -789,16 +790,10 @@ impl Switch { } } - match errors.len() { - 0 => Ok(()), - _ => Err(anyhow!( - errors - .iter() - .map(|e| e.to_string()) - .collect::>() - .join(", ") - )), + if !errors.is_empty() { + bail!("{}", errors.join(", ")); } + Ok(()) } /// If no client_name is given, this returns the number of packets dropped @@ -814,23 +809,20 @@ impl Switch { counter: &str, client_name: Option<&str>, ) -> anyhow::Result { - let client_name = if let Some(client_name) = client_name { - client_name.to_string() - } else { - "ingress_drop_reason".to_string() - }; + let client_name = client_name.unwrap_or("ingress_drop_reason"); - self.client - .counter_get(&client_name, true) + let entries = self + .client + .counter_get(client_name, true) .await - .map_err(|e| anyhow::anyhow!("failed to fetch counters: {e:?}")) - .and_then(|entries| { - entries - .iter() - .find(|e| e.keys.get("label").unwrap().as_str() == counter) - .map(|e| e.data.pkts.unwrap()) - .ok_or(anyhow::anyhow!("no such counter: {counter}")) - }) + .map_err(|e| anyhow!("failed to fetch counters: {e:?}"))?; + let Some(entry) = entries + .iter() + .find(|e| e.keys.get("label").unwrap().as_str() == counter) + else { + bail!("no such counter: {counter} in {client_name}"); + }; + Ok(entry.data.pkts.unwrap()) } } @@ -842,25 +834,31 @@ pub async fn check_counter_incremented( client_name: Option<&str>, ) -> anyhow::Result { let expected = baseline + expected_increment; + let table = client_name.unwrap_or("ingress_drop_reason"); + let incr_expect = format!( + "counter '{counter_name}' in {table} expected to increase by \ + {expected_increment} ({baseline} -> {expected})" + ); let mut new_value = 0; // Poll for the counter value (with timeout) for _i in 0..COUNTER_POLL_ATTEMPTS { thread::sleep(COUNTER_POLL_INTERVAL); - new_value = - switch.get_counter(counter_name, client_name).await.unwrap(); + new_value = switch.get_counter(counter_name, client_name).await?; if new_value == expected { return Ok(new_value); } + if new_value > expected { + bail!("{incr_expect}, but overshot to {new_value}"); + } } // Counter didn't increment as expected - Err(anyhow!( - "Counter '{counter_name}' expected to increase by \ - {expected_increment} (from {baseline} to {expected}), but only \ - reached {new_value}" - )) + bail!( + "{incr_expect}, but reached {new_value} after \ + {COUNTER_POLL_ATTEMPTS} polls" + ) } // Construct a single TCP packet with an optional payload diff --git a/dpd-client/tests/integration_tests/mcast.rs b/dpd-client/tests/integration_tests/mcast.rs index 1d8a003b..508eab4e 100644 --- a/dpd-client/tests/integration_tests/mcast.rs +++ b/dpd-client/tests/integration_tests/mcast.rs @@ -12,6 +12,7 @@ use std::{ use crate::integration_tests::common; use crate::integration_tests::common::prelude::*; +use ::common::counters::SERVICE_PORT_LABEL; use ::common::network::MacAddr; use anyhow::anyhow; use dpd_client::{Error, types}; @@ -2713,6 +2714,62 @@ async fn test_link_local_multicast_counter_attribution() -> TestResult { Ok(()) } +#[tokio::test] +#[ignore] +async fn test_link_local_multicast_inbound_counter_attribution() -> TestResult { + let switch = &*get_switch().await; + + let ingress = PhysPort(10); + + let src = + Endpoint::parse("e0:d5:5e:67:89:ab", "fd00:1122:7788:0101::4", 3333) + .unwrap(); + let dst = Endpoint::parse("33:33:00:00:00:01", "ff02::1", 4444).unwrap(); + + let send = TestPacket { + packet: Arc::new(common::gen_udp_packet(src, dst)), + port: ingress, + }; + let mut recv = common::gen_udp_packet(src, dst); + common::add_sidecar_hdr( + switch, + &mut recv, + sidecar::SC_FWD_TO_USERSPACE, + ingress, + NO_PORT, + None, + ); + let expected_pkts = + vec![TestPacket { packet: Arc::new(recv), port: SERVICE_PORT }]; + + let ctr_baseline_link_local = switch + .get_counter(SERVICE_PORT_LABEL, Some("multicast_link_local")) + .await?; + let ctr_baseline_unicast = + switch.get_counter(SERVICE_PORT_LABEL, Some("unicast")).await?; + + switch.packet_test(vec![send], expected_pkts)?; + + check_counter_incremented( + switch, + SERVICE_PORT_LABEL, + ctr_baseline_link_local, + 1, + Some("multicast_link_local"), + ) + .await?; + check_counter_incremented( + switch, + SERVICE_PORT_LABEL, + ctr_baseline_unicast, + 0, + Some("unicast"), + ) + .await?; + + Ok(()) +} + #[tokio::test] #[ignore] async fn test_ipv4_multicast_drops_ingress_is_egress_port() -> TestResult { diff --git a/dpd-client/tests/integration_tests/route_ipv4.rs b/dpd-client/tests/integration_tests/route_ipv4.rs index 39f38ace..c1362d61 100644 --- a/dpd-client/tests/integration_tests/route_ipv4.rs +++ b/dpd-client/tests/integration_tests/route_ipv4.rs @@ -9,6 +9,7 @@ use std::net::Ipv4Addr; use std::sync::Arc; use anyhow::anyhow; +use anyhow::bail; use oxnet::Ipv4Net; use reqwest::StatusCode; @@ -89,9 +90,7 @@ async fn validate_routes( ) -> TestResult { if expected.is_empty() { match client.route_ipv4_get(&cidr).await { - Ok(f) => { - Err(anyhow!("found {} targets - expected no route", f.len())) - } + Ok(f) => bail!("found {} targets - expected no route", f.len()), Err(_) => Ok(()), } } else { @@ -604,7 +603,8 @@ async fn fill_until_ipv4_filler_alloc_unavailable( Err(e) if e.status() == Some(StatusCode::INSUFFICIENT_STORAGE) => { assert!( i > 0, - "alloc({SHRINK_TEST_FILLER_TARGETS}) failed before any IPv4 fillers" + "alloc({SHRINK_TEST_FILLER_TARGETS}) failed before any \ + IPv4 fillers" ); validate_routes( client, @@ -614,20 +614,19 @@ async fn fill_until_ipv4_filler_alloc_unavailable( .await?; return Ok(fillers); } - Err(e) => { - return Err(anyhow!( - "unexpected error probing IPv4 \ - alloc({SHRINK_TEST_FILLER_TARGETS}) availability at \ - filler prefix {cidr}: {e:?}" - )); - } + Err(e) => bail!( + "unexpected error probing IPv4 \ + alloc({SHRINK_TEST_FILLER_TARGETS}) availability at \ + filler prefix {cidr}: {e:?}" + ), } } - Err(anyhow!( - "IPv4 route target allocation of size {SHRINK_TEST_FILLER_TARGETS} did not fail after \ + bail!( + "IPv4 route target allocation of size \ + {SHRINK_TEST_FILLER_TARGETS} did not fail after \ {MAX_ROUTE_TARGET_FILLERS} filler prefixes" - )) + ) } #[tokio::test] diff --git a/dpd/p4/metadata.p4 b/dpd/p4/metadata.p4 index 0521a93e..4471eb22 100644 --- a/dpd/p4/metadata.p4 +++ b/dpd/p4/metadata.p4 @@ -11,7 +11,8 @@ header bridge_h { PortId_t ingress_port; bool nat_egress_hit; - bit<6> reserved; + bool is_link_local_mcastv6; + bit<5> reserved; } struct sidecar_ingress_meta_t { diff --git a/dpd/p4/parser.p4 b/dpd/p4/parser.p4 index f90668b6..ee012768 100644 --- a/dpd/p4/parser.p4 +++ b/dpd/p4/parser.p4 @@ -56,6 +56,7 @@ 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.is_link_local_mcastv6 = false; meta.bridge_hdr.reserved = 0; transition port_metadata; diff --git a/dpd/p4/sidecar.p4 b/dpd/p4/sidecar.p4 index 9cd96b8d..c25d7c61 100644 --- a/dpd/p4/sidecar.p4 +++ b/dpd/p4/sidecar.p4 @@ -2139,6 +2139,8 @@ control Ingress( if (meta.nat_egress_hit && !meta.service_routed) { meta.bridge_hdr.nat_egress_hit = true; } + } else if (meta.is_link_local_mcastv6) { + meta.bridge_hdr.is_link_local_mcastv6 = true; } if (meta.encap_needed) { @@ -2324,11 +2326,8 @@ control Egress( // Check multicast egress packets by checking that RID is not 0. bool is_egress_rid_mcast = eg_intr_md.egress_rid > 0; // We track IPv6 multicast packets separately for counters. - bool is_link_local_ipv6_mcast = false; - if (hdr.ipv6.isValid() && - hdr.ipv6.dst_addr[127:112] == IPV6_LINK_LOCAL_16) { - is_link_local_ipv6_mcast = true; - } + bool is_link_local_ipv6_mcast = + meta.bridge_hdr.is_link_local_mcastv6; bool is_mcast = is_egress_rid_mcast || is_link_local_ipv6_mcast; if (is_egress_rid_mcast == true) { diff --git a/dpd/src/counters.rs b/dpd/src/counters.rs index b189cb6f..fc25c0da 100644 --- a/dpd/src/counters.rs +++ b/dpd/src/counters.rs @@ -24,11 +24,26 @@ use aal_macros::*; use asic::Handle; use common::counters::CounterId; use common::counters::EgressCounterId; +use common::counters::SERVICE_PORT_LABEL; use common::table::TableType; use anyhow::Context; use dpd_types::table::TableCounterEntry; +/// The ASIC port carrying packets to and from userspace, matching +/// `USER_SPACE_SERVICE_PORT` in `sidecar.p4`. +/// +/// On the current switch, this is the PCIe CPU port, which has no link. The +/// `int0` link maps to the Ethernet CPU port, as reported by +/// `bf_eth_cpu_port_get` (ASIC port 2 on the Tofino model). +/// Without a label for ASIC port 0, its per-port counters get omitted from +/// the response. +/// +/// Note: the SDE's PCIe CPU port selection can be found within +/// `lld_get_pcie_cpu_port`: +/// +const USER_SPACE_SERVICE_PORT: u16 = 0; + // Counters in an indirect table are accessed by their index number rather than // a key. Still, we define a key anyway to allow us to use the direct counter // infrastructure to extract the counter data. @@ -274,10 +289,13 @@ fn reason_label(ctr: u8) -> Result, String> { // dozen are associated with configured links at any given time. If this ID // doesn't match to a port, it's not an error. async fn port_label(switch: &Switch, ctr: u16) -> Option { - switch - .asic_port_id_to_port_link(ctr) - .map(|(port, link)| format!("{port}/{link}")) - .ok() + match switch.asic_port_id_to_port_link(ctr) { + Ok((port, link)) => Some(format!("{port}/{link}")), + Err(_) if ctr == USER_SPACE_SERVICE_PORT => { + Some(SERVICE_PORT_LABEL.to_string()) + } + Err(_) => None, + } } /// Fetch all of the accumulated values for this counter.