[vpp] VXLAN L3 dataplane: inner-aware encap hash, underlay ECMP, source-independent decap (patches 0015-0017) - #262
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
592e27d to
efe4ea1
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
efe4ea1 to
c7ed40f
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…MP, source-independent VNET decap
Add three VPP patch-series entries required to enable VXLAN dataplane
features on sonic-vpp, and bump VPP_VERSION so the buildkite deb cache
misses and the patches are recompiled into the image:
- 0012 VXLAN encap inner-aware flow hash: hash the inner IP 5-tuple on
L3 VXLAN encap so inner flows spread across underlay ECMP / LAG paths
instead of collapsing onto a single next-hop / bond member.
- 0013 VXLAN underlay ECMP load-balance tolerance: tighten the multipath
load-balance error tolerance so all equal-cost underlay paths are
retained (buckets:256), fixing near-ideal 3-way distribution.
- 0014 VXLAN VNET source-independent ("decap-any") decap: match on local
dst + VNI ignoring the outer source, supporting RIOT / secondary-VTEP
VNET decap, plus an l2_bvi helper.
rules/vpp.mk: VPP_VERSION 2606-0.3 -> 2606-0.5 (patch-series changed).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa
Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
Address code-review findings on the VXLAN VNET source-independent
("decap-any") decap patch series:
- Scope the source-independent wildcard bihash entry and the L3-promiscuous
BVI receive to secondary-VTEP decap terms only, via a decap-any flag SAI
sets in the high bit (VXLAN_DECAP_ANY_FLAG) of the wire decap_next_index.
Ordinary VXLAN tunnels keep exact outer-source matching and strict BVI
my-mac.
- Check all wildcard clib_bihash_add_del return codes. An add failure rolls
back the exact entry and fails the tunnel; a delete-time re-point failure
drops the stale entry instead of leaving it pointing at a freed pool slot.
- Replace the per-packet device-class string match in l2_to_bvi with an O(1)
sw_if_index bitmap owned by l2_bvi and populated by the vxlan plugin,
scoping L3-promiscuous receive to decap-any ingress interfaces.
- Patch headers: clarify 0013 as a global FIB-ECMP tolerance change (LAG
unaffected; inner-aware spreading is 0012), and document the L2-EVPN and
mixed-family limitations in the 0014 header.
Bump VPP_VERSION 0.5 -> 0.6 so the prebuilt-deb cache misses and the updated
patches reach the built VPP.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa
Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
c7ed40f to
1e42841
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
…5-0017 master added sflow patches at series slots 0012-0014, colliding with this branch's VXLAN patches. Renumber the three VXLAN patches to 0015-0017 (file contents unchanged), keep both patch sets in vppbld/patches/series, and bump VPP_VERSION to 2606-0.7 so the build cache misses and VPP recompiles with the combined sflow + VXLAN series. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
8c73818 to
fdfd03d
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR advances SONiC-VPP’s VXLAN L3 dataplane support by adding a new VPP patch trio (0015–0017) to improve underlay load-balancing behavior, correctly hash on inner (overlay) flows for VXLAN encap, and enable source-independent (“decap-any”) VNET decap. It also bumps VPP_VERSION to ensure downstream builds rebuild VPP with these patches applied.
Changes:
- Register VPP patches 0015–0017 in the build patch series.
- Add VXLAN encap inner-aware flow hashing and underlay ECMP tolerance tightening.
- Add VXLAN VNET “decap-any” (source-independent) decap support plus a BVI L3-promiscuous fast path for decap-and-route.
- Bump
VPP_VERSIONto force a rebuild and avoid stale cached artifacts.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vppbld/patches/series | Adds patches 0015–0017 to the VPP patch application order. |
| vppbld/patches/0015-vxlan-encap-inner-aware-flow-hash.patch | Updates VXLAN encap behavior to hash on inner IP flow (overlay) for better ECMP/LAG spreading. |
| vppbld/patches/0016-sonic-vxlan-underlay-ecmp-loadbalance-tolerance.patch | Tightens global multipath normalization tolerance to retain equal-cost paths. |
| vppbld/patches/0017-sonic-vxlan-vnet-source-independent-decap.patch | Implements decap-any VXLAN VNET decap behavior and adds BVI routing bypass for decap-and-route. |
| rules/vpp.mk | Bumps VPP_VERSION to force rebuild with the new patch set. |
The 0016 description referenced "patch 0012" for inner-aware tunnel hashing, but after the series renumber that change is patch 0015 in this series (0012-0014 are master's sflow patches). Update the reference. This edits only the patch's description text (above the '---' separator), not the applied diff, so the compiled VPP artifact is byte-identical and no VPP_VERSION bump is required. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
@yejianquan heads-up on the Evidence:
Root cause (from #259's fuller log) — the i.e. trixie's Separately, I've addressed the two review comments on this PR: fixed the stale 0016 patch reference (0012 -> 0015), and replied on the l3-promiscuous BVI bypass (it wraps only the Happy to open a separate tracking issue for the trixie ext-deps break if it isn't already being tracked. Co-authored by CoPilot |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
vppbld/patches/0017-sonic-vxlan-vnet-source-independent-decap.patch:262
- Same issue as the IPv4 wildcard maintenance: the decap-any wildcard key for IPv6 is built from local dst IP + (UDP dst port, FIB, VNI), but the survivor selection also requires
src_portto match. Since the decap lookup key does not include UDP source port, this can incorrectly delete the wildcard entry while another matching decap-any tunnel remains.
+ if (ip6_address_is_equal (&st->src.ip6, &t->src.ip6) &&
+ st->vni == t->vni &&
+ st->encap_fib_index == t->encap_fib_index &&
+ st->src_port == t->src_port)
+ {
vppbld/patches/0017-sonic-vxlan-vnet-source-independent-decap.patch:216
- In the decap-any delete path, the wildcard entry is keyed by (local dst IP, UDP dst port, FIB, VNI). The survivor selection additionally requires
src_portto match, but the decap lookup key does not include UDP source port (see e.g.vppbld/plugins/tunterm_acl/tunterm_acl_decap.c:61-65, which keys onudp->dst_port,fib_index, andvni_reserved). This can cause the wildcard entry to be removed even though another decap-any tunnel with the same wildcard key still exists, breaking source-independent decap for that remaining tunnel.
This issue also appears on line 258 of the same file.
+ if (st->src.ip4.as_u32 == t->src.ip4.as_u32 &&
+ st->vni == t->vni &&
+ st->encap_fib_index == t->encap_fib_index &&
+ st->src_port == t->src_port)
+ {
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks for putting this together — I went through all three PRs (#262, sonic-sairedis#1996, sonic-mgmt#26220). Overall the series looks solid: the rollback ladders and refcounting in the saivpp side are careful, and the scope note you wrote into 0016 is exactly the kind of transparency that makes a downstream VPP patch reviewable. A few comments, none of them blocking except the CI note at the end. 0015 — inner-aware VXLAN encap hashThe diagnosis is convincing and worth calling out explicitly: One suggestion: 0011 was deliberately strict about being opt-in ("Existing hash-eth-l34 and IP_FLOW_HASH_DEFAULT are byte-for-byte unchanged"). 0015 replaces Minor: 0016 — global multipath load-balance toleranceTwo things worth separating here. First, in its defence: this patch is not a nice-to-have, it's load-bearing. At the default 0.1 tolerance a 3-way ECMP group quantizes to 16 buckets, which is roughly a 6% inherent distribution skew before any hashing behaviour enters the picture. That alone would make the distribution assertions in sonic-mgmt#26220 unpassable at any tolerance in that range, so I don't think the series works without it. Second, the reservation: as you note, 0017 — source-independent decapNo objections. The blast radius is well contained: CIThe current red on |
|
Thanks @yejianquan, appreciate the thorough pass. Responses per patch: 0015 — L3 gating suggestionI dug into this and I don't think a gate is needed, because the new
So L2/EVPN tunnels already take an identical path; the only frames whose hash actually changes are the L3-routed ones this patch is meant to fix. I also considered implementing the gate via The unchecked 0016 — startup-config knobAgreed this is the better long-term shape, and I'd like to do it — but as a follow-up rather than in this PR. Converting the — default preserving the upstream CIThe trixie red has actually moved past the earlier ext-deps Co-authored by CoPilot |
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Thanks for the detailed responses — all three land well. 0015 — withdrawing the gating suggestionYour equivalence argument is right and I'm happy to drop this. I checked u8 *l3h = (u8 *) eh + vnet_buffer (b)->l2.l2_len;
u16 ethertype = clib_net_to_host_u16 (*(u16 *) (l3h - 2));With The unchecked deref is fine to leave as-is given it matches the pre-existing assumption at these call sites. 0016 — follow-up split works for meAgreed, and your reasoning for splitting is better than doing it here: the knob changes bucket allocation, so it wants its own qualification and version bump rather than riding along with the VXLAN work. The proposed shape is exactly what I had in mind: defaulting to upstream CIConfirmed green now (4/4). For the record, the earlier red never got as far as compiling VPP — all 17 patches applied cleanly at 12:26:30 and the failure was in Nothing outstanding from me on this one. |
| * distribution error tolerance for load-balancing | ||
| */ | ||
| -const f64 multipath_next_hop_error_tolerance = 0.1; | ||
| +const f64 multipath_next_hop_error_tolerance = 0.01; |
There was a problem hiding this comment.
smaller tolerance means more bucket with same weight distribution. It can increase memory usage with more buckets and reduce throughput (inline bucket is 4).
The thing I don't agree is treating vpp as only a test vehicle and make vpp behaves the same way as sonic-mgmt desires. We have hardware platform (cisco C0 product) using vpp as dataplane, which cares memory usage and throughput. If we look at sonic-mgmt, different platforms have different capabilities and test cases can adapt to it. vpp is no different. Is it possible to change the test case for vpp to adapt to the capability? It doesn't lose code coverage.
There was a problem hiding this comment.
@yue-fred-gao thanks for reviewing! I removed patch 0016 and we keep tolerance of 0.1. I updated the sonic-mgmt PR so that random hash check uses 0.2 for VPP only, while others stay at 0.3. I revalidated on the t1-lag-vpp testbed with 0016 dropped: Test_VxLAN_ecmp_random_hash and Test_VxLAN_entropy pass 20/20 across all four encap types, no cores, and BGP fully converged.
|
have you considered or started upstreaming the changes to fdio/vpp? I am concerned with the growing number of patches. Each patch adds potential task of merging with upstream changes. So we should try to make the change "upstreamable". For example, don't change existing behavior and new feature can be turned on in someway. |
Per maintainer review on sonic-net#262, VPP package versioning is owned by the monthly release process, so this feature PR should not carry a per-PR minor-suffix bump. Revert VPP_VERSION from 2606-0.7 back to master's 2606-0.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa Signed-off-by: Aaron Bernardino <aaronber@microsoft.com>
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
Patch 0016 tightened multipath_next_hop_error_tolerance from 0.1 to 0.01 in src/vnet/dpo/load_balance.c. That is a global FIB load-balance constant used by every equal-cost group on all ASIC targets, not just the VPP VXLAN underlay, so tightening it changed shared forwarding behavior for real dataplanes. Drop the patch and keep the default 0.1 tolerance. The non-power-of-2 bucket skew that 0016 was papering over (a 3-way group lands on a 16-bucket 6/5/5 load-balance split) is a property of VPP's default multipath allocation, so it is absorbed in the sonic-mgmt VXLAN ECMP test tolerance instead of by recompiling a global constant. Series now applies 0015 (encap inner-aware flow hash) and 0017 (source- independent VNET decap). VPP_VERSION stays 2606-0.3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3446b9ac-fa7f-4d26-914b-a4f076c603aa
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
@yue-fred-gao yes I have considered upstreaming but you're right that it's better to make the changes "upsteramable". I will this task to my backlog items to work at. For 0015 (inner aware VXLAN encap hash) - I think this is the best upstream candidate. vxlan_encap_inner_flow_hash() is byte for byte identical to vnet_l2_compute_flow_hash() for every existing frame type, untagged L2, tagged, and non IP all take the same path, so it does not change existing behavior. It only adds inner entropy for the L3 routed tunnel frames that today collapse onto a single outer UDP source port, which is a straight bug fix. Once it merges upstream I drop the local patch and bump vpp_version, so the series gets smaller. For 0017 (source independent decap) - this is already opt in in the shape you want. The decap_any behavior only takes effect for tunnels explicitly marked for it, the default decap path is unchanged, and the mark is cleared on tunnel delete. It touches more API surface, so I expect it to need more discussion with the VPP maintainers, but maybe I can upstream this as an opt in feature and carry it downstream only until it lands. For 0016 - dropped the patch Thanks Co-authored with CoPilot. |
Thanks. I will review it the next week. |
Description of PR
Summary:
Enable correct L3 VXLAN forwarding on the sonic-vpp platform by adding the VPP
dataplane patch series that the saivpp adaptation and tests depend on.
Part of sonic-net/sonic-buildimage#25777
This is the dataplane layer (layer 1) of a cross-repo feature and lands first:
Changes (vppbld/patches):
flow for VXLAN encap so IPinIP/VXLAN transit traffic spreads across ECMP
next-hops and LAG members instead of polarizing onto a single path.
(decap-any) VNET decap plus review hardening (checked wildcard bihash return codes and an O(1) L3-promiscuous BVI receive bitmap).
Type of change
Approach
What is the motivation for this PR?
On sonic-vpp, L3 VXLAN transit traffic polarized onto a single next-hop / bond
member (no inner-aware hashing) and source-independent VNET decap was missing.
These fixes require VPP dataplane behavior that cannot be handled in the SAI
adaptation layer (saivpp / vslib/vpp):
VPP encap node derived the outer UDP source port from a mis-parsed inner
frame on L3-routed tunnels (patch 0015).
outer source for a given local dst + VNI; VPP keys decap on the exact
(src, dst, vni) tuple, so a new wildcard match path was required (patch
0017). The matching saivpp inner-source-MAC and decap wiring lands in
[vpp] saivpp VXLAN L3 decap: source-independent decap, inner source MAC, BD teardown fix sonic-sairedis#1996.
Note on underlay ECMP fairness: an earlier revision of this series also carried
a patch (0016) that tightened VPP's global FIB multipath load-balance tolerance
(multipath_next_hop_error_tolerance 0.1 -> 0.01) so non-power-of-2 equal-cost
groups allocate more buckets. That constant is shared by every ASIC target, not
just the VPP VXLAN underlay, so it has been dropped to avoid changing shared
forwarding behavior. The resulting bucket skew for non-power-of-2 groups is a
property of VPP's default multipath allocation and is handled in the sonic-mgmt
VXLAN ECMP test tolerance instead (sonic-net/sonic-mgmt#26220).
How did you do it?
Added patches 0015 and 0017 to the vppbld patch series (after master's sflow
patches 0012-0014).
How did you verify/test it?
Built a sonic-vpp image from this branch and ran the hashing/VXLAN regression
suite on a t1-lag-vpp testbed: fib/test_fib.py hashing pass; the VXLAN ECMP and
entropy suites (Test_VxLAN_ecmp_random_hash, Test_VxLAN_entropy) pass across all
four encap types with the default multipath tolerance and the test-side
tolerance in #26220; VNET decap tracks green. CI build+boot proof via the
validation image PR sonic-net/sonic-buildimage#28437.
Any platform specific information?
sonic-vpp platform only.
Documentation
No SONiC doc/HLD changes in this PR.