[vpp] CoPP dataplane enablement HLD - #2539
nhegde-microsoft wants to merge 8 commits into
Conversation
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
No pipelines are associated with this pull request. |
|
/azp run |
|
No pipelines are associated with this pull request. |
9a741ba to
7352b23
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
Multi-model review: SONiC #2539 (CoPP dataplane enablement HLD)I ran the CoPP series (this HLD plus sonic-platform-vpp #281 and sonic-sairedis #2070) through a three model consensus review and an adversarial follow up. The design reads well and the punt plus policer approach is sound. My comments here are about accuracy between the HLD and what the two code PRs actually do, so the doc does not over promise. 1. REQ-5 (policer statistics) is not met by the current implementationREQ-5 implies the SAI policer stat counters are populated. In the implementation, the #281 data path meters with the raw 2. REQ-2 trap scope is broader than what is bound in the dataplaneThe requirement text lists a broad set of control plane protocols (for example SNMP, SSH, and others), but the implementation binds a policer for only the five ethertype based traps that the plugin keys on (ARP, LACP, LLDP, UDLD, and TTL error / IP2ME style punt). It would help readers to state explicitly which traps get dataplane policing in this phase and which are out of scope, so the HLD scope matches the shipped 3. The validation status table claims passes that the code PR calls not yet exercisedThe status table marks cases such as the trap config save after reboot case and the policer MTU (BGP) case as passing, but the #2070 PR description says those paths are not yet exercised. Please reconcile the two so the HLD status reflects the actual test coverage. 4. CosmeticThe added markdown file is committed with mode 100755 (executable). Doc files are normally 100644. None of these block the design discussion. They are about keeping the HLD claims aligned with #281 and #2070 so REQ-5 in particular is not marked complete while the policer stats read all zeros. This HLD can land any time once the REQ-5 and REQ-2 scope wording is reconciled. Reviewed by AI agent on behalf of aaronber0614. |
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Strong HLD overall — above the median for this repo. It does the three things design docs usually skip: (1) states why VPP's stock policer-classify can't cover this traffic with precise arc-level detail (ARP/LACP/LLDP/UDLD on linux-cp-paired L3 ports go ethernet-input → arp-input/linux-cp-punt-xc → TAP and never touch the l2-input/ip4-unicast/ip6-unicast arcs where policer-classify lives — correct, and it's the real justification for a new plugin); (2) documents alternates that were built and disproven (the tc ingress-qdisc-ordering fact — AF_PACKET copy happens before the ingress qdisc — is a real, subtle reason that approach fails); (3) carries a concrete pass/fail validation table. REQ-1..REQ-8 are well-formed and testable, and REQ-8 is unusually honest about testbed/harness blockers. Nice work.
Four technical questions I'd want answered before design-approval (inline), plus doc hygiene below. Flagging as Comment, not blocking — these are "clarify the design," not "the design is wrong."
Doc hygiene:
- Revisions table author is "Vesper" while the work is under nhegde-microsoft — codename/placeholder? Merged HLDs normally carry the real handle.
- Background says the ask is for
t1-lagtopology, but the testbed ist1-lag-vpp/vms-kvm-vpp-t1-lag— drop of the-vppsuffix. - REQ-2 lists SNMP/SSH in its trap enumeration, but neither the in-scope table nor the Status table covers them — are they in scope for this effort or aspirational? The "etc." is doing a lot of work in a requirement.
- REQ-4 calls out runtime add/remove (
test_add_new_trap/test_remove_trap) but onlytest_trap_config_save_after_reboot(REQ-6) appears in the Status table — is the runtime add/remove path passing too? - Consider a one-line VPP node-graph fragment (
device-input → copp_punt_policer → {police → interface-output(TAP) | drop}vs the bypassedethernet-input → arp-input/linux-cp-punt-xc) — it would make the whole design legible at a glance, and HLDs here usually carry one.
|
Thanks for the thorough review — addressed all of this in eeffa10, replies inline on the four technical questions. Doc hygiene:
|
|
/azp run |
|
No pipelines are associated with this pull request. |
Documents the copp_punt_policer VPP device-input plugin design that implements SAI POLICER/HOSTIF_TRAP/HOSTIF_TRAP_GROUP dataplane enforcement for control-plane protocols (ARP, LACP, LLDP, UDLD, TTL_ERROR) on SONiC-VPP. All CoPP policer tests pass on vlab-vpp-01 with this design (test_verify_copp_configuration_cli plus test_policer for ARP/LACP/LLDP/UDLD/Default/DHCP/DHCP6). Companion code: sonic-sairedis and sonic-platform-vpp branch copp-vpp-enablement. Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
…cer_mtu[BGP] pass Also documents the known, unresolved test_policer_mtu[IP2ME/SNMP/SSH] failure (zero packets received) as a separate, pre-existing gap unrelated to the copp_punt_policer plugin. Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
- Fix author handle (nhegde-microsoft, not placeholder), t1-lag-vpp
topology name in Background.
- REQ-2: clarify SNMP/SSH are pre-existing (ip4/ip6-unicast path),
not part of this effort's plugin scope.
- REQ-4: mark test_add_new_trap/test_remove_trap as not yet
validated in both the requirement and Status table.
- Add a node-graph fragment contrasting the bypassed
arp-input/linux-cp-punt-xc path with the new
copp_punt_policer -> police -> {TAP|drop} path.
- Add explicit design notes: tagged/VLAN frames are out of scope
(fixed 14-byte parse assumes untagged Ethernet); metering is
pps-based so the fixed 256-byte token-bucket reference length is
deliberately packet-size-independent (reconciled with REQ-5's
byte counters); the node consumes matched packets so VPP's
arp-input ARP-learning side effect is lost for punted ARP
(expected to happen at the CPU/Linux TAP side instead); fast-path
cost for non-punted traffic is a 14-byte header read plus a
linear scan of a small bounded (<=16 entry) table.
Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
- Remove the two 'not yet validated' test_add_new_trap/test_remove_trap rows from the Status table (and matching REQ-4 'see Status' pointer) -- per standing instruction, the Status table only carries confirmed passes and legitimate skips-with-reason, never in-progress or failing items. - Add a 'Legitimately skipped' subsection documenting test_trap_neighbor_miss: gated to T0-family topologies only in the generic (non-VPP) conditional-mark file, and this testbed (vms-kvm-vpp-t1-lag) is T1, so the skip is topology-driven and unrelated to asic_type or this effort's CoPP dataplane work. Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Per feedback, use the same table/format as the other Status rows instead of a separate subsection: SKIPPED with a green check and a short (t0 only) note, consistent with how the rest of the table reads. Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
ac49392 to
89c010c
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
REQ-4 validated live; Status table + Key files changed updated to reflect the dynamic trap add/remove tests passing, plus the sairedis reply-handler fix and sonic-mgmt test-harness asic_type fix that unblocked them. Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
Signed-off-by: Nikhil Hegde <nikhilhegde@microsoft.com>
89c010c to
f63b727
Compare
|
/azp run |
|
No pipelines are associated with this pull request. |
nhegde-microsoft
changed the title
|
|
||
| ## Background | ||
|
|
||
| [sonic-buildimage#25801](https://github.com/sonic-net/sonic-buildimage/issues/25801) asks to enable Control Plane Policing (CoPP) testing for the `t1-lag-vpp` topology (`vms-kvm-vpp-t1-lag` testbed) on the SONiC-VPP KVM testbed. |
There was a problem hiding this comment.
I think the design should not limit the scope to t1-lag topology. We should at least include t0 to make sure it is compatible. we don't want to redesign it to work for both t1 and t0
There was a problem hiding this comment.
Fair point, I did not make it generic enough to cover t0. I believe the primary difference is the VLAN/bridge setup in t0 versus the L3 routed ports in t1-lag. t0 still uses the same linux-cp tap pairing, so I think this should work just fine, but I'd like to run a quick test first. I will update here and in the document when done. Thanks for the suggestion.
| 2. **Police**: applies VPP's existing `vnet_police_packet()` token-bucket primitive against the same VPP policer object `SwitchVppPolicer.cpp` already creates from SAI `POLICER` attributes — no new metering implementation. Uses a fixed 256-byte reference packet length since VPP's pps→token-bucket conversion assumes that fixed size internally. | ||
| 3. **Deliver**: for a conforming packet, sets the buffer's TX interface directly to the mapped linux-cp TAP and dispatches straight to `interface-output`. Resolves against the raw ingress port for every ethertype uniformly, matching what this project's PTF test harness (`ptf_nn_agent`) actually observes. | ||
|
|
||
| SAI wiring (`SwitchVppHostifTrap.cpp`) mirrors the existing per-trap dispatch pattern used for bookkeeping: on `createHostifTrap`/`setHostifTrap`/`setHostifTrapGroup`, resolve the trap's bound policer to its VPP policer name and call the plugin's `copp_punt_policer_bind` API — switch-wide, since the plugin auto-enables its feature on every interface as it's created (`VNET_SW_INTERFACE_ADD_DEL_FUNCTION`), no per-port bind needed. |
There was a problem hiding this comment.
I have concern with adding this node to device input, which will affect every packets. Most of the protocols listed (except UDLD and TTL expiry requiring investigation) are already punted to control plane today. Can we add the policer to the punt path? For example, in the interface output arc of tap interface?
There was a problem hiding this comment.
I did have the same concern but it seemed like the best option while I was trying this out. I also ran some tests and found the added delay was ~50ns per packet and the protocol match list was small (<16). The advantage was that this would work for any topology.
Having said that, your other point about policing it in the output path is interesting because that is the first option I tried and ran into issues. However, after several other changes, I suspect the issue was not related to the location of the policer because I just ran it again now with the policer in tap interface output and it seems to work, which I am surprised to see myself. I am going to do a couple of more runs to be sure, but if it is stable, I will switch the design to use this instead. Should be a small change.
| | Rev | Date | Author(s) | Changes | | ||
| |-----|------|-----------|---------| | ||
| | 1.0 | 2026-09-02 | nhegde-microsoft | Initial HLD for `copp_punt_policer` design. | | ||
| | 1.1 | 2026-09-10 | nhegde-microsoft | Added `copp_ip2me_policer` plugin (IP2ME/SNMP/SSH enforcement on `ip4-punt`); updated status table. | |
There was a problem hiding this comment.
so far sonic-ext plugin is used for multiple features for sonic. Please consider if it is better to use the same plugin. The benefit is we can avoid too many plugins. But we lose the flexibility to enable/disable a feature through plugin infra. Currently, we use API in the sonic-ext plugin to enable/disable individual feature.
There was a problem hiding this comment.
Fair point. Let me look into feasibility of moving this into sonic-ext.
5 participants
Why I did it
Adds a High-Level Design document for CoPP (Control Plane Policing) dataplane enablement on SONiC-VPP.
sonic-buildimage#25801 asks for CoPP test coverage on the SONiC-VPP KVM testbed (
t1-lag-vpp). The SAI config-plane path already worked (orchagent'sCoppOrchissues normal SAI calls, accepted and stored bysaivpp), but no VPP dataplane mechanism actually classified, rate-limited, or punted control-plane traffic to the CPU. This HLD documents the design that closes that gap: two new VPP core plugins —copp_punt_policer(device-inputarc, ethertype-keyed traps) andcopp_ip2me_policer(ip4-puntarc, IP2ME/SNMP/SSH).Companion PRs
POLICER/HOSTIF_TRAP/HOSTIF_TRAP_GROUPwiring — [vpp] SAI support for CoPP dataplane enforcement sonic-sairedis#2070Test result
Manually validated on
vlab-vpp-01(t1-lag-vpptestbed): all 24tests/copp/test_copp.pysubtests pass, zero skips. Full details in the HLD's Status section.