dendrite#87 all tables should have counters added #88
Conversation
dpd/p4/sidecar.p4
Outdated
| mcast_ctr.count(eg_intr_md.egress_port); | ||
|
|
||
| if (is_ipv6_mcast) { | ||
| if (!is_ipv6_mcast) { |
There was a problem hiding this comment.
At line 2122 we have:
is_ipv6_mcast = (ipv6_prefix != 16w0xff02);
ff02 is for link-local multicast, so is_ipv6_mcast means any non-linklocal multicast, right? So we want to bump the link-local counter for !is_ipv6_mcast? Or am I getting turned around by all the nots?
There was a problem hiding this comment.
The logic or possibly the variable naming seems kind of confused here, before this change. 0xff is the multi-cast prefix, and 0x02 means "link-local scope". So is_ipv6_mcast is true for anything that is not exactly a link-local multicast address. It is true for multicast addresses of other scopes, or for addresses that aren't multicast at all like link-local addresses or globally-unique addresses.
What are we trying to catch here? Only link-local multicast addresses? If that's true, then I'd probably invert the prefix check on L2122 and rename the variable to is_ipv6_ll_mcast or something like that.
There was a problem hiding this comment.
@Nieuwejaar yeah. My bad. I think this got lost in the review changes. This should be renamed to is_link_local_mcastv6 and be
is_link_local_ipv6_mcast = (ipv6_prefix == 16w0xff02);
egress_rid will capture replicated mcast packets. So, I think it's renamed for the counter, and we don't need the !.
There was a problem hiding this comment.
The logic or possibly the variable naming seems kind of confused here, before this change.
0xffis the multi-cast prefix, and0x02means "link-local scope". Sois_ipv6_mcastis true for anything that is not exactly a link-local multicast address. It is true for multicast addresses of other scopes, or for addresses that aren't multicast at all like link-local addresses or globally-unique addresses.What are we trying to catch here? Only link-local multicast addresses? If that's true, then I'd probably invert the prefix check on L2122 and rename the variable to
is_ipv6_ll_mcastor something like that.
Yeah. What I said in my comment (before I expanded yours @bnaecker) haha.
There was a problem hiding this comment.
I made the suggested change to is_link_local_ipv6_mcast
|
Good catch on the CounterIDs @Nieuwejaar. I forgot that's what the stats client relies on. |
Adds table counters to the 2 mac rewrite tables.
Adds new counters to the Service control, so each path is counted - including "take no action".
Adds a counter for unicast packets in the Egress pipeline
dpd was returning the
pipe.Egress.mcast_ctrcounter for all multicast counter requests fromswadm. Added newCounterIdvariants to catch them all.