vpp: support punting ARP, LLDP, LACP, DHCP via member interface in VLAN - #1959
yue-fred-gao wants to merge 9 commits into
Conversation
Signed-off-by: Yue Gao <yuega2@cisco.com>
Signed-off-by: Yue Gao <yuega2@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Yue Gao <yuega2@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Yue Gao <yuega2@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Signed-off-by: Yue Gao <yuega2@cisco.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Review: #1959
Read the full SaiVppXlate.c and SwitchVppFdb.cpp, not just the diff. This is strong, well-designed work — the ~90-line header comment in SwitchVppFdb.cpp explaining classifier slot-selection (IP4 vs OTHER by outer ethertype), the tagged/untagged offset math, and why LACP and tagged-LLDP bypass the BD via linux-cp-punt-xc is genuinely excellent and rare to see. Teardown paths mirror setup. A few real issues inline, one of which is a concrete latent bug.
Highest-value item (#1, inline): three of the new reply handlers deref get_index_ptr(msg->context) without a NULL check, but that function returns NULL on a stale/duplicate reply (generation mismatch). The same PR's create_subif handler does guard it — so the pattern is known, just omitted in three places. And it's made more reachable by #2.
The one I'd most like your eyes on (#2, inline): the WR macro rewrite (1s→10s + idle-deadline) is reasonable on its own, but it silently changes the reply-wait behavior for all 72 VPP API call sites in the file — every bfd/acl/route/tunnel call, not just the new classify ones — and raises the worst-case blocking time under VPP_LOCK from ~1s to 10s. That's a cross-cutting operational change buried in a VLAN-BVI feature PR; it may deserve its own PR or at least an explicit call-out + sign-off. Worth confirming vl_socket_client_read(1) == 0 reliably means "one message processed" and can't refresh the idle deadline on a spurious wakeup.
Positives worth acknowledging: the sw_if_index cache insert/evict in create_sub_interface/delete_sub_interface (avoids a full sw_interface_dump on each LCP resolve) is a good improvement, and interface_set_promiscuous is correctly paired with the tagged-frame requirement. Nothing here undermines the core approach — the punt design is sound.
| set_reply_status(retval); | ||
|
|
||
| uint32_t *table_index = (uint32_t *) get_index_ptr(msg->context); | ||
| *table_index = ntohl(msg->new_table_index); |
There was a problem hiding this comment.
🟠 Null-deref on a stale/duplicate reply. get_index_ptr() returns NULL when the context index is invalid or the generation doesn't match (lines 626-632) — i.e. a late/duplicate reply for an already-released context. This handler (and the two add_node_next / get_next_index handlers just below, lines ~1484 and ~1497) deref the result unconditionally:
uint32_t *table_index = (uint32_t *) get_index_ptr(msg->context);
*table_index = ntohl(msg->new_table_index); // NULL deref if staleThe create_subif handler this same PR added (~914) does it correctly: if (msg->context) { ...; if (swif_idx) {...} }. Please add the same guard to all three:
uint32_t *table_index = (uint32_t *) get_index_ptr(msg->context);
if (table_index) *table_index = ntohl(msg->new_table_index);
release_index(msg->context);This is more reachable because of the WR-macro change below — tolerating event bursts over a 10s window raises the odds of processing exactly the kind of stale reply that yields NULL here.
| * - The loop breaks when we get the expected reply (vam->result_ready == 1), the 10 second hard | ||
| * cap is reached, or 1 second elapses with no new message processed. | ||
| */ | ||
| #define WR(ret) \ |
There was a problem hiding this comment.
VPP_LOCK, that serializes every other API caller behind the stuck one for up to 10s. Consider splitting this into its own PR (or at minimum calling it out explicitly in the description for a dedicated sign-off). Also worth confirming: does vl_socket_client_read(1) return 0 only when a message was actually processed? If it can return 0 on a spurious/timeout wakeup, the idle deadline refreshes without progress and every call effectively waits the full 10s hard cap.
There was a problem hiding this comment.
Updated PR description in #1981 to capture this change
| return ret; | ||
| } | ||
|
|
||
| int vpp_classify_table_delete(uint32_t table_index) |
There was a problem hiding this comment.
🟡 Dead code: vpp_classify_table_delete (here) and vpp_classify_session_del (~4473) are defined and exported in the header but never called anywhere. The classify tables created in l2_punt_classify_init are process-lifetime; l2_punt_classify_remove only detaches tables from the interface (set_interface_l2_tables(~0,~0,~0)), it never deletes them. Either wire these into a real teardown path or drop them + their header decls. If the process-lifetime/shared-table design is intentional (it's defensible — the tables are shared across all members), a one-line comment at l2_punt_classify_init saying so would stop the next reader from "fixing" the apparently-missing cleanup.
There was a problem hiding this comment.
Added for symmetry of the add/del pair and to avoid a follow-up PR when needed.
| * LCP host tap. Tagged frames still carry the 802.1Q header | ||
| * when l2-input-classify runs (VTR has not yet stripped it). | ||
| */ | ||
| l2_punt_classify_apply(hw_ifname, true /*tagged*/); |
There was a problem hiding this comment.
🟡 Return value ignored — a member can come up "successfully" with no control-plane punt. l2_punt_classify_apply can fail (init failure, table-create failure, or set_interface_l2_tables failure — all logged ERROR internally), but the result is dropped here and at the untagged call site (~566), and vpp_create_vlan_member returns SAI_STATUS_SUCCESS regardless. So a VLAN member silently comes up with DHCP/LLDP unpunted — precisely the condition DHCPBroadcastNotFloodedTest guards, but only surfacing after a silent partial config. Please at least propagate/log-fail, or add a comment stating best-effort punt is acceptable on the VS platform and why.
There was a problem hiding this comment.
Fixed as best-effort. Both sites now SWSS_LOG_WARN on non-zero return so the partial config is visible in syslog, comment added noting the best-effort intent.
| #define SAIVS_DHCP_BOOTPC 68 | ||
| #define SAIVS_DHCP_BOOTPS 67 | ||
|
|
||
| static int l2_punt_classify_init() |
There was a problem hiding this comment.
🟡 Unsynchronized lazy init — worth a one-line assumption comment. l2_punt_classify_init mutates ~7 file-static globals (s_l2_punt_classify_inited, s_*_table, s_*_next_index) with no locking. This is almost certainly fine because SAI object creation in syncd is serialized on the single processing thread — but the classify wrappers it calls all take VPP_LOCK, so a reader could reasonably assume the init needs locking too. A single line like // Called only from the single SAI processing thread; the s_* globals need no locking. would make the invariant explicit and prevent a future "add a mutex" churn or a misplaced concurrent call.
|
Hi @dypet , could you please follow up/fork and create new pr based on that during Fred's OOF, since the vlan is a big block of T0 and some features of T1-lag |
|
@yejianquan @lolyu re-opened here: #1981. Responded to existing review comments and addressed in new PR. |
…AN (re-opened) (#1981) Description of PR Summary: Fixes #26936 Opening PR for #1959 while original owner is out of office. Type of change Bug fix New feature Refactor / cleanup Documentation update Test improvement 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 Microsoft ADO (number only): 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. Signed-off-by: dypet <dypeters@cisco.com>
|
Closing, merged with: #1981 |
5 participants
Description of PR
Summary:
Fixes # (issue)
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.
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