[mcast] remove multicast feature + keep routing table sizes by zeeshanlakhani · Pull Request #371 · oxidecomputer/dendrite

zeeshanlakhani · 2026-09-09T14:12:14Z

This work is split up into two pieces in turning multicast "on" by default:

[mcast, feature] Feature removal

This removes the multicast feature gating altogether, making multicast inclusive to the work in dendrite/dpd/sidecar.

[p4, sidecar] keep (IPV*_LPM_SIZE - 1) entries for routing tables w/ multicast

Removing the multicast feature gate got us over Tofino's 20-stage pipeline limit. The table resizing work in #268 matched the v6 LPM table size to what we had in v4, putting us at 20 stages without multicast.

In #208, I explored a few different approaches torefactor our pipeline, across ingress and egress, to give us more breathing room (stage-wise). However, we want to be efficient and targeted when making changes to the sidecar.

Reducing mcast table sizes was my first attempt. Route and replication tables fit in SRAM; SSM is TCAM. The result, sadly, was fewer TCAM blocks, but the same stage count.

Instead, I went the direction of fulfilling a Nils'ism:

XXX: this control could be moved to the Egress pipeline if we need more space
in the Ingress pipeline. Currently unicast packets are able to bypass that
pipeline, which is why we've tacked it on here. We could probably also merge
it with the MacRewrite control, as they are both per-port settings, but that
would present some weird semantics to the control plane daemon.

I moved the EgressFilter control to the egress pipeline to save a stage, carrying (or bridging) the NAT egress bit in the bridge header. I did not move the MacRewrite control over to handle both uni+multi(cast) in egress, but that was demonstrated in #208 for a follow-up (post-testing).

With the move, 20 stages is achieved! However, the v4/v6 route tests still stopped below 8191 entries with a deficit. The exact-match route-target tables were losing capacity under four-way cuckoo placement (default). @ways(8) was the trick I found that gives each key in the table eight placement choices, which now makes table tests fill all 8191 entries.

Includes:

Removal of the #ifdef MULTICAST codegen guards.
Counter updates to match our move of the NAT egress filter

This removes the multicast feature gating altogether, making multicast inclusive to the work in dendrite/dpd/sidecar.

…multicast Removing the multicast feature gate got us over Tofino's 20-stage pipeline limit. The table resizing work in #268 matched the v6 LPM table size to what we had in v4, putting us at 20 stages without multicast. In #208, I explored a few different approaches to refactor our pipeline, across ingress and egress, to give us more breathing room (stage-wise). However, we want to be efficient and targeted when making changes to the sidecar. Reducing mcast table sizes was my first attempt. Route and replication tables fit in SRAM; SSM is TCAM. The result, sadly, was fewer TCAM blocks, but the same stage count. Instead, I went the direction of fulfilling a Nils'ism: > XXX: this control could be moved to the Egress pipeline if we need more space > in the Ingress pipeline. Currently unicast packets are able to bypass that > pipeline, which is why we've tacked it on here. We could probably also merge > it with the MacRewrite control, as they are both per-port settings, but that > would present some weird semantics to the control plane daemon. I moved the `EgressFilter` control to the egress pipeline to save a stage, carrying (or bridging) the NAT egress bit in the bridge header. I *did not* move the `MacRewrite` control over to handle both uni+multi(cast) in egress, but that was demonstrated in #208 for a follow-up (post-testing). With the move, 20 stages is achieved! However, the v4/v6 route tests still stopped below 8191 entries with a deficit. The exact-match route-target tables were losing capacity under four-way cuckoo placement (default). [`@ways(8)`][ways] was the trick I found that gives each key in the table eight placement choices, which now makes table tests fill all 8191 entries. Includes: - Removal of the `#ifdef MULTICAST` codegen guards. - Counter updates to match our move of the NAT egress filter - Ordering of the NAT egress filter before the CPU-copy (`egress_rid`) check, making sure NAT unicast is not treated as a multicast CPU copy. [ways]: https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183

cfzimmerman

From an aesthetic perspective, this PR is magnificent. Tysm for doing this. Very excited to see it land.
What's the plan for validation? All good if E2E testing needs to come at the top of the stack, but do our current CI tests cover these changes well? Both for new functionality and to prevent regressions. If not, I'd appreciate whatever is needed to fill the gap.
I'm still pretty new to our P4 program, so I'm unlikely to give a great review on that. Hopefully we can get at least one more reviewer. I'm also experimenting with Claude PR reviews. Imo it's not a replacement for human review, but they are a nice supplement. I'm going to have Claude take a look at this, and I'll comb through some of its comments to see how useful they are.

Lmk once you feel confident about validation, and I'm happy to stamp.

cfzimmerman

Recent changes lgtm. Although a second reviewer warranted before merge, particularly to review the p4 diffs.

FelixMcFelix

Thanks Zeeshan.

FelixMcFelix · 2026-09-14T14:58:59Z

-// TODO: these all need to be bigger. Early experimentation is showing that this
-// is going to need to come either through ATCAM/ALPM or code restructuring.


I think this comment still holds true. 1024 entries for, e.g., NAT is quite paltry compared to how many VMs you can feasibly run on a single rack (particularly with each OPTE port consuming several such entries).

Can return/extend it.

FelixMcFelix · 2026-09-14T16:29:15Z

+	 *
+	 * See https://github.com/p4lang/p4c/blob/a19f1c3d85a867a6288fd983f7bad505ac47d728/backends/tofino/bf-p4c/common/pragma/pragmas.cpp#L1172-L1183.
 	 */
+	@ways(8)


Out of curiosity, do we have any idea what this defaults to?

I'll add that to the comment.

FelixMcFelix · 2026-09-14T17:53:41Z

 			if (ig_tm_md.ucast_egress_port != USER_SPACE_SERVICE_PORT) {
 				mac_rewrite.apply(hdr, ig_tm_md.ucast_egress_port);
 			}
-			meta.bridge_hdr.setInvalid();
-			ig_tm_md.bypass_egress = 1w1;
+			if (meta.nat_egress_hit && !meta.service_routed) {
+				meta.bridge_hdr.nat_egress_hit = true;
+			} else {
+				meta.bridge_hdr.setInvalid();
+				ig_tm_md.bypass_egress = 1w1;
+			}
 		}


I think the change in meanings of counters is confusing enough that it should be documented or worked around.

My current read is that to get the true egress count on a port, you need to compute pipe.Ingress.egress_ctr[port_idx] - pipe.Egress.drop_port_ctr[port_idx]. Not unreasonable, but it requires decent knowledge of the dataplane to make sense. Thinking on alternate approaches:

Could we move the egress counter hit into the else block? This way the sum of packets allowed to leave on a port is pipe.Ingress.egress_ctr[port_idx] + pipe.Egress.unicast_ctr[port_idx] (plus + pipe.Egress.mcast_ctr[port_idx], if you're so inclined). The counter names are still a little misleading this way, in that Ingress.egress_ctr would be underlay plus inbound nat'd traffic, whereas Egress.unicast_ctr is outbound traffic being decapsulated successfully.

Does bypass_egress give us meaningful latency improvement? Does it make enough difference to prevent us pushing all packets through the egress pipeline, and then we only need pipe.Egress.unicast_ctr[port_idx]? This would also mesh well with making MacRewrite an unconditional part of the egress pipeline in future.

zeeshanlakhani · 2026-09-15T00:56:45Z

On #371 (comment),

I was being a bit conservative here, but the second alternative approach is what I already explored in #208 (with the MAC rewriting, however). I'll make that update, as it's cleaner anyway and sets us up for that move.

This work follows up on Kyle's review, including adding documentation on `@ways` defaults (and now refers to a more informational link) and removing leftover, unnecessary `{}` blocks. The main change is that every packet copy now runs through to the egress pipeline, no longer relying on `bypass_egress` in the ingress pipeline. This follows from #208 and makes the counters more explicit: - `forwarded_ctr` records all packets leaving a port; - `unicast_ctr` and `mcast_ctr` now partition what gets forwarded by type; - drops are recorded separately with port and reason counters.

zeeshanlakhani requested review from FelixMcFelix, cfzimmerman, rcgoodfellow and taspelund September 9, 2026 14:24

zeeshanlakhani added 2 commits September 9, 2026 14:36

[mcast, feature] remove multicast feature; make it truly default

8c9c42b

This removes the multicast feature gating altogether, making multicast inclusive to the work in dendrite/dpd/sidecar.

zeeshanlakhani mentioned this pull request Sep 9, 2026

[mcast, on] default multicast feature across all crates #361

Closed

zeeshanlakhani force-pushed the zl/mcast-remove-feature branch from 61460dc to 7790240 Compare September 9, 2026 14:49

zeeshanlakhani added this pull request to stack #372 September 9, 2026 14:54

cfzimmerman reviewed Sep 10, 2026

View reviewed changes

Comment thread dpd-client/tests/integration_tests/table_tests.rs

Comment thread dpd/p4/constants.p4

Comment thread dpd/src/counters.rs Outdated

This comment was marked as resolved.

Sign in to view

[review] counter renaming, formatting, unskip fragmented tests

4d4fc50

zeeshanlakhani requested a review from cfzimmerman September 11, 2026 07:18

cfzimmerman approved these changes Sep 11, 2026

View reviewed changes

FelixMcFelix reviewed Sep 14, 2026

View reviewed changes

zeeshanlakhani requested a review from FelixMcFelix September 15, 2026 09:01

		// TODO: these all need to be bigger. Early experimentation is showing that this
		// is going to need to come either through ATCAM/ALPM or code restructuring.

Uh oh!

Conversation

zeeshanlakhani commented Sep 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

[mcast, feature] Feature removal

[p4, sidecar] keep (IPV*_LPM_SIZE - 1) entries for routing tables w/ multicast

Uh oh!

cfzimmerman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

This comment was marked as resolved.

cfzimmerman left a comment

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix left a comment

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FelixMcFelix Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

FelixMcFelix Sep 14, 2026

Choose a reason for hiding this comment

Uh oh!

zeeshanlakhani commented Sep 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zeeshanlakhani commented Sep 9, 2026 •

edited

Loading