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]

This ifdef'ing is a bit gnarly (understandably so). Instead can we can treat (some of) the Mcast controls as passthrough stub-definitions when multicast is disabled, e.g.:

#ifdef MULTICAST
control MulticastRouter4(...) {
    // ... multicast logic ...
}
#else
control MulticastRouter4(...) {
    Router4.apply(hdr, meta, ig_intr_md, ig_tm_md);
}
#endif

and then use singular control flow everywhere:

if (hdr.ipv4.isValid()) {
    if (meta.is_mcast && !meta.is_link_local_mcastv6) {
        // if mcast is disabled, this is a passthorugh to Router4, etc
        MulticastRouter4.apply(...);
    } else {
        Router4.apply(...);
    }
}

This pattern works well for the Router controls. Some of the other multicast controls (e.g., MulticastEgress) may need to be fully gated rather than stubbed, but reducing some of the duplication where possible would help.

[Nieuwejaar]

I see what you're saying, but I'm not sure it's any easier to read. It's just hard to read in a different way. I'm also reluctant to add anything funny to the compiler's job, given that we're trying to work around a potential compiler issue here.

[zeeshanlakhani]

Totally understand.

[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?

[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?