[vpp][acl] do not count policy denies as interface drops

[lolyu]

What

An ACL deny is an intentional forwarding decision, not an interface discard, but
today the deny path sends the buffer to error-drop, which charges it to the
ingress interface's drop counter (/if/drops). SONiC exports that counter as
SAI_PORT_STAT_IF_IN_DISCARDS and displays it as RX_DRP, so a port doing
exactly what it was configured to do reports a large and growing discard count.

This adds VNET_BUFFER_F_POLICY_DROP and stops charging policy denies to the
interface drop counter, while keeping them fully visible as per-node/per-ACE
errors.

Why

This is most visible on a dual-ToR standby mux port, where the standby ToR is
supposed to drop the traffic it receives from the server. The port reports a
~99% RX_DRP rate while forwarding perfectly correctly:

Port Ethernet16 on standby ToR vlab-vpp-04 has RX_DRP increment of 5706
which exceeds threshold of 10 (before=0, after=5706)

Correction (2026-09-04). An earlier version of this description claimed
"SONiC itself already treats ACL drops as RX_ERR, not RX_DRP", and leaned on
RFC 1213 as if it mandated the change. Both were wrong, and the section below
has been rewritten. Thanks to @nhegde-microsoft and @aaronber0614 for catching it.
The corrected justification is stronger, not weaker — see
this comment
for the discussion.

The counter semantics are the problem, not the drops themselves.

Where SONiC expects an ACL drop to be counted. Upstream sonic-mgmt states
this directly. For discard_group == "ACL", tests/drop_packets/test_drop_counters.py
asserts the per-ACE ACL rule counter increments, and then — on any platform not
listed as having combined counters — asserts that neither RX_DRP nor
RX_ERR moved:

if acl_drops != pkt_number:
    pytest.fail(...)                      # per-ACE counter MUST increment
if not COMBINED_ACL_DROP_COUNTER:
    executor.submit(ensure_no_l3_and_l2_drops, duthost, packets_count=pkt_number)

So an ACL drop belongs in the per-ACE counter — not RX_ERR, and not RX_DRP.

VPP is already declared as such a platform. tests/drop_packets/combined_drop_counters.yml
lists the platforms where ACL and L2 drop counters legitimately are combined:

acl_l2:
    - "x86_64-mlnx"
    - "x86_64-dell.*"
    - "x86_64-arista.*"
    - "x86_64-cel_seastone.*"
    - "x86_64-nokia.*"

x86_64-kvm_x86_64-r0 (VPP) appears in that file's l2_l3 list but deliberately
not in acl_l2 — added by @AkeelAli in
sonic-mgmt#24031
("Enable test_drop_counters.py on Sonic-VPP"). Upstream has therefore already
declared that on VPP an ACL drop must not land in the L2/RX_DRP counter. Today
VPP contradicts that declaration; this patch makes it conform.

And the dual-ToR case was called out explicitly. The test that fails here was
added by a third party in
sonic-mgmt#23577, whose
description reads:

In case of active-standby configuration, an DROP ACL is attached to the standby
port. This ACL drop is intentional and should not raise the RX_DROPS. RX_DROP
count increment can raise alarms in alerting systems.

On RFC 1213. For completeness, the RFC does not settle this. ifInDiscards
is "the number of inbound packets which were chosen to be discarded even though no
errors had been detected... One possible reason ... could be to free up
buffer space" — buffer exhaustion is an example, not the definition. A strict
reading leaves RX_DRP defensible for a policy drop. The justification for this
change is the SONiC-level convention above, not the RFC.

How

Three small changes:

1. src/vnet/buffer.h — allocate VNET_BUFFER_F_POLICY_DROP on bit 19, the
   first free bit. Per the in-tree convention comment, AVAIL1..AVAIL9 are
   redefined as AVAIL1..AVAIL8 and VNET_BUFFER_FLAGS_ALL_AVAIL is updated.
2. src/plugins/acl/dataplane_node.c — set the flag at the single ACL deny
   choke point in acl_fa_inner_node_fn().
3. src/vnet/interface_output.c — interface_drop_punt() discounts flagged
   buffers from the interface drop counter. It reuses the existing per-buffer
   loop added by patch 0008, so there is no new pass over the frame.

The flag is set only for ACL_FA_ERROR_ACL_DROP. Drops caused by a lack of
resources, such as ACL_FA_ERROR_ACL_TOO_MANY_SESSIONS, are genuine discards and
continue to increment the interface counter.

Denies remain fully observable — this suppresses one counter, not the visibility:

11398   acl-plugin-in-ip4-l2    ACL deny packets    error

Verification

Built as VPP 2606-0.6 and deployed to a KVM dual-ToR testbed
(vms-kvm-dual-vpp-t0-1, vlab-vpp-03/vlab-vpp-04).

dualtor_io/test_normal_op.py::test_upstream_standby_rx_drop_check passes:

test_normal_op.py:467  Port Ethernet40 on standby ToR vlab-vpp-04:
                       RX_DRP diff=0 (before=0, after=0), threshold=10

Raw port counters on the standby mux port confirm the mechanism rather than just
the outcome — 5762 packets received and denied, none counted as interface drops:

	Before	After
RX_OK	5706	5,762
RX_DRP	+5706	0

The full patch series (0001–0017 plus this one) was also verified to apply
cleanly with git apply against pinned VPP commit 3f9e978d.

On the version label. The testbed run above was built as 2606-0.6. The
branch now sets 2606-0.7, because #278 merged first and published 0.6 for
patch 0018. The patch content is byte-identical to what was tested — only
series and rules/vpp.mk changed in the rebase — so no re-test is implied.

Notes for reviewers

• Scope of the suppression. This applies to all ACL denies, including
  operator-configured DATAACL rules, not just the dual-ToR case. That matches the
  upstream discard_group == "ACL" expectation above, which is not dual-ToR
  specific, and avoids the incoherent rule that a DATAACL deny would be an
  interface discard on a normal port but not on a mux port. It is still a
  deliberate semantic change and worth a second opinion.
• Nothing under-reports as a result. SONiC detects ACL drops via the ACL_ANY
DEBUG_COUNTER and the per-ACE acl_facts packets_count, both untouched here.
  I found no consumer of SAI_PORT_STAT_IF_IN_DISCARDS / RX_DRP in monit or
  sonic-swss-common that keys on ACL denies.
• Bit 19 reallocation touches a shared header. It follows the documented
  convention in buffer.h exactly, and avail9 is confirmed gone from the built
  libvnet.so.
• This was split out of [vpp][acl] match on ingress interface to support SAI ACL IN_PORTS #278 (ACL IN_PORTS support), where it originated. The
  two changes are independent — they touch disjoint files — and [vpp][acl] match on ingress interface to support SAI ACL IN_PORTS #278 will be
  rebased on top of whichever lands first.

[azure-pipelines]

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

[mssonicbld]

/azp run

[azure-pipelines]

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

[Copilot]

🟢 Approval recommended

The patch cleanly scopes the counter semantic change to explicit ACL policy denies via a dedicated buffer flag while leaving resource-drop paths and existing observability intact.

Pull request overview

This PR adjusts how VPP accounts ACL deny traffic so intentional policy drops don’t inflate interface drop counters exported to SONiC as RX_DRP (SAI_PORT_STAT_IF_IN_DISCARDS), while preserving visibility via existing per-node/per-ACE error counters.

Changes:

• Add a new VPP buffer flag (VNET_BUFFER_F_POLICY_DROP) and set it on the ACL deny path for true policy denies (not resource-exhaustion denies).
• Update interface_drop_punt() accounting to exclude flagged buffers from /if/drops (and still charge original member interfaces for rewritten RX sw_if_index where applicable).
• Bump the VPP package minor version to ensure downstream builds don’t reuse cached/stale debs after patch content changes.

File summaries

File	Description
vppbld/patches/series	Adds the new patch to the build patch series with rationale in comments.
vppbld/patches/0018-acl-do-not-count-policy-denies-as-interface-drops.patch	Implements VNET_BUFFER_F_POLICY_DROP, sets it for ACL policy denies, and discounts those buffers from interface drop counters.
rules/vpp.mk	Bumps VPP_VERSION from 2606-0.5 to 2606-0.6 to invalidate cached artifacts after patch updates.

Review details

• Files reviewed: 3/3 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[mssonicbld]

/azp run

[azure-pipelines]

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

[lolyu]

Renumbered this patch 0018 → 0019.

0018 is the next free slot after master's 0017, and it now goes to #278 (the IN_PORTS feature, which has an accompanying HLD). Having both PRs claim 0018 would have produced two differently-named 0018-*.patch files on the second merge — and because the filenames differ, git would add both without any conflict on the patch files themselves, leaving a duplicate number for someone to notice later. Distinct numbers avoid that.

The patch content is unchanged; this is the filename plus the series entry. The two patches touch disjoint files (dataplane_node.c / buffer.h / interface_output.c here, versus the acl plugin's acl.c, acl_types.api, hash_lookup.c, public_inlines.h and types.h in #278), so apply order doesn't matter and either PR can merge first.

Re-verified with git apply against pinned VPP commit 3f9e978d that this branch's series applies cleanly on its own, and that both patches apply together.

The VPP_VERSION note still stands: both PRs set 2606-0.6, git auto-resolves rules/vpp.mk silently because the edit is identical, so whoever merges second should hand-bump to 2606-0.7.

[mssonicbld]

/azp run

[azure-pipelines]

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

[aaronber0614]

Reviewed this against the pinned VPP source (commit 3f9e978d). The mechanism is correct and the targeting is precise. Nice, well-scoped fix. A few notes and one question.

What I verified

• The interface_output.c change is safe. interface_drop_punt recomputes off = frame->n_vectors - n_left and rebases sw_if_index at the top of every iteration and has no end-of-loop pointer advance, so the new if (0 == count) continue; cannot misalign a later block in a multi-interface frame. off here is exactly patch 0008's orig_off, so bufs[off + j] reads the right block.
• The flag targets exactly the intentional denies. b[0]->error = error_node->errors[action] on the new-packet path makes a deny (action 0) land on ACL_FA_ERROR_ACL_DROP, which you key on. The existing-session path keeps error ACL_EXIST_SESSION and only returns action 0 in the hash-collision bug case (the "drop the unlucky packet" branch), and TOO_MANY_SESSIONS keeps its own error, so both correctly stay as genuine interface discards. So this catches all policy denies and nothing that is really a resource/bug drop.
• A policy that flips to deny on an already-established flow is still covered: with reclassify_sessions, stale_session_deleted forces acl_check_needed back to 1 and re-runs the match, which lands on ACL_DROP again.
• The bit-19 reallocation follows the in-file convention (first free bit, AVAIL1..8, VNET_BUFFER_FLAGS_ALL_AVAIL updated), and no other patch in the series references an AVAIL bit, so the shared-header change looks clean.
• Visibility is preserved: you only gate the interface simple counter, so the per-node "ACL deny packets" error, the per-ACE "show acl-plugin acl" counters, and tracing are untouched.

Semantic scope (your flagged question)

• I agree this should apply to all ACL denies, not just the mux case. A per-port carve-out would put "is this a mux port" policy into the generic error-drop path and give the incoherent rule that a DATAACL deny is an interface discard on a normal port but not on a mux port. Keying on the ACL_DROP error is the cleaner invariant.
• I would frame the justification as a deliberate SONiC/VPP semantic rather than an RFC mandate: RFC 1213 lists freeing buffer space as one example reason for ifInDiscards, not the only one, but the direction lines up with it and with SONiC already mapping ACL drops to RX_ERR via drop_packets, and with dualtor_io/test_normal_op.py::test_upstream_standby_rx_drop_check expecting a healthy standby port at RX_DRP ~0, which this delivers.
• One confirmation worth a sentence in the PR: is anything on the SONiC side consuming SAI_PORT_STAT_IF_IN_DISCARDS or port RX_DRP to detect ACL denies (a drop monitor or telemetry alarm) that would now under-report? If not, the RX_ERR / per-ACE counters are the right place for it and this is strictly an improvement.

Minor

• The description says "Built as VPP 2606-0.6" but the PR bumps VPP_VERSION to 0.7. Worth confirming the tested binary is the same patch content, just relabeled, so no re-test is implied.

Reviewed by AI agent on behalf of aaronber0614.

[lolyu]

Tracking issue filed: sonic-net/sonic-buildimage#29335.

Also noting for reviewers that this branch has been rebased onto master since #278 merged. VPP_VERSION moved to 2606-0.7: patch 0018 from #278 already published 0.6, and since both branches had independently made the identical 0.5→0.6 bump, git merged rules/vpp.mk cleanly without flagging the collision — the suffix had to be advanced by hand. Leaving it at 0.6 would have made downstream sonic-buildimage builds pull the stale 0.6 debs that pre-date this patch.

Full series re-verified against the pinned VPP commit 3f9e978d: all 17 patches apply clean with 0018 and 0019 both present, and the two ACL patches touch disjoint files, so apply order is irrelevant. CI is green on both bookworm and trixie.

[nhegde-microsoft]

If I understand this correctly, the port is reporting RX_DRP correctly, as it should because the packet was dropped (as opposed to RX_ERR). The description states the issue is that "SONiC itself already treats ACL drops as RX_ERR, not RX_DRP". Is the problem here in how SONiC treats the drops and should that be fixed rather than have VPP not report any drops when it does drop packets due to ACL match?

[nhegde-microsoft]

As far as I understand the original code, it added the count for regular interfaces and then down below also added same count for a super interface (like a bond) if it exists. Are we double counting by adding this code for sub-interfaces?
Also, since we skip this code if this is a policy drop, is the intention here to do this for sub-interfaces only when this is not a policy drop?

[lolyu]

Good question, and the diff is genuinely misleading here — sorry. That loop isn't new; I moved it.

It comes from patch 0008-bond-drop-stats-track-original-member-interface.patch. You can see it already present in the tree with only 0008 applied:

$ git apply .../0008-bond-drop-stats-track-original-member-interface.patch
$ sed -n '1052,1065p' src/vnet/interface_output.c
      /* Also count against the original member interface if the
         RX sw_if_index was rewritten (e.g. by bond-input). */
      {
        u32 orig_off = frame->n_vectors - n_left - count;
        for (u32 j = 0; j < count; j++)
          ...
      }

The diff shows it as + at the top and - at the bottom because I merged it into my new loop, so the block is walked once instead of twice. Net effect on non-policy traffic: identical work, one pass.

On the double-counting concern specifically — these are two different mechanisms, and neither is about sub-interfaces in the same sense:

	what it charges	when it applies
sup_sw_if_index	sub-interface → its parent (Ethernet0.100 → Ethernet0)	when the RX interface is a sub-interface
orig_rx_sw_if_index	LAG member whose RX index was rewritten (BondEthernet0 → Ethernet0)	bond traffic only

orig_rx_sw_if_index is written in exactly one place — src/vnet/bonding/node.c, where bond-input rewrites sw_if_index[VLIB_RX] from the member to the bond — and is explicitly zeroed otherwise. It's also guarded by orig && orig != sw_if_index[0], so it can never charge the same interface twice.

The offsets are equivalent too: the original computed orig_off = frame->n_vectors - n_left - count after n_left -= count, which is the same value as my off = frame->n_vectors - n_left computed before the decrement.

is the intention here to do this for sub-interfaces only when this is not a policy drop?

Yes — intentional, and it applies to all three counters, not just this one:

• RX interface — via the reduced count
• super-interface — same reduced count
• original bond member — via the continue

If the bond member were still charged for a policy drop, a mux port that happened to be a LAG member would keep reporting RX_DRP and the fix wouldn't work there. Suppressing it in one place and not the others would be the inconsistent option.

[lolyu]

You're right to push on this, and you caught a real error in my PR description. Let me correct it, because the sentence you quoted was simply wrong.

The claim you quoted is wrong — I've retracted it

"SONiC itself already treats ACL drops as RX_ERR, not RX_DRP"

That is not what SONiC does, and I shouldn't have written it. What upstream actually expects is that an ACL drop increments neither counter. From tests/drop_packets/test_drop_counters.py, for discard_group == "ACL":

if acl_drops != pkt_number:
    pytest.fail(...)                      # per-ACE ACL counter MUST increment
if not COMBINED_ACL_DROP_COUNTER:
    executor.submit(ensure_no_l3_and_l2_drops, ...)   # RX_DRP and RX_ERR must NOT

So an ACL drop belongs in the per-ACE ACL counter, and nowhere else. I've rewritten that section of the description.

That correction doesn't change what the patch does — it only stops charging RX_DRP and leaves the per-ACE counters intact, which is exactly the shape above. It would have been wrong to move these to RX_ERR, and the patch doesn't.

And you're right about the RFC too

I overstated it. The actual text is:

"The number of inbound packets which were chosen to be discarded even though no errors had been detected... One possible reason for discarding such a packet could be to free up buffer space."

Buffer exhaustion is an example, not the definition. An ACL drop genuinely is "chosen to be discarded, no error detected" — so on a strict reading, your position is defensible and my "RFC says resource exhaustion only" framing was not. @aaronber0614 flagged the same thing. I've dropped that argument.

Why the fix still belongs in VPP, not SONiC

The real justification is a SONiC-level convention that already exists — and which already names VPP specifically.

tests/drop_packets/combined_drop_counters.yml lists the platforms where ACL and L2 drop counters legitimately are combined:

acl_l2:
    - "x86_64-mlnx"
    - "x86_64-dell.*"
    - "x86_64-arista.*"
    - "x86_64-cel_seastone.*"
    - "x86_64-nokia.*"

On those platforms, an ACL drop showing up in the L2 drop counter is expected and the test tolerates it. x86_64-kvm_x86_64-r0 — VPP — is not in that list. It's in the file's l2_l3 list but deliberately not acl_l2, added by @AkeelAli in sonic-mgmt#24031 ("Enable test_drop_counters.py on Sonic-VPP"), not by me.

So this isn't VPP inventing a semantic, and it isn't a SONiC-side bug to fix: upstream has already declared that on VPP an ACL drop must not land in the L2/RX_DRP counter, and VPP currently contradicts that declaration. This patch makes VPP conform to a contract that already names it.

The dual-ToR case was also called out explicitly and independently — sonic-mgmt#23577, which added the failing test, says:

In case of active-standby configuration, an DROP ACL is attached to the standby port. This ACL drop is intentional and should not raise the RX_DROPS. RX_DROP count increment can raise alarms in alerting systems.

That test carries no platform carve-out, so it applies to VPP as written.

On "VPP not reporting any drops"

Worth being clear that it still reports them — this only gates the interface discard counter. The denies remain fully visible where SONiC actually looks for them:

• per-ACE counters (show acl-plugin acl, and acl_facts packets_count, which the test above asserts on)
• the per-node error counter (show errors → acl-plugin-in-ip4-l2 / ACL deny packets)
• the ACL_ANY DEBUG_COUNTER path

And only ACL_FA_ERROR_ACL_DROP is flagged — ACL_FA_ERROR_ACL_TOO_MANY_SESSIONS and the other resource-exhaustion cases stay genuine interface discards, which is the distinction the counter is for.

Thanks for the challenge — the description is materially more accurate for it.

[lolyu]

Thanks — and your read of the mechanism matches mine on every point, including the off / orig_off equivalence and the stale_session_deleted re-classification path. Answering the two open items.

"Is anything consuming IF_IN_DISCARDS / RX_DRP to detect ACL denies?"

No, as far as I can find, and the designed path is a different counter entirely.

• Where SONiC does look: the ACL_ANY DEBUG_COUNTER (tests/drop_packets/test_configurable_drop_counters.py), and the per-ACE acl_facts packets_count. Both untouched here.
• Where I looked for consumers: files/image_config/monit/ and sonic-swss-common — no reference to SAI_PORT_STAT_IF_IN_DISCARDS or RX_DRP keyed on ACL denies.

Stronger than "nothing breaks", though: upstream actively asserts the opposite of the current behaviour. For discard_group == "ACL", test_drop_counters.py requires the per-ACE counter to increment and then calls ensure_no_l3_and_l2_drops — so RX_DRP incrementing on an ACL deny is what upstream treats as the failure, on any platform not listed in combined_drop_counters.yml's acl_l2. VPP isn't listed there.

On your framing point — you were right, and it went further than you flagged

You suggested framing this as a deliberate SONiC/VPP semantic rather than an RFC mandate. Agreed, and while checking I found the RFC sentence is weaker than I'd represented ("one possible reason ... could be to free up buffer space" — an example, not the definition), so a strict reading actually supports @nhegde-microsoft's objection. I've dropped that argument.

I also had to retract the adjacent claim outright: the description said SONiC "already treats ACL drops as RX_ERR". It doesn't — upstream expects neither RX_ERR nor RX_DRP, only the per-ACE counter. The patch already did the right thing; the stated reason was wrong. Description rewritten.

Version label

Good catch. The testbed run was built as 2606-0.6; the branch now says 0.7 purely because #278 merged first and took 0.6 for patch 0018. The patch content is byte-identical to what was tested — I confirmed the 0019 patch file is unchanged across the rebase, with only series and rules/vpp.mk differing — so no re-test is implied. I've added a note to that effect in the description.

Worth flagging for anyone landing a third ACL patch: rules/vpp.mk does not conflict in this situation, because both branches make the identical 0.5→0.6 edit and git merges them silently. The stale-deb hazard is invisible unless you go looking.

[nhegde-microsoft]

Thanks for the response. Agreed on the double counting. I see how it won't get double counted.
For the more basic question of changing SONiC vs VPP, if tests/drop_packets/combined_drop_counters.yml is meant to include platforms where ACL and L2 drop counters are combined, would it not be better to include VPP in the list instead of changing core VPP functions?

[AkeelAli]

I found that drop_packets test_acl_drop and test_acl_egress_drop have been passing on VPP only by accident. There's a bug in the test that skips the drop-counter check instead of running it.

Once I fix it, both tests FAIL on VPP t1-lag, confirming VPP counts ACL denies to RX_DRP, not just the dual-tor mux path. So VPP was never actually validated against the ACL/RX_DRP check, it just wasn't being checked.

I put up a sonic-mgmt-side fix here (test bug fix + adding vpp to acl_l2): sonic-net/sonic-mgmt#27815. With these changes, the test now legitimately passes on sonic-vpp (tested it locally on t1-lag).

Note though, the acl_l2 route of my fix conflicts with this PR's approach, and it doesn't fix dualtor_io test_upstream_standby_rx_drop_check either.

[yue-fred-gao]

I agree with @nhegde-microsoft and @AkeelAli, since many platforms in acl_l2 list are treating ACL drop as l2 drop, vpp can do the same. It is not 0 cost to behave like the other half, particularly the patch to maintain.