Repository navigation
[mcast] remove multicast feature + keep routing table sizes - #371
zeeshanlakhani wants to merge 8 commits into
Conversation
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
61460dc to
7790240
Compare
There was a problem hiding this comment.
- 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.
Lmk once you feel confident about validation, and I'm happy to stamp.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
cfzimmerman
left a comment
cfzimmerman
left a comment
There was a problem hiding this comment.
Recent changes lgtm. Although a second reviewer warranted before merge, particularly to review the p4 diffs.
| 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; | ||
| } | ||
| } |
There was a problem hiding this comment.
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 thatIngress.egress_ctrwould be underlay plus inbound nat'd traffic, whereasEgress.unicast_ctris outbound traffic being decapsulated successfully. - Does
bypass_egressgive us meaningful latency improvement? Does it make enough difference to prevent us pushing all packets through the egress pipeline, and then we only needpipe.Egress.unicast_ctr[port_idx]? This would also mesh well with makingMacRewritean unconditional part of the egress pipeline in future.
|
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.
FelixMcFelix
left a comment
FelixMcFelix
left a comment
There was a problem hiding this comment.
Okay, I'm happier with these changes now. I think this keeps the counter state somewhat sensible. I don't know what we want the wider testing plan to be yet before we merge, given the risk of London-bridges type scenarios. Are we wanting to validate the whole PR stack with product assurance etc. beforehand?
Good question on the latter. I'll tag @askfongjojo and @sion42x here as well. |
|
I ran @rcgoodfellow's tof PHVIngress (
Egress (
Both Ingress and Egress (
I also ran tof with vars + container kinds, focused on fields sharing ingress normal 16-bit containers (e.g.
|
| transition select(hdr.ipv4.protocol) { | ||
| IPPROTO_ICMP: parse_icmp; | ||
| IPPROTO_TCP: parse_tcp; | ||
| IPPROTO_UDP: parse_udp; | ||
| default: accept; | ||
| } |
There was a problem hiding this comment.
In my understanding, we only parse the IPv4 header on egress to check geneve options. And afaik an Oxide geneve header should only ever live behind an outer IPv6 header. So can we just skip parsing an L4 header on egress for IPv4 packets? Because our spec asserts there's never a geneve header within an IPv4 packet?
By the same principle, I'm curious if we can skip parsing a geneve header on IPv4 ingress as well, but that's beyond the scope of this PR.
If sidecar must support IPv4 encapsulated geneve traffic (why?), then this should probably be aware of IPv4 fragments and options. Because in both cases the bytes you're parsing here as an L4 header may be something else entirely. I'm handling this for ingress in another branch right now.
There was a problem hiding this comment.
Update: I would like to discuss this further. But maybe it doesn't need to hold up this PR because I'll be proposing it in my own PR soon. I would like to at least have a plan for dealing with irregularly shaped IPv4 packets on egress before we merge this.
There was a problem hiding this comment.
So, I ended up taking this on here. It's worth it, I'd say.
Egress parse_ipv4 will now just extract and accept. For IPv6, we only continue to UDP/Geneve parsing when the outer dst is in our underlay ff04::/64 block (and the next_hdr is UDP), as that's the only thing the egress pipeline actually needs Geneve for.
The tof2 parser only has four 8-bit match registers (citation provided in the diff), which meant splitting the /64 check across parser states. I went with 16 bits per state to keep them uniform. I'll leave ingress (fragments/options) to your upcoming work.
There was a problem hiding this comment.
Nice! We'll need Ry's review on this one as well, but imo this is a great conceptual win. And I think it's a step closer to rfd 63.
Worth noting, I believe we compute an incorrect checksum if the egressing IPv4 packet has options because option bytes should be part of the checksum.
Lines 2387 to 2401 in bc40ea7
But that's a niche case, and IPv4 ingress currently has the same checksum gap. I'm inclined to want all checksum fixes in the branch where I fix ingress. Parsing IPv4 options for all cases in a single PR might be easiest, particularly since the data structure for options bytes is a murky design decision.
So in summary, lgtm!
There was a problem hiding this comment.
Yeah, makes sense to come back to this in one checksum swoop.
> In my understanding, we only parse the IPv4 header on egress to check > geneve options. And afaik an Oxide geneve header should only ever live > behind an outer IPv6 header. So can we just skip parsing an L4 header > on egress for IPv4 packets? Because our spec asserts there's never a > geneve header within an IPv4 packet? This was a good point. The egress pipeline only needs Geneve handling for underlay mcast replicas (outer hdr with `ff04::/64`, always IPv6), but we were walking `IPv4` -> `UDP` -> `Geneve` for UDP traffic to the Geneve port (6081), and similarly for IPv6, where decapped external replicas or customer traffic to port 6081 could end up in the Geneve parser. For IPv4, options or a later fragment's payload could have been read as UDP, with an apparent dst port of 6081 transitioning into the Geneve parser state. Now, we get: - IPv4: extract the header and accept - IPv6: only goes on to UDP for `ff04::/64` + UDP.
|
Has this been run on a racklette where we've seen mcast packets flowing from an off-rack source all the way down to a target instance? I think we want to get through that test before we merge this. |
When merging forward the `main` branch's/bookmark's flow hash (#398) work, two components broke in the combination: - `level2_mcast_hash`: With `flow_hash[15:3]` being a shifted copy, this pushes `flow_hash` into a 32b container, and the egress decap bitmap no longer fits in PHV. - *fix*: level2 is now a `Hash<bit<13>>` over `{flow_hash, ingress_port}` - Note: Neither level has an ECMP/LAG node to incorporate at the moment per the #398 thread. - With the changes upstream, p4c mistakenly reused the match register holding `dst_mac[39:32]` for the IPv4 protocol byte, causing IPv4 multicast packets to be dropped due to "invalid MACs". - This existing IPv6 MAC check is also in the `Filter` control. - *fix*: the IPv4 mcast OUI (Organizationally Unique Identifier) check has been moved from the parser to the `Filter` control. Includes: - `TOFINO_STAGES=19` remains the same as `main` and is unconditional. - Usable IPv4 route capacity doesn't change here.
|
Note that there's some TTL/parser cleanup that happens in #400 as well, specific to TTL/hop limit checking. cc @taspelund. |
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 it to where multicast is always built into dpd and sidecar.p4.
[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 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:
I moved the
EgressFiltercontrol to the egress pipeline to save a stage, carrying (or bridging) the NAT egress bit in the bridge header. I did not move theMacRewritecontrol over to handle both uni+multi(cast) in egress, but that was demonstrated in #208 for a follow-up (post-testing).With this move, we got to 20 stages (19 after the main merge, below)! 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.[merge] main (#398) fallout
@FelixMcFelix's flow hash (#398) merged atop this broke two things that previous incarnations of this PR didn't hit:
level2_mcast_hash: withflow_hash[15:3]being a shifted copy, this pushesflow_hashinto a 32b container, and the egress decap bitmap no longer fits in PHV.Hash<bit<13>>over{flow_hash, ingress_port}.thread
dst_mac[39:32]for the IPv4 protocol byte, causing IPv4 multicast packets to be dropped due to "invalid MACs".01:00:5echeck (IANA's00:00:5eOUI with the group bit set) has been moved from the parser to theFiltercontrol.Filter, too.Includes:
#ifdef MULTICASTcodegen guards.common.shfollows with one limitTOFINO_STAGES=19and withMULTICASTgone.