Skip to content

Commit a75a02e

Browse files
[sidecar/pipeline] P4 cleanup and a return to 18 stages
P4/sidecar pipeline changes: - Unify route_result_t struct for Router4/Router6 to prevent PHV liverange divergence - this came up in checking logs for stage + bit overlap - Unify nexthop: replace separate nexthop_ipv4/nexthop_ipv6 with single nexthop (ipv6_addr_t) plus nexthop_is_v6 flag - Add TTL compound key (idx, route_ttl_is_1) to route tables for TTL offload - This includes Rust updates and a larger routing table for the compound key - Add @pa_no_init pragmas for metadata fields to guard against compiler init bugs, even though we've updated/removed the compiler pass in p4c - Bridge header now includes is_mcast_routed for CPU copy detection Egress MacRewrite: - Create separate unicast_mac_rewrite and mcast_mac_rewrite instances of MacRewrite - Unicast instance: rewrites src_mac only (dst_mac already correct from ARP/NDP) - Multicast instance: derives dst_mac from group address per RFC 1112/2464 Multicast Egress changes: - Check egress_rid \!= 0 first to identify PRE-replicated packets - Check is_mcast_routed for CPU copies (egress_rid == 0 but routed to multicast) - Drop CPU copies with DROP_MULTICAST_CPU_COPY reason Counters: - Moved Unicast, MulticastLL, EgressDropPort, EgressDropReason counters from multicast to base - Added a Forwarded counter (for every packet copy that egresses the pipeline) - Removed Egress counter (from ingress pipe, replaced by per-port counters) - Removed MulticastDrop from MULTICAST_COUNTERS, as it's covered in general drop w/ reason - Handle link-local multicast counting in both MULTICAST and non-MULTICAST paths softnpu: - Route table updates for IPv4/IPv6 TTL handling compatibility - Skip route_ttl_is_1 entries and ttl_exceeded actions (sidecar-lite TODO) Build: - Configure TOFINO_STAGES: 15 for base, 18 for multicast
1 parent 7a66c1b commit a75a02e

27 files changed

Lines changed: 786 additions & 638 deletions

.github/buildomat/common.sh

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
#!/bin/bash
22

3-
# The tofino2 has 20 stages, and the current sidecar.p4 needs all 20 of them.
4-
# Specifying the number of stages isn't strictly necessary, but it allows us to
5-
# track when we exceed the current ceiling. The underlying intention is to grow
6-
# deliberately and thoughtfully, given the limited space on the ASIC.
7-
#
8-
# Note: this now seems silly since we have maxed out the number of stages, but
9-
# we want to leave this check and note in place should we ever find a way to
10-
# reduce our footprint below 20 stages.
11-
TOFINO_STAGES=20
3+
# The tofino2 has 20 stages. Base sidecar.p4 needs 15 stages, and with
4+
# multicast support it needs 18. Specifying the number of stages isn't strictly
5+
# necessary, but it allows us to track when we exceed the current ceiling.
6+
# The underlying intention is to grow deliberately and thoughtfully, given the
7+
# limited space on the ASIC.
8+
if [[ "${BUILD_FEATURES:-}" == *"multicast"* ]]; then
9+
TOFINO_STAGES=18
10+
else
11+
TOFINO_STAGES=15
12+
fi
1213

1314
# These describe which version of the SDE to download and where to find it
1415
SDE_COMMIT=e61fe02c3c1c384b2e212c90177fcea76a31fd4e

.github/buildomat/packet-test-common.sh

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,23 @@ export RUST_BACKTRACE=1
33
source .github/buildomat/common.sh
44
source .github/buildomat/linux.sh
55

6-
wd=`pwd`
6+
wd=$(pwd)
77
export WS=$wd
88
MODEL_STARTUP_TIMEOUT=${MODEL_STARTUP_TIMEOUT:=5}
99
STARTUP_TIMEOUT=${STARTUP_TIMEOUT:=120}
1010

11-
if [ x$MULTICAST == x ]; then
12-
BUILD_FEATURES=tofino_asic
13-
CODEGEN_FEATURES=
14-
SWADM_FEATURES=
15-
else
16-
BUILD_FEATURES=tofino_asic,multicast
17-
CODEGEN_FEATURES=--multicast
18-
SWADM_FEATURES=--features=multicast
11+
if [[ -z "$MULTICAST" ]]; then
12+
BUILD_FEATURES=tofino_asic
13+
CODEGEN_FEATURES=
14+
SWADM_FEATURES=
15+
TOFINO_STAGES=15
16+
else
17+
BUILD_FEATURES=tofino_asic,multicast
18+
CODEGEN_FEATURES=--multicast
19+
SWADM_FEATURES=--features=multicast
20+
TOFINO_STAGES=18
1921
fi
20-
22+
2123
function cleanup {
2224
set +o errexit
2325
set +o pipefail
@@ -66,29 +68,33 @@ fi
6668

6769
banner "Test"
6870
sudo -E ./tools/veth_setup.sh
69-
id=`id -un`
70-
gr=`id -gn`
71+
id=$(id -un)
72+
gr=$(id -gn)
7173
sudo -E mkdir -p /work
7274
sudo -E chown $id:$gr /work
73-
sudo -E ./tools/run_tofino_model.sh &> /work/simulator.log &
75+
sudo -E ./tools/run_tofino_model.sh &>/work/simulator.log </dev/null &
76+
stty sane 2>/dev/null || true
7477
sleep $MODEL_STARTUP_TIMEOUT
75-
sudo -E ./tools/run_dpd.sh -m 127.0.0.1 &> /work/dpd.log &
78+
sudo -E ./tools/run_dpd.sh -m 127.0.0.1 &>/work/dpd.log </dev/null &
79+
stty sane 2>/dev/null || true
7680
echo "waiting for dpd to come online"
7781
set +o errexit
7882

7983
SLEEP_TIME=5
80-
iters=$(( $STARTUP_TIMEOUT / $SLEEP_TIME ))
81-
while [ 1 ] ; do
82-
./target/debug/swadm --host '[::1]' build-info 2> /dev/null
83-
if [ $? == 0 ]; then
84-
break
85-
fi
86-
iters=$(($iters - 1))
87-
if [ $iters = 0 ]; then
88-
echo "dpd failed to come online in $STARTUP_TIMEOUT seconds"
89-
exit 1
90-
fi
91-
sleep $SLEEP_TIME
84+
iters=$(($STARTUP_TIMEOUT / $SLEEP_TIME))
85+
while [ 1 ]; do
86+
./target/debug/swadm --host '[::1]' build-info 2>/dev/null
87+
rc=$?
88+
stty sane 2>/dev/null || true
89+
if [ $rc == 0 ]; then
90+
break
91+
fi
92+
iters=$(($iters - 1))
93+
if [ $iters = 0 ]; then
94+
echo "dpd failed to come online in $STARTUP_TIMEOUT seconds"
95+
exit 1
96+
fi
97+
sleep $SLEEP_TIME
9298
done
9399
set -o errexit
94100

asic/src/chaos/table.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ pub const ROUTE_IPV4: &str = "pipe.Ingress.l3_router.routes_ipv4";
2020
pub const ROUTE_IPV6: &str = "pipe.Ingress.l3_router.routes_ipv6";
2121
pub const ARP_IPV4: &str = "pipe.Ingress.l3_router.arp_ipv4";
2222
pub const NEIGHBOR_IPV6: &str = "pipe.Ingress.l3_router.neighbor_ipv6";
23-
pub const MAC_REWRITE: &str = "pipe.Ingress.mac_rewrite.mac_rewrite";
23+
pub const MAC_REWRITE: &str = "pipe.Egress.unicast_mac_rewrite.mac_rewrite";
2424
pub const SWITCH_IPV4_ADDR: &str = "pipe.Ingress.filter.switch_ipv4_addr";
2525
pub const SWITCH_IPV6_ADDR: &str = "pipe.Ingress.filter.switch_ipv6_addr";
2626
pub const NAT_INGRESS_IPV4: &str = "pipe.Ingress.nat_ingress.ingress_ipv4";
@@ -42,7 +42,7 @@ pub(crate) const MCAST_ROUTE_IPV4: &str =
4242
pub(crate) const MCAST_ROUTE_IPV6: &str =
4343
"pipe.Ingress.l3_router.MulticastRouter6.tbl";
4444
pub(crate) const MCAST_MAC_REWRITE: &str =
45-
"pipe.Egress.mac_rewrite.mac_rewrite";
45+
"pipe.Egress.mcast_mac_rewrite.mac_rewrite";
4646
pub(crate) const MCAST_DECAP_PORTS: &str =
4747
"pipe.Egress.mcast_egress.tbl_decap_ports";
4848
pub(crate) const MCAST_PORT_ID_MAPPING: &str =

asic/src/softnpu/table.rs

Lines changed: 101 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,12 @@ pub struct Table {
2323
}
2424

2525
// soft-npu table names
26+
// Route tables are idx-only in sidecar-lite; route_ttl_is_1 is ignored here.
27+
// TODO: remove compat once sidecar-lite updates route keys/actions:
28+
// - p4/sidecar-lite.p4: add route_ttl_is_1 to route table keys and add
29+
// ttl_exceeded actions; set ingress.route_ttl_is_1 in router.
30+
// - p4/softnpu.p4: add route_ttl_is_1 to ingress metadata.
31+
// - scadm + softnpu tests: encode/decode idx + ttl in route keys.
2632
const ROUTER_V4_RT: &str = "ingress.router.v4_route.rtr";
2733
const ROUTER_V4_IDX: &str = "ingress.router.v4_idx.rtr";
2834
const ROUTER_V6_RT: &str = "ingress.router.v6_route.rtr";
@@ -44,16 +50,19 @@ const _PROXY_ARP: &str = "ingress.pxarp.proxy_arp";
4450
const SWITCH_ADDR4: &str = "pipe.Ingress.filter.switch_ipv4_addr";
4551
const SWITCH_ADDR6: &str = "pipe.Ingress.filter.switch_ipv6_addr";
4652
const ROUTER4_LOOKUP_RT: &str =
47-
"pipe.Ingress.l3_router.Router4.lookup_idx.route";
53+
"pipe.Ingress.l3_router.router4.lookup_idx.route";
4854
const ROUTER4_LOOKUP_IDX: &str =
49-
"pipe.Ingress.l3_router.Router4.lookup_idx.lookup";
55+
"pipe.Ingress.l3_router.router4.lookup_idx.lookup";
5056
const ROUTER6_LOOKUP_RT: &str =
51-
"pipe.Ingress.l3_router.Router6.lookup_idx.route";
57+
"pipe.Ingress.l3_router.router6.lookup_idx.route";
5258
const ROUTER6_LOOKUP_IDX: &str =
53-
"pipe.Ingress.l3_router.Router6.lookup_idx.lookup";
59+
"pipe.Ingress.l3_router.router6.lookup_idx.lookup";
5460
const NDP: &str = "pipe.Ingress.l3_router.Ndp.tbl";
5561
const ARP: &str = "pipe.Ingress.l3_router.Arp.tbl";
56-
const DPD_MAC_REWRITE: &str = "pipe.Ingress.mac_rewrite.mac_rewrite";
62+
const DPD_UNICAST_MAC_REWRITE: &str =
63+
"pipe.Egress.unicast_mac_rewrite.mac_rewrite";
64+
#[cfg(feature = "multicast")]
65+
const DPD_MCAST_MAC_REWRITE: &str = "pipe.Egress.mcast_mac_rewrite.mac_rewrite";
5766
const NAT_INGRESS4: &str = "pipe.Ingress.nat_ingress.ingress_ipv4";
5867
const NAT_INGRESS6: &str = "pipe.Ingress.nat_ingress.ingress_ipv6";
5968
const ATTACHED_SUBNET_INGRESS4: &str =
@@ -85,8 +94,12 @@ impl TableOps<Handle> for Table {
8594
SWITCH_ADDR6 => (Some(LOCAL_V6.into()), Some(SWITCH_ADDR6.into())),
8695
NDP => (Some(RESOLVER_V6.into()), Some(NDP.into())),
8796
ARP => (Some(RESOLVER_V4.into()), Some(ARP.into())),
88-
DPD_MAC_REWRITE => {
89-
(Some(MAC_REWRITE.into()), Some(DPD_MAC_REWRITE.into()))
97+
DPD_UNICAST_MAC_REWRITE => {
98+
(Some(MAC_REWRITE.into()), Some(DPD_UNICAST_MAC_REWRITE.into()))
99+
}
100+
#[cfg(feature = "multicast")]
101+
DPD_MCAST_MAC_REWRITE => {
102+
(Some(MAC_REWRITE.into()), Some(DPD_MCAST_MAC_REWRITE.into()))
90103
}
91104
NAT_INGRESS4 => (Some(NAT_V4.into()), Some(NAT_INGRESS4.into())),
92105
NAT_INGRESS6 => (Some(NAT_V6.into()), Some(NAT_INGRESS6.into())),
@@ -135,9 +148,22 @@ impl TableOps<Handle> for Table {
135148
let action_data = data.action_to_ir().unwrap();
136149

137150
trace!(hdl.log, "entry_add called");
138-
trace!(hdl.log, "table: {}", table);
139-
trace!(hdl.log, "match_data:\n{:#?}", match_data);
140-
trace!(hdl.log, "action_data:\n{:#?}", action_data);
151+
trace!(hdl.log, "table: {table}");
152+
trace!(hdl.log, "match_data:\n{match_data:#?}");
153+
trace!(hdl.log, "action_data:\n{action_data:#?}");
154+
155+
let is_route_table =
156+
matches!(dpd_table.as_str(), ROUTER4_LOOKUP_RT | ROUTER6_LOOKUP_RT);
157+
if is_route_table {
158+
if route_ttl_is_1(&match_data.fields) {
159+
trace!(hdl.log, "skipping ttl==1 route entry for {dpd_table}");
160+
return Ok(());
161+
}
162+
if action_data.action == "ttl_exceeded" {
163+
trace!(hdl.log, "skipping ttl_exceeded action for {dpd_table}");
164+
return Ok(());
165+
}
166+
}
141167

142168
let keyset_data = keyset_data(match_data.fields, &table);
143169

@@ -440,7 +466,23 @@ impl TableOps<Handle> for Table {
440466
}
441467
("rewrite_dst", params)
442468
}
443-
(DPD_MAC_REWRITE, "rewrite") => {
469+
(DPD_UNICAST_MAC_REWRITE, "rewrite") => {
470+
let mut params = Vec::new();
471+
for arg in action_data.args {
472+
match arg.value {
473+
ValueTypes::U64(v) => {
474+
let mac = v.to_le_bytes();
475+
params.extend_from_slice(&mac[0..6]);
476+
}
477+
ValueTypes::Ptr(v) => {
478+
params.extend_from_slice(v.as_slice());
479+
}
480+
}
481+
}
482+
("rewrite", params)
483+
}
484+
#[cfg(feature = "multicast")]
485+
(DPD_MCAST_MAC_REWRITE, "rewrite") => {
444486
let mut params = Vec::new();
445487
for arg in action_data.args {
446488
match arg.value {
@@ -545,10 +587,10 @@ impl TableOps<Handle> for Table {
545587
};
546588
let action = action.to_string();
547589
trace!(hdl.log, "sending request to softnpu");
548-
trace!(hdl.log, "table: {}", table);
549-
trace!(hdl.log, "action: {:#?}", action);
550-
trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data);
551-
trace!(hdl.log, "parameter_data:\n{:#?}", parameter_data);
590+
trace!(hdl.log, "table: {table}");
591+
trace!(hdl.log, "action: {action:#?}");
592+
trace!(hdl.log, "keyset_data:\n{keyset_data:#?}");
593+
trace!(hdl.log, "parameter_data:\n{parameter_data:#?}");
552594

553595
let msg = ManagementRequest::TableAdd(TableAdd {
554596
table,
@@ -576,9 +618,9 @@ impl TableOps<Handle> for Table {
576618
let action_data = data.action_to_ir().unwrap();
577619

578620
trace!(hdl.log, "entry_update called");
579-
trace!(hdl.log, "table: {}", table);
580-
trace!(hdl.log, "match_data:\n{:#?}", match_data);
581-
trace!(hdl.log, "action_data:\n{:#?}", action_data);
621+
trace!(hdl.log, "table: {table}");
622+
trace!(hdl.log, "match_data:\n{match_data:#?}");
623+
trace!(hdl.log, "action_data:\n{action_data:#?}");
582624

583625
//TODO implement in softnpu
584626
Ok(())
@@ -593,17 +635,31 @@ impl TableOps<Handle> for Table {
593635
None => return Ok(()),
594636
Some(id) => id.clone(),
595637
};
638+
let dpd_table = match &self.dpd_id {
639+
None => return Ok(()),
640+
Some(id) => id.clone(),
641+
};
596642
let match_data = key.key_to_ir().unwrap();
597643

598644
trace!(hdl.log, "entry_del called");
599-
trace!(hdl.log, "table: {}", table);
600-
trace!(hdl.log, "match_data:\n{:#?}", match_data);
645+
trace!(hdl.log, "table: {table}");
646+
trace!(hdl.log, "match_data:\n{match_data:#?}");
647+
648+
let is_route_table =
649+
matches!(dpd_table.as_str(), ROUTER4_LOOKUP_RT | ROUTER6_LOOKUP_RT);
650+
if is_route_table && route_ttl_is_1(&match_data.fields) {
651+
trace!(
652+
hdl.log,
653+
"skipping ttl==1 route entry delete for {dpd_table}"
654+
);
655+
return Ok(());
656+
}
601657

602658
let keyset_data = keyset_data(match_data.fields, &table);
603659

604660
trace!(hdl.log, "sending request to softnpu");
605-
trace!(hdl.log, "table: {}", table);
606-
trace!(hdl.log, "keyset_data:\n{:#?}", keyset_data);
661+
trace!(hdl.log, "table: {table}");
662+
trace!(hdl.log, "keyset_data:\n{keyset_data:#?}");
607663

608664
let msg =
609665
ManagementRequest::TableRemove(TableRemove { keyset_data, table });
@@ -639,12 +695,12 @@ fn keyset_data(match_data: Vec<MatchEntryField>, table: &str) -> Vec<u8> {
639695
let mut data: Vec<u8> = Vec::new();
640696
match table {
641697
RESOLVER_V4 => {
642-
// "nexthop_ipv4" => bit<32>
698+
// "nexthop" => bit<32>
643699
serialize_value_type(&x, &mut data);
644700
keyset_data.extend_from_slice(&data[..4]);
645701
}
646702
RESOLVER_V6 => {
647-
// "nexthop_ipv4" => bit<128>
703+
// "nexthop" => bit<128>
648704
let mut buf = Vec::new();
649705
serialize_value_type(&x, &mut buf);
650706
buf.reverse();
@@ -654,10 +710,12 @@ fn keyset_data(match_data: Vec<MatchEntryField>, table: &str) -> Vec<u8> {
654710
serialize_value_type(&x, &mut data);
655711
keyset_data.extend_from_slice(&data[..2]);
656712
}
657-
ROUTER_V4_RT => {
658-
// "idx" => exact => bit<16>
659-
serialize_value_type(&x, &mut data);
660-
keyset_data.extend_from_slice(&data[..2]);
713+
ROUTER_V4_RT | ROUTER_V6_RT => {
714+
// sidecar-lite route keys are idx-only.
715+
if m.name == "idx" {
716+
serialize_value_type(&x, &mut data);
717+
keyset_data.extend_from_slice(&data[..2]);
718+
}
661719
}
662720
NAT_V4 => {
663721
// "dst_addr" => hdr.ipv4.dst: exact => bit<32>
@@ -747,3 +805,18 @@ fn serialize_value_type_be(x: &ValueTypes, data: &mut Vec<u8>) {
747805
}
748806
}
749807
}
808+
809+
fn route_ttl_is_1(fields: &[MatchEntryField]) -> bool {
810+
fields.iter().any(|field| {
811+
if field.name != "route_ttl_is_1" {
812+
return false;
813+
}
814+
match &field.value {
815+
MatchEntryValue::Value(ValueTypes::U64(v)) => *v != 0,
816+
MatchEntryValue::Value(ValueTypes::Ptr(v)) => {
817+
v.first().is_some_and(|b| *b != 0)
818+
}
819+
_ => false,
820+
}
821+
})
822+
}

dpd-client/tests/integration_tests/mcast.rs

Lines changed: 2 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1442,14 +1442,6 @@ async fn test_ipv6_multicast_invalid_destination_mac() -> TestResult {
14421442
let ctr_baseline =
14431443
switch.get_counter("multicast_invalid_mac", None).await.unwrap();
14441444

1445-
let port_label_ingress = switch.port_label(ingress).unwrap();
1446-
1447-
// Check the Multicast_Drop counter baseline for the ingress port
1448-
let drop_mcast_baseline = switch
1449-
.get_counter(&port_label_ingress, Some("multicast_drop"))
1450-
.await
1451-
.unwrap();
1452-
14531445
switch.packet_test(vec![test_pkt], expected_pkts).unwrap();
14541446

14551447
check_counter_incremented(
@@ -1462,17 +1454,6 @@ async fn test_ipv6_multicast_invalid_destination_mac() -> TestResult {
14621454
.await
14631455
.unwrap();
14641456

1465-
// Verify that the Multicast_Drop counter also incremented
1466-
check_counter_incremented(
1467-
switch,
1468-
&port_label_ingress,
1469-
drop_mcast_baseline,
1470-
1,
1471-
Some("multicast_drop"),
1472-
)
1473-
.await
1474-
.unwrap();
1475-
14761457
cleanup_test_group(switch, get_group_ip(&created_group)).await
14771458
}
14781459

@@ -2156,7 +2137,8 @@ async fn test_encapped_multicast_geneve_mcast_tag_to_underlay_and_external_membe
21562137

21572138
#[tokio::test]
21582139
#[ignore]
2159-
async fn test_ipv4_multicast_drops_ingress_is_egress_port() -> TestResult {
2140+
async fn test_ipv4_ingress_multicast_drops_ingress_is_egress_port() -> TestResult
2141+
{
21602142
let switch = &*get_switch().await;
21612143

21622144
// Define test ports

0 commit comments

Comments
 (0)