Skip to content

[vpp] SAI support for CoPP dataplane enforcement - #2070

Open
nhegde-microsoft wants to merge 4 commits into
sonic-net:masterfrom
nhegde-microsoft:copp-vpp-enablement
Open

nhegde-microsoft wants to merge 4 commits into
sonic-net:masterfrom
nhegde-microsoft:copp-vpp-enablement

Conversation

@nhegde-microsoft

@nhegde-microsoft nhegde-microsoft commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

What

Adds SAI POLICER, HOSTIF_TRAP, HOSTIF_TRAP_GROUP support to vslib/vpp, backed by real VPP policer objects and two VPP plugins (companion sonic-platform-vpp PR): copp_punt_policer (ethertype-keyed) and copp_ip2me_policer (IP2ME/SNMP/SSH).

Why

sonic-buildimage#25801. These object types previously fell through to a generic attribute store with no VPP dataplane wiring — CoPP config was accepted but never enforced.

How

  • SwitchVppPolicer.cpp/.h (new): SAI POLICER → VPP native policer_add/policer_update/policer_dump.
  • SwitchVppHostifTrap.cpp (new): per-trap ethertype/TTL match table; binds copp_punt_policer. IP2ME (also covers SNMP/SSH) binds copp_ip2me_policer instead, since it has no ethertype. Also fixes a default-trap-group binding bug (affected TTL_ERROR).
  • SwitchVppRif.cpp: router-interface IPv4 add/remove syncs copp_ip2me_policer's tracked-address set.
  • SwitchVpp.cpp/.h: dispatch wiring for the 3 new object types.
  • vppxlate/SaiVppXlate.c/.h: VAPI wrappers for both plugins.

An earlier ip4-policer-classify-based IP2ME approach (per-interface bind) was replaced after proving fragile — see HLD.

Design: https://github.com/nhegde-microsoft/SONiC/blob/copp-vpp-enablement/doc/vpp/vpp_copp_HLD.md

Suggested merge order: sonic-platform-vpp#281 → this PR.

Companion PRs

Test

vlab-vpp-01: all 24 tests/copp/test_copp.py subtests pass, zero skips.

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@nhegde-microsoft
nhegde-microsoft marked this pull request as ready for review September 3, 2026 21:44
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@aaronber0614

Copy link
Copy Markdown
Contributor

Multi-model review: sonic-sairedis #2070 (CoPP dataplane enforcement)

I ran this through a three model consensus review (Claude Opus, GPT, Grok, all at high reasoning) and then a second adversarial pass to disprove each finding. Everything below is grounded against the pinned VPP revision 3f9e978d (policer at src/plugins/policer/) and the base sonic-sairedis tree. The dispatch and trap wiring are solid overall. The red Build/BuildAsan is expected: it fails on copp_punt_policer.api_enum.h: No such file or directory, which is generated by #281 and is not yet on platform-vpp master. That is a cross repo build ordering dependency, not a code defect, so merge order is #281 first, master rebuild, then this PR.

Blocker 1: SAI policer stats return 0 (HLD REQ-5 gap)

getPolicerStats calls vpp_policer_get_counters, which sums the VPP stats segment counters /net/policer/{conform,exceed,violate}. In the pinned VPP those counters are incremented ONLY by the policer_police() wrapper (src/plugins/policer/police_inlines.h). The #281 data path meters with the raw vnet_police_packet(...), which never touches them, so SAI_POLICER_STAT_GREEN/YELLOW/RED_PACKETS/BYTES come back 0 for ARP, LACP, LLDP, UDLD, and TTL_ERROR. The #281 plugin keeps its own per ethertype counters, but it exposes them through a separate API that getPolicerStats does not read. test_policer does not catch this because it measures PTF rx_pps, not SAI stats. Fix: either have the #281 node call policer_police(), or point getPolicerStats at the plugin's own per ethertype counters. This is the concrete gap behind HLD REQ-5.

Blocker (dependency): this PR aborts syncd unless #281 also enables policer_plugin.so

get_base_msg_id() runs policer_msg_id_base = vl_client_get_first_plugin_msg_id("policer_...") followed by assert(policer_msg_id_base != (u16)~0). In the pinned VPP, policer is a loadable policer_plugin.so (add_vpp_plugin(policer ...)), and #281's startup.conf.tmpl (which uses plugin default { disable }) never enables it. On a clean deploy that assert fires and aborts syncd at connect time. I filed the fix on #281 (add plugin policer_plugin.so { enable }). Flagging it here too because this PR is the code that trips it. Note that the sibling classify msg id lookup added in the same commit is safe, because classify is vnet core; it is specifically policer's plugin form that is the trap.

High priority (real bug, but not on the CoPP path shipped here): setPolicer never reaches the dataplane

setPolicer ends with return createPolicer(...). createPolicer's first line is CHECK_STATUS(create_internal(SAI_OBJECT_TYPE_POLICER, ...)). On an already existing OID, create_internal returns SAI_STATUS_ITEM_ALREADY_EXISTS (SwitchStateBase.cpp), and CHECK_STATUS early returns (SwitchStateBase.h), so vpp_policer_add_replace is never reached and the SET returns a hard error to orchagent. Even if it were reached, the wrapper named vpp_policer_add_replace actually emits POLICER_ADD (via del plus add) even for is_replace=true, and does not use VPP's real policer_add_replace or policer_update API. The in code comment claiming an in place recreate via add_replace(is_replace=true) is therefore inaccurate on both counts.

The adversarial pass confirmed this is NOT reached by the committed CoPP path (CoPP programs policers via config rewrite plus swss restart, which is CREATE, not runtime SET), so it does not block the CoPP feature itself. But any runtime CIR/CBS update on a policer would fail, so I would fix it before it bites a later feature. Fix: in setPolicer, call vpp_policer_add_replace(is_replace=true) on the tracked index directly, or make createPolicer tolerate ALREADY_EXISTS, and emit VPP's real add_replace/update message.

Note (real, non blocking): shared ethertype unbind wipes the sibling ARP trap

SAI_HOSTIF_TRAP_TYPE_ARP_REQUEST and ARP_RESPONSE both map to ethertype 0x0806 in SwitchVppHostifTrap.cpp. The plugin binding is keyed by ethertype with a single slot, and unbind memsets that slot with no refcount. Removing or disabling one ARP trap therefore clears the shared 0x0806 slot and unpolices the other ARP trap (still delivered, just unpoliced). This is not hit by the default CoPP profile (both ARP traps stay installed), so it is non blocking, but worth a refcount per ethertype or keying by trap OID.

Minor (real, non blocking): trap moved between groups keeps its old policer

In setHostifTrap, the install and uninstall branches are gated on classify_installed transitions, so an already installed trap that stays installed while its TRAP_GROUP changes hits neither branch and keeps its old policer binding. setHostifTrapGroup does correctly rebind a group's traps when the group's POLICER attribute changes, so the common CoPP path (change the group's policer) works. Only moving a trap between groups is affected, which is rare and not in the committed tests. Low priority: in the TRAP_GROUP case of setHostifTrap, re-run the classify install with the new group's policer.

Confirmed correct (I checked these)

Removing the old sflow HOSTIF_TRAP dispatch is not a regression. The old sflowHostifTrapSamplePacketCreate/Remove handlers were bookkeeping only (create_internal/remove_internal plus a NOTICE), and the new createHostifTrap/removeHostifTrap call the same create_internal/remove_internal, so behavior is preserved.

Summary

Blocker 1 (stats/REQ-5) and the #281 dependency should be resolved before merge. The setPolicer issue is a real bug but not on the CoPP path, so I would treat it as high priority rather than a CoPP merge blocker. The two ethertype/trap group items are non blocking notes. Merge after #281 lands and platform-vpp master rebuilds.

Reviewed by AI agent on behalf of aaronber0614.

@nhegde-microsoft

Copy link
Copy Markdown
Contributor Author

@aaronber0614

Thanks — all three check out, fixed.

setPolicer never reaching the dataplane: Fixed. Extracted the VPP-programming logic into a shared programPolicer(object_id, attrs, is_replace) that doesn't call create_internal/set_internal itself. createPolicer and setPolicer each do their own internal-store call, then invoke it — so setPolicer no longer routes through createPolicer's ITEM_ALREADY_EXISTS short-circuit. Also fixed is_replace (was hardcoded false) and had it look up the existing VPP policer index so the delete-then-recreate lands at the right slot.

Shared ARP ethertype unbind: Fixed. uninstallTrapClassify now scans the tracked trap map for any other still-installed trap on the same ethertype before unbinding; only unbinds when nothing else needs it.

Trap moved between groups: Fixed. setHostifTrap now re-runs the classify install with the new group's policer when the trap stays installed but its TRAP_GROUP changes.

All three verified via a real build (-Wall -Wextra -Werror) — clean.

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Add SAI POLICER, HOSTIF_TRAP, and HOSTIF_TRAP_GROUP support backed by
VPP native policer objects and the copp_punt_policer device-input
plugin. Control-plane traffic (ARP, LACP, LLDP, UDLD, TTL_ERROR) is
classified and rate-limited.

Also fixes a bug in setHostifTrapGroup() where the switch's implicit
default trap group was never tracked, silently dropping its policer
binding.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
…up move

- SwitchVppPolicer.cpp: extract programPolicer() (the VPP-side work)
  out of createPolicer(), shared with setPolicer() instead of routing
  through createPolicer()'s create_internal() call, which always
  returned ITEM_ALREADY_EXISTS on an existing OID and silently
  short-circuited every runtime SET before it ever reached VPP. Also
  fixes is_replace (was hardcoded false) and looks up the existing VPP
  policer_index so the replace path's delete-then-recreate lands at
  the correct slot.

- SwitchVppPolicer.cpp: vpp_policer_mode_from_sai()'s SR_TCM (RFC 2697,
  1R3C) branch now discriminates on has_pbs instead of has_pir --
  PIR/EIR belongs to the two-rate TR_TCM modes, not single-rate SR_TCM.

- SwitchVppHostifTrap.cpp: uninstallTrapClassify() now checks whether
  any other tracked, classify_installed trap still needs the same
  ethertype before actually unbinding it (the device-input plugin's
  bind table has one slot per ethertype, and SAI_HOSTIF_TRAP_TYPE_
  ARP_REQUEST/_RESPONSE both map to 0x0806, so removing one trap could
  otherwise silently unpolice the other).

- SwitchVppHostifTrap.cpp: setHostifTrap() now re-runs the classify
  install with the new trap group's policer when a trap stays
  installed but its TRAP_GROUP attribute changes -- previously this
  transition hit neither the install nor uninstall branch, so the trap
  silently kept its old group's policer binding.

All changes verified via a real build (-Wall -Wextra -Werror), clean.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Fixes syncd watchdog hangs: policer programming and
installTrapClassify/uninstallTrapClassify now use the existing
deferred one-item-per-call queue pattern instead of calling VPP
synchronously. Also registers the missing
policer_classify_set_interface reply handler (msg id 425), which
VPP was replying to but the client silently dropped, causing an
infinite wait.

Confirmed live: testbed fully converged (24/24 BGP, 8/8 LACP),
unblocks test_add_new_trap/test_remove_trap.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Replace the ip4-policer-classify approach (per-interface binding,
deferred-queue bookkeeping) with calls into the new copp_ip2me_policer
plugin, which polices on the global ip4-punt arc instead -- no
per-interface binding needed.

Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
@mssonicbld

Copy link
Copy Markdown
Collaborator

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

@nhegde-microsoft nhegde-microsoft changed the title [vpp] Implement CoPP dataplane enforcement via copp_punt_policer plugin [vpp] Implement CoPP dataplane enforcement via copp_punt_policer and copp_ip2me_policer plugins Sep 10, 2026
@nhegde-microsoft nhegde-microsoft changed the title [vpp] Implement CoPP dataplane enforcement via copp_punt_policer and copp_ip2me_policer plugins [vpp] Implement SAI support for CoPP dataplane enforcement Sep 10, 2026
@nhegde-microsoft nhegde-microsoft changed the title [vpp] Implement SAI support for CoPP dataplane enforcement [vpp] SAI support for CoPP dataplane enforcement Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants