Repository navigation
Set Geneve encap source port using flow hash - #398
Conversation
19e9a61 to
d7482c3
Compare
This PR moves computation of any packet's flow hash (previously used for ECMP in the Router4/Router6 controls) to the head of packet processing. This allows us to use one flow hash to fill out the UDP source port for Geneve packets, and for ECMP as before. This is slightly different to what we had before in two ways. The first is that this hash includes the IpProtocol value, and the second is that we will use the outer flow hash rather than the inner when decapping and forwarding. Part of why this is still valid is that OPTE already sets the encap source port to the flow hash, so hashing early still includes the entropy from the inner flow hash. This ended up needing to be done because I couldn't convince p4c to let me place an extra two `Hash`es into `NatIngress`. This also fixes some of the external subnets & NAT unit ingress tests, which weren't *actually* generating the packet types we would have expected for each (we got all UDP, apparently!). Fixes #324.
d7482c3 to
e466bee
Compare
| }) & 0x3f; | ||
| // bits of the 16-bit hash calculated earlier to complete the | ||
| // 11-bit index. | ||
| fwd.ecmp_hash = 2w0 ++ meta.flow_hash[5:0]; |
There was a problem hiding this comment.
It feels a bit odd to be getting a bit<8> here with 6 bits occupied for select_route, but moving it from operating on a (bit<8>, bit<8>) to a (bit<6>, bit<5>) seemed like it'd be out of scope for this PR, particularly since res.slots is numbered 1--32 rather than 0--31.
There was a problem hiding this comment.
Converting the former argument was actually painless, done so in a59506b.
There was a problem hiding this comment.
...or not. I'm getting a test failure in route_ipv6::fill_until_ipv6_filler_alloc_unavailable that I can't make sense of right now. I'm going to say there's not much value to get out of this change worth debugging that, at the minute.
| if [ "$MULTICAST" == "" ]; then | ||
| TOFINO_STAGES=17 | ||
| else | ||
| TOFINO_STAGES=19 | ||
| fi |
There was a problem hiding this comment.
It looks like the limits on main were 18 for non-multicast, and 20 for multicast. I kinda wish there were a way to be alerted when we go down in stage count. Do we know when things improved here?
There was a problem hiding this comment.
On the framing here, going back: main's limit was 20 for both builds since #201 split multicast out as a separate feature but kept the one TOFINO_STAGES argument (a hard cap, as --stages becomes p4c's --num-stages-override).
The 18/20 has been what each build actually used, respectively. #208 (closed) had gotten these down to 16/19 with its refactoring of the pipeline, but none of that landed. Moving us to 17/19 here is great! It'll be awesome to combine this with #371, which also keeps the routing tables at full occupancy with @ways(8) (the table-size being finicky here is the same issue). With that work, multicast is always "on", as the feature flag goes away there.
| if [ "$MULTICAST" == "" ]; then | ||
| TOFINO_STAGES=17 | ||
| else | ||
| TOFINO_STAGES=19 | ||
| fi |
There was a problem hiding this comment.
On the framing here, going back: main's limit was 20 for both builds since #201 split multicast out as a separate feature but kept the one TOFINO_STAGES argument (a hard cap, as --stages becomes p4c's --num-stages-override).
The 18/20 has been what each build actually used, respectively. #208 (closed) had gotten these down to 16/19 with its refactoring of the pipeline, but none of that landed. Moving us to 17/19 here is great! It'll be awesome to combine this with #371, which also keeps the routing tables at full occupancy with @ways(8) (the table-size being finicky here is the same issue). With that work, multicast is always "on", as the feature flag goes away there.
rcgoodfellow
left a comment
rcgoodfellow
left a comment
There was a problem hiding this comment.
looks good to me. one question on multicast level hashes below.
| // Set L3/ECMP and L2/LAG multicast hashes using the packet's | ||
| // flow hash. This will include entropy from the inner flow, see | ||
| // the commentary on FlowHash. | ||
| ig_tm_md.level1_mcast_hash = meta.flow_hash[12:0]; |
There was a problem hiding this comment.
Maybe not a question for this PR. @zeeshanlakhani are we using ECMP level 1 node types or LAG level 2 node types? Put differently, are level1_mcast_hash and level2_mcast_hash being used for anything? ref: section 5.7.3.1 in the TNA.
There was a problem hiding this comment.
Thanks for taking the review. I don't believe we are, from my read on #398 (comment). They're filled out pre-emptively I think.
I'm going to merge this just now, setting these fields (or not) seems like a #371 question.
There was a problem hiding this comment.
Yep, taking this on.
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.
This PR moves computation of any packet's flow hash (previously used for ECMP in the Router4/Router6 controls) to the head of packet processing. This allows us to use one flow hash to fill out the UDP source port for Geneve packets, and for ECMP as before.
This is slightly different to what we had before in two ways. The first is that this hash includes the IpProtocol value, and the second is that we will use the outer flow hash rather than the inner when decapping and forwarding. Part of why this is still valid is that OPTE already sets the encap source port to the flow hash, so hashing early still includes the entropy from the inner flow hash. This ended up needing to be done because I couldn't convince p4c to let me place an extra two
Hashes intoNatIngress.This also fixes some of the external subnets & NAT unit ingress tests, which weren't actually generating the packet types we would have expected for each (we got all UDP, apparently!).
Fixes #324.