Count ICMP-needed drops from L3Router by FelixMcFelix · Pull Request #406 · oxidecomputer/dendrite

FelixMcFelix · 2026-10-16T11:31:57Z

Packets which are dropped in L3Router and its nested controls all need to be handled by the host, so we cannot actually drop these packets in the Tofino. However the counters were only ever modified when the packet is truly dropped, so these counters were non-functional. This PR separates out the counter increment to occur for both classes of drop.

I did look into centralising everything onto ig_dprsr_md.drop_ctl and ridding ourselves of meta.dropped, but this ended up both uglier and took up more stages under multicast. Simple seems best, here!

Fixes #404.

Packets which are dropped in L3Router and its nested controls all need to be handled by the host, so we cannot actually drop these packets in the Tofino. However the counters were only ever modified when the packet is truly dropped, so these counters were non-functional. This PR separates out the counter increment to occur for both classes of drop. I did look into centralising everything onto `ig_dprsr_md.drop_ctl` and ridding ourselves of `meta.dropped`, but this ended up both uglier and took up more stages under multicast. Simple seems best, here! Fixes #404.

cfzimmerman

Lgtm. Just want to double check the motivation.

I believe this now increments the drop counter for ARP and NDP packets that are handled by the switch zone. We want that behavior?

cfzimmerman · 2026-10-22T17:11:16Z

+/// If the test runs to completion we return the counter evaluation as a
+/// boolean rather than an Error.  This allows us to trigger the error in
+/// the test routine, which gives us a more specific test failure message
+pub async fn one_counted_test(


Noting that this is somewhat similar to check_counter_incremented in https://github.com/oxidecomputer/dendrite/pull/362/changes#diff-892387f02bc3907dbe15941e9c76aecc0b8ca4b85d61f6a6130686aaba5b77ca

Which was moved from a helper that already exists in main. Could we easily achieve the same end with only one of these?

dendrite/dpd-client/tests/integration_tests/mcast.rs

Lines 74 to 103 in 9ffd5a1

async fn check_counter_incremented(

switch: &Switch,

counter_name: &str,

baseline: u64,

expected_increment: u64,

client_name: Option<&str>,

) -> anyhow::Result<u64> {

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

))

}

cfzimmerman · 2026-10-22T17:44:14Z

+		}
+
+		if (meta.dropped) {
+			ig_dprsr_md.drop_ctl = 1;


I believe these are all the cases this affects. There's some duplication for the multicast blocks. Is it worth adding checks for the TTL, ARP, and NDP cases?

dendrite/dpd/p4/sidecar.p4

Lines 1141 to 1143 in 9ffd5a1

meta.drop_reason = DROP_ARP_MISS;

// Dont set meta.dropped because we want an error packet

// to go up to the switch.

dendrite/dpd/p4/sidecar.p4

Lines 1189 to 1191 in 9ffd5a1

meta.drop_reason = DROP_NDP_MISS;

// Dont set meta.dropped because we want an error packet

// to go up to the switch.

dendrite/dpd/p4/sidecar.p4

Lines 1247 to 1249 in 9ffd5a1

// Dont set meta.dropped because we want an error packet

// to go out.

meta.drop_reason = DROP_IPV4_UNROUTEABLE;

dendrite/dpd/p4/sidecar.p4

Lines 1252 to 1254 in 9ffd5a1

// Dont set meta.dropped because we want an error packet

// to go out.

meta.drop_reason = DROP_IPV4_TTL_EXCEEDED;

dendrite/dpd/p4/sidecar.p4

Lines 1329 to 1331 in 9ffd5a1

meta.drop_reason = DROP_IPV4_UNROUTEABLE;

// Dont set meta.dropped because we want an error packet

// to go out.

dendrite/dpd/p4/sidecar.p4

Lines 1334 to 1336 in 9ffd5a1

meta.drop_reason = DROP_IPV4_TTL_INVALID;

// Dont set meta.dropped because we want an error packet

// to go out.

dendrite/dpd/p4/sidecar.p4

Lines 1386 to 1388 in 9ffd5a1

meta.drop_reason = DROP_IPV6_UNROUTEABLE;

// Dont set meta.dropped because we want an error packet

// to go out.

dendrite/dpd/p4/sidecar.p4

Lines 1391 to 1393 in 9ffd5a1

meta.drop_reason = DROP_IPV6_TTL_EXCEEDED;

// Dont set meta.dropped because we want an error packet

// to go out.

dendrite/dpd/p4/sidecar.p4

Lines 1465 to 1467 in 9ffd5a1

meta.drop_reason = DROP_IPV6_UNROUTEABLE;

// Dont set meta.dropped because we want an error packet

// to go out.

dendrite/dpd/p4/sidecar.p4

Lines 1470 to 1472 in 9ffd5a1

meta.drop_reason = DROP_IPV6_TTL_EXCEEDED;

// Dont set meta.dropped because we want an error packet

// to go out.

taspelund

Nice improvement to the tests! Looks like a pretty straightforward change.

cfzimmerman · 2026-10-22T20:17:27Z

Inspired by this PR, I had claude write a template and script formatting compiler diffs into markdown. The P4 changes here are fairly benign, but it's nice to keep an eye on how it evolves.

TOF report: dendrite#406

Count ICMP-needed drops from L3Router

	Target branch	PR
Branch	`main`	`felixmcfelix/fix-404`
Commit	`9ffd5a1a49eb`	`c4068517dd8d`
Codegen	`--stages 19 --multicast`	`--stages 19 --multicast`
tof	ry/tof @ `ed37578b`	ry/tof @ `ed37578b`

Summary

From tof dump --summary, tof phv and pipe/logs/metrics.json

	`main`	`felixmcfelix/fix-404`	Δ
Last stage used, ingress (0–18)	18	18	0
Last stage used, egress (0–18)	5	5	0
PHV containers, ingress (of 280)	158	158	0
PHV containers, egress (of 280)	44	44	0
PHV containers, total (of 280)	197	197	0
MAU SRAMs	259	259	0
MAU TCAMs	94	94	0
MAU logical tables	133	134	+1
Parser TCAM rows, ingress	104	104	0
Parser TCAM rows, egress	54	54	0

Compiler resources

From pipe/logs/metrics.json

Resource	`main`	`felixmcfelix/fix-404`	Δ
Parser TCAM rows, ingress	104	104	0
Parser TCAM rows, egress	54	54	0
MAU logical tables	133	134	+1
MAU SRAMs	259	259	0
MAU TCAMs	94	94	0
MAU map RAMs	90	90	0
Exact-match crossbar bytes	274	275	+1
Ternary-match crossbar bytes	169	169	0
Action bus bytes	482	482	0
Latency, ingress (cycles)	410	410	0
Latency, egress (cycles)	154	154	0
Power estimate, ingress (W)	14.87	14.93	+0.07
Power estimate, egress (W)	1.59	1.59	0
Deparser FDE entries, ingress	51	51	0
Deparser FDE entries, egress	42	42	0
Deparser POV bits, ingress	21	21	0
Deparser POV bits, egress	17	17	0
Compile time (s)	83.72	83.89	+0.17

PHV containers

Ingress

From tof phv -g ingress sidecar.bfa

Ingress PHV	`main`	`felixmcfelix/fix-404`
Normal word	30 / 48 (62.5%)	30 / 48 (62.5%)
Normal half	61 / 72 (84.7%)	61 / 72 (84.7%)
Normal byte	9 / 48 (18.8%)	9 / 48 (18.8%)
Mocha word	15 / 16 (93.8%)	15 / 16 (93.8%)
Mocha half	23 / 24 (95.8%)	23 / 24 (95.8%)
Mocha byte	8 / 16 (50.0%)	8 / 16 (50.0%)
Dark word	2 / 16 (12.5%)	2 / 16 (12.5%)
Dark half	10 / 24 (41.7%)	10 / 24 (41.7%)
Dark byte	0 / 16 (0.0%)	0 / 16 (0.0%)
Total	158 / 280 (56.4%)	158 / 280 (56.4%)

Egress

From tof phv -g egress sidecar.bfa

Egress PHV	`main`	`felixmcfelix/fix-404`
Normal word	12 / 48 (25.0%)	12 / 48 (25.0%)
Normal half	6 / 72 (8.3%)	6 / 72 (8.3%)
Normal byte	12 / 48 (25.0%)	12 / 48 (25.0%)
Mocha word	1 / 16 (6.2%)	1 / 16 (6.2%)
Mocha half	1 / 24 (4.2%)	1 / 24 (4.2%)
Mocha byte	9 / 16 (56.2%)	9 / 16 (56.2%)
Dark word	0 / 16 (0.0%)	0 / 16 (0.0%)
Dark half	2 / 24 (8.3%)	2 / 24 (8.3%)
Dark byte	1 / 16 (6.2%)	1 / 16 (6.2%)
Total	44 / 280 (15.7%)	44 / 280 (15.7%)

Total

From tof phv sidecar.bfa

Total PHV	`main`	`felixmcfelix/fix-404`
Normal word	41 / 48 (85.4%)	41 / 48 (85.4%)
Normal half	65 / 72 (90.3%)	65 / 72 (90.3%)
Normal byte	20 / 48 (41.7%)	20 / 48 (41.7%)
Mocha word	16 / 16 (100.0%)	16 / 16 (100.0%)
Mocha half	24 / 24 (100.0%)	24 / 24 (100.0%)
Mocha byte	16 / 16 (100.0%)	16 / 16 (100.0%)
Dark word	2 / 16 (12.5%)	2 / 16 (12.5%)
Dark half	12 / 24 (50.0%)	12 / 24 (50.0%)
Dark byte	1 / 16 (6.2%)	1 / 16 (6.2%)
Total	197 / 280 (70.4%)	197 / 280 (70.4%)

PHV fields

From tof vars

No change

FelixMcFelix added this pull request to stack #407 October 16, 2026 11:35

FelixMcFelix marked this pull request as ready for review October 16, 2026 11:37

FelixMcFelix requested a review from cfzimmerman October 16, 2026 11:38

Base automatically changed from felixmcfelix/fix-324 to main October 20, 2026 12:59

FelixMcFelix force-pushed the felixmcfelix/fix-404 branch from 21e3553 to c406851 Compare October 20, 2026 12:59

cfzimmerman approved these changes Oct 22, 2026

View reviewed changes

taspelund approved these changes Oct 22, 2026

View reviewed changes

Navigation Menu

Repository navigation

Count ICMP-needed drops from L3Router - #406

Count ICMP-needed drops from L3Router#406
FelixMcFelix wants to merge 1 commit into
mainfrom
felixmcfelix/fix-404

FelixMcFelix commented Oct 16, 2026

Uh oh!

cfzimmerman left a comment •

edited

Loading

Uh oh!

cfzimmerman Oct 22, 2026

Uh oh!

cfzimmerman Oct 22, 2026

Uh oh!

taspelund left a comment

Uh oh!

cfzimmerman commented Oct 22, 2026 •

edited

Loading

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	async fn check_counter_incremented(
	switch: &Switch,
	counter_name: &str,
	baseline: u64,
	expected_increment: u64,
	client_name: Option<&str>,
	) -> anyhow::Result<u64> {
	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
	))
	}

	meta.drop_reason = DROP_ARP_MISS;
	// Dont set meta.dropped because we want an error packet
	// to go up to the switch.

	meta.drop_reason = DROP_NDP_MISS;
	// Dont set meta.dropped because we want an error packet
	// to go up to the switch.

	// Dont set meta.dropped because we want an error packet
	// to go out.
	meta.drop_reason = DROP_IPV4_UNROUTEABLE;

	// Dont set meta.dropped because we want an error packet
	// to go out.
	meta.drop_reason = DROP_IPV4_TTL_EXCEEDED;

	meta.drop_reason = DROP_IPV4_TTL_INVALID;
	// Dont set meta.dropped because we want an error packet
	// to go out.

	meta.drop_reason = DROP_IPV6_UNROUTEABLE;
	// Dont set meta.dropped because we want an error packet
	// to go out.

	meta.drop_reason = DROP_IPV6_TTL_EXCEEDED;
	// Dont set meta.dropped because we want an error packet
	// to go out.

Conversation

FelixMcFelix commented Oct 16, 2026

Uh oh!

cfzimmerman left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cfzimmerman Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

cfzimmerman Oct 22, 2026

Choose a reason for hiding this comment

Uh oh!

taspelund left a comment

Choose a reason for hiding this comment

Uh oh!

cfzimmerman commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TOF report: dendrite#406

Summary

Compiler resources

PHV containers

Ingress

Egress

Total

PHV fields

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cfzimmerman left a comment •

edited

Loading

cfzimmerman commented Oct 22, 2026 •

edited

Loading