dendrite#87 all tables should have counters added (#88)

Author: Nieuwejaar

dpd/p4/constants.p4

@@ -45,7 +45,9 @@ const bit<8> SVC_COUNTER_FW_FROM_USER = 1;
 const bit<8> SVC_COUNTER_V4_PING_REPLY = 2;
 const bit<8> SVC_COUNTER_V6_PING_REPLY = 3;
 const bit<8> SVC_COUNTER_BAD_PING = 4;
-const bit<32> SVC_COUNTER_MAX = 5;
+const bit<8> SVC_COUNTER_INBOUND_LL = 5;
+const bit<8> SVC_COUNTER_PASS = 6;
+const bit<32> SVC_COUNTER_MAX = 7;
 
 /* Encapped Multicast Tags */
 const bit<2> MULTICAST_TAG_EXTERNAL = 0;

dpd/p4/sidecar.p4

@@ -292,7 +292,6 @@ control Services(
 	// It's not strictly necessary, but we also bump up the TTL to make sure
 	// the packet makes it all the way back to the sender.
 	action ping4_reply() {
-		service_ctr.count(SVC_COUNTER_V4_PING_REPLY);
 		hdr.ethernet.dst_mac = meta.orig_src_mac;
 
 		hdr.ipv4.src_addr = hdr.ipv4.dst_addr;
@@ -305,10 +304,10 @@ control Services(
 		ig_tm_md.ucast_egress_port = ig_intr_md.ingress_port;
 
 		meta.service_routed = true;
+		service_ctr.count(SVC_COUNTER_V4_PING_REPLY);
 	}
 
 	action ping6_reply() {
-		service_ctr.count(SVC_COUNTER_V6_PING_REPLY);
 		hdr.ethernet.dst_mac = meta.orig_src_mac;
 
 		bit<128> orig_src = hdr.ipv6.src_addr;
@@ -322,6 +321,7 @@ control Services(
 		ig_tm_md.ucast_egress_port = ig_intr_md.ingress_port;
 
 		meta.service_routed = true;
+		service_ctr.count(SVC_COUNTER_V6_PING_REPLY);
 	}
 
 	// Send a network service request to a service port.  Push on a
@@ -372,6 +372,11 @@ control Services(
 		ig_tm_md.ucast_egress_port = USER_SPACE_SERVICE_PORT;
 		meta.is_mcast = true;
 		meta.is_link_local_mcastv6 = true;
+		service_ctr.count(SVC_COUNTER_INBOUND_LL);
+	}
+
+	action no_service() {
+		service_ctr.count(SVC_COUNTER_PASS);
 	}
 
 	table service {
@@ -398,6 +403,7 @@ control Services(
 			forward_from_userspace;
 			forward_to_userspace;
 			mcast_inbound_link_local;
+			no_service;
 		}
 
 		const entries = {
@@ -417,6 +423,7 @@ control Services(
 			( 0, false, true, true, _, _, _, _, _, _, _, _, _ ) : mcast_inbound_link_local;
 		}
 
+		default_action = no_service;
 		const size = 16;
 	}
 
@@ -1384,15 +1391,19 @@ control MacRewrite(
 	inout sidecar_headers_t hdr,
 	in PortId_t port
 ) {
+	DirectCounter<bit<32>>(CounterType_t.PACKETS_AND_BYTES) ctr;
+
 	action rewrite(mac_addr_t mac) {
 		hdr.ethernet.src_mac = mac;
+		ctr.count();
 	}
 
 	table mac_rewrite {
 		key     = { port: exact ; }
 		actions = { rewrite; }
 
 		const size = 256;
+		counters = ctr;
 	}
 
 	apply {
@@ -1409,15 +1420,19 @@ control MulticastMacRewrite(
 	inout sidecar_headers_t hdr,
 	in PortId_t port
 ) {
+	DirectCounter<bit<32>>(CounterType_t.PACKETS_AND_BYTES) ctr;
+
 	action rewrite(mac_addr_t mac) {
 		hdr.ethernet.src_mac = mac;
+		ctr.count();
 	}
 
 	table mac_rewrite {
 		key     = { port: exact ; }
 		actions = { rewrite; }
 
 		const size = 256;
+		counters = ctr;
 	}
 
 	apply {
@@ -2089,6 +2104,7 @@ control Egress(
 	MulticastMacRewrite() mac_rewrite;
 	MulticastEgress() mcast_egress;
 
+	Counter<bit<64>, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) unicast_ctr;
 	Counter<bit<64>, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) mcast_ctr;
 	Counter<bit<64>, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) link_local_mcast_ctr;
 	Counter<bit<64>, PortId_t>(512, CounterType_t.PACKETS_AND_BYTES) external_mcast_ctr;
@@ -2100,12 +2116,12 @@ control Egress(
 		// Check multicast egress packets by checking that RID is not 0.
 		bool is_egress_rid_mcast = eg_intr_md.egress_rid > 0;
 		// We track IPv6 multicast packets separately for counters.
-		bool is_ipv6_mcast = false;
+		bool is_link_local_ipv6_mcast = false;
 		if (hdr.ipv6.isValid()) {
 			bit<16> ipv6_prefix = (bit<16>)hdr.ipv6.dst_addr[127:112];
-			is_ipv6_mcast = (ipv6_prefix != 16w0xff02);
+			is_link_local_ipv6_mcast = (ipv6_prefix == 16w0xff02);
 		}
-		bool is_mcast = is_egress_rid_mcast || is_ipv6_mcast;
+		bool is_mcast = is_egress_rid_mcast || is_link_local_ipv6_mcast;
 
 		if (is_egress_rid_mcast == true) {
 			if (meta.bridge_hdr.ingress_port == eg_intr_md.egress_port) {
@@ -2117,7 +2133,8 @@ control Egress(
 				mcast_egress.apply(hdr, meta, eg_intr_md, eg_dprsr_md);
 				mac_rewrite.apply(hdr, eg_intr_md.egress_port);
 			}
-        } else if (eg_intr_md.egress_rid == 0 && eg_intr_md.egress_rid_first == 1) {
+		} else if (eg_intr_md.egress_rid == 0 &&
+		    eg_intr_md.egress_rid_first == 1) {
 			// Drop CPU copies (RID=0) to prevent unwanted packets on port 0
 			eg_dprsr_md.drop_ctl = 1;
 			meta.drop_reason = DROP_MULTICAST_CPU_COPY;
@@ -2130,7 +2147,7 @@ control Egress(
 		} else if (is_mcast == true) {
 			mcast_ctr.count(eg_intr_md.egress_port);
 
-			if (is_ipv6_mcast) {
+			if (is_link_local_ipv6_mcast) {
 				link_local_mcast_ctr.count(eg_intr_md.egress_port);
 			} else if (hdr.geneve.isValid()) {
 				external_mcast_ctr.count(eg_intr_md.egress_port);
@@ -2139,6 +2156,10 @@ control Egress(
 			           hdr.geneve_opts.oxg_mcast.mcast_tag == MULTICAST_TAG_UNDERLAY) {
 				underlay_mcast_ctr.count(eg_intr_md.egress_port);
 			}
+		} else {
+			# non-multicast packets should bypass the egress
+			# pipeline, so we would expect this to be 0.
+			unicast_ctr.count(eg_intr_md.egress_port);
 		}
 	}
 }

dpd/src/counters.rs

@@ -52,10 +52,16 @@ enum CounterId {
     Service,
     Ingress,
     Egress,
-    Multicast,
     Packet,
     DropPort,
     DropReason,
+    EgressDropPort,
+    EgressDropReason,
+    Unicast,
+    Multicast,
+    MulticastExt,
+    MulticastLL,
+    MulticastUL,
 }
 
 impl From<CounterId> for u8 {
@@ -78,7 +84,7 @@ struct CounterDescription {
     p4_name: &'static str,
 }
 
-const COUNTERS: [CounterDescription; 12] = [
+const COUNTERS: [CounterDescription; 13] = [
     CounterDescription {
         id: CounterId::Service,
         client_name: "Service",
@@ -110,32 +116,37 @@ const COUNTERS: [CounterDescription; 12] = [
         p4_name: "pipe.Ingress.drop_reason_ctr",
     },
     CounterDescription {
-        id: CounterId::DropPort,
+        id: CounterId::EgressDropPort,
         client_name: "Egress_Drop_Port",
         p4_name: "pipe.Egress.drop_port_ctr",
     },
     CounterDescription {
-        id: CounterId::DropReason,
+        id: CounterId::EgressDropReason,
         client_name: "Egress_Drop_Reason",
         p4_name: "pipe.Egress.drop_reason_ctr",
     },
     CounterDescription {
-        id: CounterId::Multicast,
-        client_name: "Multicast",
-        p4_name: "pipe.Egress.mcast_ctr",
+        id: CounterId::Unicast,
+        client_name: "Unicast",
+        p4_name: "pipe.Egress.unicast_ctr",
     },
     CounterDescription {
         id: CounterId::Multicast,
-        client_name: "Multicast_Link_Local",
-        p4_name: "pipe.Egress.link_local_mcast_ctr",
+        client_name: "Multicast",
+        p4_name: "pipe.Egress.mcast_ctr",
     },
     CounterDescription {
-        id: CounterId::Multicast,
+        id: CounterId::MulticastExt,
         client_name: "Multicast_External",
         p4_name: "pipe.Egress.external_mcast_ctr",
     },
     CounterDescription {
-        id: CounterId::Multicast,
+        id: CounterId::MulticastLL,
+        client_name: "Multicast_Link_Local",
+        p4_name: "pipe.Egress.link_local_mcast_ctr",
+    },
+    CounterDescription {
+        id: CounterId::MulticastUL,
         client_name: "Multicast_Underlay",
         p4_name: "pipe.Egress.underlay_mcast_ctr",
     },
@@ -230,6 +241,8 @@ fn service_label(ctr: u8) -> Option<String> {
         2 => "ping_v4_reply".to_string(),
         3 => "ping_v6_reply".to_string(),
         4 => "bad_ping".to_string(),
+        5 => "inbound_link_local".to_string(),
+        6 => "pass".to_string(),
         x => format!("unknown service counter {x}"),
     };
     Some(label)
@@ -397,9 +410,16 @@ pub async fn get_values(
             CounterId::Service => service_label(idx.idx as u8),
             CounterId::Ingress
             | CounterId::Egress
+            | CounterId::EgressDropPort
             | CounterId::DropPort
-            | CounterId::Multicast => port_label(switch, idx.idx).await,
-            CounterId::DropReason => reason_label(idx.idx as u8)?,
+            | CounterId::Unicast
+            | CounterId::Multicast
+            | CounterId::MulticastExt
+            | CounterId::MulticastLL
+            | CounterId::MulticastUL => port_label(switch, idx.idx).await,
+            CounterId::DropReason | CounterId::EgressDropReason => {
+                reason_label(idx.idx as u8)?
+            }
         };
 
         if let Some(key) = key {

dpd/src/table/mod.rs

@@ -333,6 +333,9 @@ pub fn get_counters(
             neighbor_ipv6::counter_fetch(switch, force_sync)
         }
         TableType::ArpIpv4 => arp_ipv4::counter_fetch(switch, force_sync),
+        TableType::PortMac => {
+            MacOps::<port_mac::PortMacTable>::counter_fetch(switch, force_sync)
+        }
         TableType::NatIngressIpv4 => {
             nat::ipv4_counter_fetch(switch, force_sync)
         }
@@ -369,10 +372,11 @@ pub fn get_counters(
         TableType::McastEgressPortMapping => {
             mcast::mcast_egress::port_mapping_counter_fetch(switch, force_sync)
         }
-
-        // There is no counter in the PortMac table, as it duplicates data
-        // already available in the rmon egress counter.
-        _ => Err(DpdError::NoSuchTable(name)),
+        TableType::PortMacMcast => {
+            MacOps::<mcast::mcast_port_mac::PortMacTable>::counter_fetch(
+                switch, force_sync,
+            )
+        }
     }
 }
 
@@ -548,6 +552,13 @@ impl<T: MacTable> MacOps<T> {
         s.table_dump::<MacMatchKey, MacAction>(T::table_type())
     }
 
+    pub fn counter_fetch(
+        s: &Switch,
+        force_sync: bool,
+    ) -> DpdResult<Vec<views::TableCounterEntry>> {
+        s.counter_fetch::<MacMatchKey>(force_sync, T::table_type())
+    }
+
     /// Remove all entries from the MAC table.
     #[cfg_attr(not(feature = "tofino_asic"), allow(dead_code))]
     pub fn reset(s: &Switch) -> DpdResult<()> {