Turn multicast into an optional feature

[Nieuwejaar]

No description provided.

[zeeshanlakhani]

Hey @Nieuwejaar, here's my first pass on changes. Even though mcast is not specifically related to issues we've seen, feature-gating is probably still a good idea overall. It'd be nice to make this cleaner though.

Let me know if you want any help working through this too, if you have other stuff.

[zeeshanlakhani]

Of note, we could move tables, counters, link/mac helpers into an mcast_support module, and move more cfg into one place, e.g.:

// mcast_support.rs
// === Tables ===
pub const TABLES: &[(&str, TableType)] = {
    #[cfg(feature = "multicast")]
    { &[
        (mcast::mcast_replication::IPV6_TABLE_NAME, TableType::McastIpv6),
        // ... rest of mcast tables
    ]}
    #[cfg(not(feature = "multicast"))]
    { &[] }
};

// === Counters ===
pub const COUNTERS: &[CounterDescription] = { /* same pattern */ };

// === MAC helpers ===
pub fn set_mac(switch: &Switch, asic_id: AsicId, mac: MacAddr) -> DpdResult<()> {
    #[cfg(feature = "multicast")]
    {
        MacOps::<mcast::mcast_port_mac::PortMacTable>::mac_set(switch, asic_id, mac)?;
        mcast::mcast_egress::add_port_mapping_entry(switch, asic_id)?;
    }
    Ok(())
}

pub fn clear_mac(switch: &Switch, asic_id: AsicId) -> DpdResult<()> { /* ... */ }
pub fn reset_tables(switch: &Switch) -> DpdResult<()> { /* ... */ }

Just throwing that out there as well for cleanup.

[zeeshanlakhani]

Looking really good @Nieuwejaar. I had a couple more comments; otherwise, this is good.

[zeeshanlakhani]

We may want to avoid empty features if a flag (in this case the mcast flag) is not provided?

[Nieuwejaar]

Either way works. If you think it's easier to read the other way, I'm happy to switch.

[zeeshanlakhani]

@Nieuwejaar It's minor but could be confusing?

We could do ${SWADM_FEATURES:+--features=$SWADM_FEATURES} right; instead of letting cargo just ignore the empty?

Suggested change

	--features=$SWADM_FEATURES \
	DENDRITE_TEST_VERBOSITY=3 \
	cargo test \
	--no-fail-fast \
	${SWADM_FEATURES:+--features=$SWADM_FEATURES} \
	--test \
	counters \
	...

[Nieuwejaar]

I want to keep all the feature definitions in one place. I went with:

diff --git a/.github/buildomat/packet-test-common.sh b/.github/buildomat/packet-test-common.sh
index 9ca7b31..bf6b2b4 100755
--- a/.github/buildomat/packet-test-common.sh
+++ b/.github/buildomat/packet-test-common.sh
@@ -15,7 +15,7 @@ if [ x$MULTICAST == x ]; then
     else
         BUILD_FEATURES=tofino_asic,multicast
        CODEGEN_FEATURES=--multicast
-        SWADM_FEATURES=multicast
+        SWADM_FEATURES=--features=multicast
 fi

 function cleanup {
@@ -104,7 +104,7 @@ DENDRITE_TEST_HOST='[::1]' \
     DENDRITE_TEST_VERBOSITY=3 \
     cargo test \
     --no-fail-fast \
-    --features=$SWADM_FEATURES \
+    $SWADM_FEATURES \
     --test \
     counters \
     -- \

[zeeshanlakhani]

Works +1.

[zeeshanlakhani]

We were doing link-local mcast (non PRE'ed) counting here. I know we didn't in times before multicast, but not sure if we want to ifdef the whole controller?

[Nieuwejaar]

I must be missing something. Isn't link-local multicast handled in the Services controller in the Ingress pipeline, then sent to the switch zone, bypassing the egress pipeline?

[zeeshanlakhani]

It is handled by the service controller, but it doesn't hit the bypass_egress flag. Previous to PRE (replicated) multicast, link-local mcast would hit the empty Egress and do nothing. During my work on PRE, adding the Egress pipeline, I added counters for those packets that made it through to the end of pipeline (since I was handling mcast anyway, and counting it in one go, as well as separately). Beforehand, it was just lumped into the ingress_ctr. We could completely bypass it (changing the logic there), but it wasn't bypassed in earlier versions.

[zeeshanlakhani]

In discussion with @Nieuwejaar, we had the counter already when it hit the service counter (SVC_COUNTER_INBOUND_LL), so we could just use that and bypass_egress for those packets (and rm the egress LL counter)

[zeeshanlakhani]

@Nieuwejaar think this change is the last bit left, right?

[Nieuwejaar]

There's no harm in leaving that counter in place. When multicast is enabled, it gives us an easy way to classify the different kinds of packets being sent to each port.