Repository navigation
[p4, sidecar] fix underlay/external mcast counter ops - #362
Open
zeeshanlakhani wants to merge 10 commits into
Open
zeeshanlakhani wants to merge 10 commits into
zeeshanlakhani wants to merge 10 commits into
Conversation
zeeshanlakhani
force-pushed
the
zl/mcast-p4-counters
branch
5 times, most recently
from
September 3, 2026 05:25
ee83887 to
873c3d3
Compare
zeeshanlakhani
changed the title
zeeshanlakhani
force-pushed
the
zl/mcast-p4-counters
branch
from
September 3, 2026 09:21
873c3d3 to
cf6c3e5
Compare
zeeshanlakhani
requested review from
FelixMcFelix,
cfzimmerman and
taspelund
September 6, 2026 04:56
zeeshanlakhani
force-pushed
the
zl/mcast-p4-counters
branch
from
September 8, 2026 03:47
cf6c3e5 to
22b3957
Compare
zeeshanlakhani
removed this pull request from stack #363
September 9, 2026 01:59
A replica still carrying a Geneve header at the egress counter block is bound
for an underlay port, while external replicas are decapsulated earlier, at
ingress, for external-only groups or in `mcast_egress` for bifurcated
(external + underlay members) ones. Previously, we used a conditional:
```p4
else if (hdr.geneve.isValid()) {
```
... on Geneve hdr validity too early in the chain to determine an
external- vs underlay-destined packet, and we didn't consider using the
`is_egress_rid_mcast` check at all.
## The Fix
Now, we treat the three conditions for link-local, external mcast, and
underlay mcast independently and more explicitly given the egress rid,
which is 0 for link-local multicast packets and combines with Geneve hdr
validity to determine the count for underlay vs external.
The underlay and external arms key on `is_egress_rid_mcast` and not the
negation of `is_link_local_ipv6_mcast`: the compiler doesn't carry the
negation into the later gateways of the pipeline, which meant that a
link-local packet incremented `external_mcast_ctr` alongside its own
counter. This is now handled because link-local replicas arrive with
`egress_rid` of 0, and scope 2 is refused at group creation, so no
replicated packet ever carries a link-local outer destination.
zeeshanlakhani
force-pushed
the
zl/mcast-p4-counters
branch
from
September 9, 2026 14:53
22b3957 to
d203dc7
Compare
zeeshanlakhani
changed the base branch from
zl/mcast-default-feature
to
zl/mcast-remove-feature
September 9, 2026 14:54
zeeshanlakhani
added this pull request to stack #372
September 9, 2026 14:54
cfzimmerman
reviewed
Sep 10, 2026
cfzimmerman
left a comment
cfzimmerman
left a comment
Contributor
There was a problem hiding this comment.
Looks reasonable, but again I'd like to defer final approval to someone with more knowledge of the sidecar program.
zeeshanlakhani
removed this pull request from stack #372
September 23, 2026 18:38
zeeshanlakhani
added this pull request to stack #377
September 24, 2026 17:37
zeeshanlakhani
removed this pull request from stack #377
October 6, 2026 02:42
> IngressDeparser has the issue that writing it more cleanly uses too > many checksum engines, because the compiler doesn't recognise the > disjoint cases for some reason. Do we end up running out of stages if > we do a sensible if...? This reworks our counter logic per Kyle's comment (quoted above). Nesting the per-type checks (and, separately, an else-if chain with link-local checked first) dropped the underlay count to 0 during testing, even though stage usage didn't change. However, this led me to find an issue with: ```p4 bit<16> ipv6_prefix = (bit<16>)hdr.ipv6.dst_addr[127:112]; is_link_local_ipv6_mcast = (ipv6_prefix == IPV6_LINK_LOCAL_16); ``` This bool lands within a 16-bit container, and the conditionals on it (e.g., `is_mcast` and link_local checks) are built with *-only match rows, making them always true. So, every forwarded packet was counted as `Multicast`, and `Unicast` remained at 0. Now, the bool is set inside the `if` and the chain branches on `egress_rid` first. We don't see any significant issues with the added cycles, however.
zeeshanlakhani
added this pull request to stack #397
October 7, 2026 10:17
zeeshanlakhani
removed this pull request from stack #397
October 13, 2026 13:58
zeeshanlakhani
added this pull request to stack #401
October 13, 2026 13:59
cfzimmerman
approved these changes
Oct 16, 2026
Contributor
Author
|
@cfzimmerman updated, including the counter fix. |
zeeshanlakhani
removed this pull request from stack #401
October 21, 2026 01:55
zeeshanlakhani
changed the base branch from
zl/mcast-remove-feature
to
zl/mcast-ingress-vlan
October 21, 2026 01:58
zeeshanlakhani
added this pull request to stack #410
October 21, 2026 01:59
cfzimmerman
approved these changes
Oct 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A replica still carrying a Geneve header at the egress counter block is bound for an underlay port, while external replicas are decapsulated earlier, at ingress, for external-only groups or in
mcast_egressfor bifurcated (external + underlay members) ones. Previously, we used a conditional:... on Geneve hdr validity too early in the chain to determine an external- vs underlay-destined packet, and we didn't consider using the
is_egress_rid_mcastcheck at all.Now, we treat the three conditions for link-local, external mcast, and underlay mcast independently and more explicitly given the egress rid, which is 0 for link-local multicast packets and combines with Geneve hdr validity to determine the count for underlay vs external.
The underlay and external arms key on
is_egress_rid_mcastand not the negation ofis_link_local_ipv6_mcast: the compiler doesn't carry the negation into the later gateways of the pipeline, which meant that a link-local packet incrementedexternal_mcast_ctralongside its own counter. This is now handled because link-local replicas arrive withegress_ridof 0, and scope 2 is refused at group creation, so no replicated packet ever carries a link-local outer destination.