vpp: support punting ARP, LLDP, LACP, DHCP via member interface in VLAN (re-opened) - #1981
Conversation
Signed-off-by: dypet <dypeters@cisco.com>
Signed-off-by: dypet <dypeters@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
dypet
changed the title
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Approving at head f9cb68e.
This is #1959 re-opened (original owner OOO). I reviewed #1959 earlier with five findings; since this is a new PR number, that inline history doesn't carry over, so here's the status verified against source at this head:
Resolved (4/5):
- ✅ Null-deref in the new reply handlers (the one real bug) — fixed.
classify_add_del_table,get_next_index, andadd_node_nexthandlers now all guardif (msg->context) { ...; if (ptr) {...} }, so a stale/duplicate reply (NULL fromget_index_ptron generation mismatch) can no longer be dereferenced. Matches thecreate_subifguard pattern. - ✅
l2_punt_classify_applyreturn value — now checked. Both call sites invpp_create_vlan_memberlog a WARN on failure with an explicit best-effort comment; a member no longer silently reports success with punt unconfigured. - ✅ Lazy-init threading assumption — documented.
l2_punt_classify_initnow states it's called only from the single SAI processing thread, so thes_*globals need no locking. - ✅ WR macro change — now disclosed. The description has a dedicated "WR macro timeout change" section explaining the 1s→10s hard cap / 1s idle-cap rationale. That was my ask: it's a cross-cutting change to all ~67
WRcall sites and shouldn't ride silently in a feature PR. Sound change, now called out. (Minor, non-blocking: worth confirmingvl_socket_client_read(1) == 0reliably means "one message processed" and can't refresh the idle deadline on a spurious wakeup.)
One non-blocking cleanup remains (inline): vpp_classify_table_delete / vpp_classify_session_del are still dead — defined and exported but never called; l2_punt_classify_remove only detaches tables, never deletes them (tables are process-lifetime by design). Either drop them + the header decls, or add a one-line note that they're kept intentionally. Cleanup, not a blocker.
The core design is unchanged and remains excellent — the l2-input-classify slot-selection reasoning (IP4 vs OTHER by outer ethertype, the tagged/untagged offset math, why LACP and tagged-LLDP bypass the BD) is genuinely well-documented. The one substantive bug is fixed and verified. Approving; please tidy the dead wrappers in a follow-up or a quick push.
| return ret; | ||
| } | ||
|
|
||
| int vpp_classify_table_delete(uint32_t table_index) |
There was a problem hiding this comment.
🟡 Non-blocking — dead code (carried over from #1959). vpp_classify_table_delete (here) and vpp_classify_session_del (~L4487) are defined and declared in the header but never called anywhere — verified across SaiVppXlate.c and SwitchVppFdb.cpp. l2_punt_classify_remove only detaches tables from the interface via vpp_classify_set_interface_l2_tables(hwif, ~0, ~0, ~0); it never deletes a table or session, and the classify tables are process-lifetime by design. So these two wrappers are unused exported API. Suggest either dropping them + their header decls, or adding a one-line comment noting the tables are intentionally process-lifetime and the delete wrappers are kept for symmetry/future teardown. Cleanup only — not blocking the approval.
There was a problem hiding this comment.
Keeping for symmetry of the add/del pair and to avoid a follow-up PR when needed.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: dypet <dypeters@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@lolyu , @yejianquan , please help review and merge. |
Xichen96
added
Request for 202605 Branch
Tested for 202605 branch
|
[Exception] Approved. This is a new feature. The cherry pick rules prohibit feature picks. Since this change is limited to vslib I am signing off. Please still update the PR with a justification for this exception according to you |
6 participants
Description of PR
Summary:
Fixes #26936
Opening PR for #1959 while original owner is out of office.
Type of change
Approach
What is the motivation for this PR?
SONiC on the VPP virtual-switch platform needs L2 bridging with an L3 SVI (VLAN interface) for both tagged and untagged VLAN members. The standard SONiC data model (VLAN + VLAN_INTERFACE with an IP + VLAN_MEMBER tagged/untagged) maps to a VPP bridge-domain (BD) with a Bridge Virtual Interface (BVI), but VPP does not wire this up to SONiC's punt model out of the box. Two problems had to be solved:
No per-member control-plane punt. SONiC expects the NPU to punt control traffic (ARP, LLDP, LACP, DHCP, and L3-to-SVI) to the member interface so Linux sees frames on the same netdev it would on real hardware — never on a virtual Vlan/PortChannel netdev directly. VPP's BVI is a purely internal L3 endpoint with no kernel-facing punt path, so the original member ingress (and its wire VLAN tag) is lost once a frame is bridged/flooded.
DHCP must be trapped, not flooded. A DHCP client broadcast (dst=ff:ff:ff:ff:ff:ff) entering the BD would hit l2-flood and fan out to every other member port, which sonic-mgmt's DHCPBroadcastNotFloodedTest forbids — a real ASIC traps DHCP to the CPU and removes it from the forwarding pipeline (SAI_PACKET_ACTION_TRAP). Routing DHCP through the BVI/L3 path is also wrong: it can't disambiguate which bridge the discover came from, and the L3 path subjects DHCP to IP/UDP checksum validation that drops malformed frames the control plane is supposed to count.
This PR implements the VPP-VS data path for VLAN BVI plus the SAI/classify plumbing needed to punt control traffic to the correct member interface. Design details are in vlan-bvi-hld.md (sonic-buildimage).
Work item tracking
How did you do it?
VLAN BVI + members (SwitchVppFdb.cpp, SwitchVppRif.cpp, SwitchVppHostif.cpp): create a BVI per SVI and add it to the bridge domain. Tagged members join via a dot1q sub-interface with symmetric VTR (pop 1/push); untagged members join the BD directly. Explicit LCP pairs are created for sub-port RIFs (since lcp-auto-subint is disabled), and promiscuous mode is enabled on each phy so tagged frames reach VPP.
L2 classifier punt (SwitchVppFdb.cpp): attach l2-input-classify tables to each member that match LLDP and DHCPv4 client broadcasts before l2-flood, so they are trapped to the CPU instead of flooded (emulating SAI_PACKET_ACTION_TRAP). Untagged DHCP punts straight to linux-cp-punt; tagged DHCP goes through sonic-ext-l2-trap-fixup to reach the parent phy's host tap with its .1Q tag intact.
New VPP API wrappers (SaiVppXlate.c/.h): classify table create/delete, session add/del, set-interface-l2-tables, add-node-next, and set-promiscuous.
WR macro timeout change. The previous WR macro waited up to a fixed 1 second for a reply. Under bursts of unsolicited events (link state, counters, etc.) the reply-queue can be saturated long enough for that budget to elapse before our reply is drained, producing spurious -99 failures in unrelated call sites. The macro now uses a 10s hard cap with a 1s idle cap that resets on each processed message.
How did you verify/test it?
Verified on the sonic-vpp platform with sonic-mgmt DHCP relay tests (DHCP punted to the member tap, not flooded — DHCPBroadcastNotFloodedTest) and LLDP/ARP punt landing on the correct member netdev for both tagged and untagged members.
Any platform specific information?
Documentation
202605 backport validation
Back port request
Tested branch
Test result
2e297e93668c2a791fba6a590befa8eb15bb5ca91196355202605,t0-vpp, one KVMdhcp_relay/test_dhcp_relay.py::test_dhcp_relay_default[isc-relay-agent]passed. Module summary: 14 passed, 2 skipped; pretest: 9 passed, 3 skipped; posttest: 5 passed.