[feature] ASIC-focused 1st draft of multicast PRE #14

zeeshanlakhani · 2025-04-01T17:03:14Z

Note: This mainly works for the tofinio_asic feature. Hence, I've created a multicast branch to plug updates/PRs into.

Includes:
* Multicast Group API management: add, modify, delete, reset, SSM handling
* sidecar.p4 tofino_asic updates to integrate multicast packet replication
* Table management for ASIC switch tables for multicast
* integration tests and test utility additions

Includes: * Multicast Group API management: add, modify, delete, reset, SSM handling * sidecar.p4 tofino_asic updates to integrate multicast packet replication * Table mangagement for ASIC switch tables for multicast * integration tests and test utility additions

zeeshanlakhani · 2025-04-15T19:38:25Z

I'll add some notes to the PR tomorrow morning (to explain through pieces).

zeeshanlakhani · 2025-04-16T09:14:25Z

dpd-client/tests/integration_tests/common.rs

+    ///
+    /// If that counter isn't in the set returned by dpd, we return
+    /// an error to the caller.
+    pub async fn get_counter(


I moved these functions in for use beyond the counters test (and having mcast tests all live in that module)

zeeshanlakhani · 2025-04-16T09:52:26Z

dpd/src/counters.rs

    CounterDescription {
        id: CounterId::DropPort,
-        client_name: "Drop_Port",
+        client_name: "Ingress_Drop_Port",


Note: client name changes to decipher.

zeeshanlakhani · 2025-04-16T09:55:09Z

dpd/p4/constants.p4

+const bit<8> DROP_MULTICAST_INVALID_MAC         = 0x13;
+const bit<8> DROP_MULTICAST_CPU_COPY            = 0x14;
+const bit<8> DROP_MULTICAST_SOURCE_FILTERED     = 0x15;
+const bit<32> DROP_REASON_MAX			= 0x16;


Note: I did move this around to make MAX post the new drop additions.

rcgoodfellow

Thanks Zeeshan. I've provided some comments on the API and am about half way through the P4 code. Have to run, but wanted to get these initial comments in.

dpd/src/api_server.rs

dpd/p4/sidecar.p4

rcgoodfellow · 2025-04-17T17:44:54Z

dpd/p4/sidecar.p4

+			( 0, false, true, true, _, USER_SPACE_SERVICE_PORT, true, _, _, _, _, _, _ ) : forward_from_userspace;
+			( 0, false, false, _, _, _, false, true, _, _, _, _, _ ) : forward_to_userspace;
+			( 0, false, false, _, true, _, _, _, _, _, _, _, _ ) : forward_to_userspace;
+			// Link-local multicast


Since we're changing quite a bit around link-local multicast, we need to test this e2e with ddm.

dpd/p4/sidecar.p4

FelixMcFelix

Thanks for this Zeeshan, some notes so far. I haven't yet made it through dpd/src/mcast.rs and the table management code yet.

dpd/src/api_server.rs

dpd-client/tests/integration_tests/mcast.rs

dpd-client/tests/integration_tests/table_tests.rs

dpd-client/tests/integration_tests/mcast.rs

dpd/p4/constants.p4

dpd/p4/parser.p4

dpd/p4/sidecar.p4

Includes: * make sure to avoid switch table apply on drop-specific code in sidecar

zeeshanlakhani · 2025-04-22T12:22:17Z

Got this back down to 14 stages.

FelixMcFelix

I think I've had a look over all the changes now, thanks for acting on the last round of feedback.

dpd/src/mcast/validate.rs

dpd/src/mcast.rs

FelixMcFelix · 2025-04-23T16:32:29Z

dpd-client/tests/integration_tests/mcast.rs

+        expected_pkts.push(TestPacket {
+            packet: Arc::new(to_recv.clone()),
+            port: egress1,
+        });


I think this ties in with my confusion over why we're always following NAT ingress with NAT egress in the multicast case, given that the output packets are only different in their VLAN. I thought a bare frame would come out Geneve encapsulated onto the underlay network (integration_tests::nat::gen_geneve_packet)? The behaviour I had maybe expected would be:

Multicast packet arrives on a front panel port, possibly on VLAN x.

Strip VLAN, Geneve encapsulate packet using fields in group_entry.nat_target.

Adjust TTL.

Replicate Geneve encapsulated frames out on all rear ports on the mcast group.

Encapsulated multicast packet arrives on a rear panel port.

Adjust TTL.

Replicate frame, including Geneve, onto all other rear panel ports in the group.

Replicate frame, excluding Geneve, onto all front panel ports in the group. Insert VLAN x if required by link.

Am I coming at this the wrong way?

Yeah, I get your question (and we're chatting tmrw); however, I'm trying to map/orient how we should do this (vs unicast). Today, nat_egress is triggered when we match a switch_address (a v4/v6 address assigned to ports on the switch when configuring port settings on switch initialization). I definitely don't know enough (classic!), but for multicast group creation, there's nothing specifically tying multicast groups to ingress ports (so that we'd know if it's a front vs rear panel port or not), and it will dynamically change.

So, we'd need to explore an alternative approach to what we did for unicast before, though we could reuse the switch tables, possibly?

Ok, after discussion, we have a way forward in separating out ingress(encap)/egress(decap) ports for replication (and some minor changes to the .p4 code to avoid checks when necessary). I'll push the changes up later today.

Nieuwejaar · 2025-04-24T11:57:31Z

dpd/p4/constants.p4

-const bit<8> SC_ARP_NEEDED		= 0x03;
-const bit<8> SC_NEIGHBOR_NEEDED		= 0x04;
-const bit<8> SC_INVALID			= 0xff;
+// Includes the checksum for the original data, the geneve header, the


Why is all of this in this file? None of these are constants.

Ok, yeah, I'll move defines to the top of the sidecar.

Nieuwejaar · 2025-04-24T13:38:46Z

dpd/src/api_server.rs

+    let group_id = query_id.into_inner().group_id;
+
+    // If a group ID is provided, get the group by ID
+    if let Some(group_id) = group_id {


Elsewhere, we have different entry points for GETs returning all or one. e.g., /ndp/ and /ndp/{ip}.

yeah, that's why I made this a query param if you want to use the ID (vs the {ip} variant). But, maybe I should just remove the get by group_id entirely to avoid confusion.

I think I'm saying the opposite of that. I'm asking you to add a second entry point rather than use a query param, so this is consistent with the other APIs. If you don't think there is a use case for getting a single group, then you can remove the query param and not add a second endpoint.

Yeah. With the updates coming for separating ingress/egress port members, I think it's less necessary.

Sadly, for a second entry point, I ran into issues with/ dropshot b/c it would error on having both /group/{ip} and /group/{id} (even with different types), so then the route names would have to be different. So, yeah, I'll remove it in the next push.

Nieuwejaar · 2025-04-24T13:47:15Z

dpd/src/route.rs

+    Ord,
+    PartialOrd,
+    Clone,
+    Deserialize,


Why add these new derives? It doesn't look like VlanId is part of the API.

I waas using it previously, but dropped it. Good catch, will remove.

Nieuwejaar · 2025-04-24T13:51:58Z

dpd/src/table/mcast.rs

This should be table/mcast/mod.rs to keep with the style elsewhere in the repo.
I would also rename the sub-modules so they are unique, and we don't have two route.rs modules inside dpd. Maybe prefix them all with mcast_?

Yeah, I'll move to the "older style".

dpd/p4/constants.p4

Nieuwejaar · 2025-04-24T14:00:01Z

dpd/src/table/route_ipv4.rs

@@ -30,7 +30,7 @@ struct IndexKey {

 // Route entries stored in the index->route_data table
 #[derive(ActionParse, Debug)]
-enum RouteAction {
+pub(crate) enum RouteAction {


I know it leads to a little code duplication, but I'd like to keep the table keys private to their modules.

Nieuwejaar · 2025-04-24T14:02:54Z

dpd/src/types.rs

@@ -85,6 +85,10 @@ pub enum DpdError {
    Oximeter(String),
    #[error("No switch identifiers available")]
    NoSwitchIdentifiers,
+    #[error("Multicast group error: {}", .0)]
+    McastGroup(String),
+    #[error("Resource exhausted: {}", .0)]


The only place this is used is when we're out of group IDs. You could just make that a specific error, rather than the vague "resource exhausted".

ResourceExhaused is for the next line, but I'll rename this to something more apt.

Nieuwejaar · 2025-04-24T14:03:46Z

packet/src/ipv4.rs

@@ -104,6 +104,19 @@ impl Ipv4Hdr {
        hdr.ipv4_ttl = ttl as u8;
        Ipv4Hdr::update_checksum(pkt);
    }
+
+    pub fn set_identification(pkt: &mut Packet, id: u16) {


Neither of these seem to be used anywhere.

Yep. I was initially setting these into the packet to copy over some mcast info. But, I'll remove this.

Nieuwejaar · 2025-04-24T14:05:33Z

packet/src/ipv6.rs

-    pub fn adjust_hlim(pkt: &mut Packet, delta: i8) {
-        let hdr = pkt.hdrs.ipv6_hdr.as_mut().unwrap();
-        hdr.ipv6_hop_lim = (hdr.ipv6_hop_lim as i8 + delta) as u8;
+    pub fn adjust_hlim(pkt: &mut Packet, delta: i16) {


This is an 8 bit field - why allow a 16 bit delta?

It was so I could update it to set it to 0 for a test:

ipv6::Ipv6Hdr::adjust_hlim(&mut to_send, -255);

zeeshanlakhani force-pushed the zl/p4-mcast branch 3 times, most recently from 4e2fc4b to 1ce4908 Compare April 2, 2025 02:40

zeeshanlakhani force-pushed the zl/p4-mcast branch from 1ce4908 to df62508 Compare April 15, 2025 18:10

zeeshanlakhani changed the title ~~wip~~ [feature] ASIC-focused 1st draft of multicast PRE Apr 15, 2025

[fix] change stage max

4a945a8

zeeshanlakhani force-pushed the zl/p4-mcast branch from 2426e18 to 4a945a8 Compare April 15, 2025 19:36

zeeshanlakhani requested review from rcgoodfellow and Nieuwejaar April 15, 2025 19:37

zeeshanlakhani marked this pull request as ready for review April 15, 2025 19:37

zeeshanlakhani requested a review from FelixMcFelix April 15, 2025 19:38

zeeshanlakhani commented Apr 16, 2025

View reviewed changes

update chaos tables, revert any p4 formatting for review

bb876fa

zeeshanlakhani commented Apr 16, 2025

View reviewed changes

[minor] comments/align

2dcb98e

zeeshanlakhani commented Apr 16, 2025

View reviewed changes

rcgoodfellow reviewed Apr 17, 2025

View reviewed changes

FelixMcFelix reviewed Apr 17, 2025

View reviewed changes

zeeshanlakhani added 3 commits April 21, 2025 20:05

Merge remote-tracking branch 'origin/multicast' into zl/p4-mcast-3

3ef78f0

[review] address first pass of review

37498e4

[review+] meta -> metadata, fix test issues

d4953df

Includes: * make sure to avoid switch table apply on drop-specific code in sidecar

zeeshanlakhani requested review from FelixMcFelix and rcgoodfellow April 21, 2025 21:43

zeeshanlakhani and others added 2 commits April 21, 2025 23:34

[stages] test back to 14

dbe684f

..

5537794

This was referenced Apr 22, 2025

Moving to Multicast: Look into Warnings found in Tofinio Simulator #34

Open

Egress (port) counters return 0 even when counters are updated in Tofino Simulator #35

Open

zeeshanlakhani requested a review from taspelund April 22, 2025 18:39

FelixMcFelix reviewed Apr 23, 2025

View reviewed changes

zeeshanlakhani and others added 2 commits April 23, 2025 19:37

[review] validation and underlying api transactions

1e29a70

..

4936363

Nieuwejaar reviewed Apr 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feature] ASIC-focused 1st draft of multicast PRE #14

[feature] ASIC-focused 1st draft of multicast PRE #14

zeeshanlakhani commented Apr 1, 2025 •

edited

Loading

zeeshanlakhani commented Apr 15, 2025

zeeshanlakhani Apr 16, 2025 •

edited

Loading

zeeshanlakhani Apr 16, 2025

zeeshanlakhani Apr 16, 2025

rcgoodfellow left a comment

rcgoodfellow Apr 17, 2025

FelixMcFelix left a comment

zeeshanlakhani commented Apr 22, 2025

FelixMcFelix left a comment

FelixMcFelix Apr 23, 2025 •

edited

Loading

zeeshanlakhani Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 25, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

Nieuwejaar Apr 24, 2025

zeeshanlakhani Apr 24, 2025

[feature] ASIC-focused 1st draft of multicast PRE #14

Are you sure you want to change the base?

[feature] ASIC-focused 1st draft of multicast PRE #14

Conversation

zeeshanlakhani commented Apr 1, 2025 • edited Loading

zeeshanlakhani commented Apr 15, 2025

zeeshanlakhani Apr 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcgoodfellow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FelixMcFelix left a comment

Choose a reason for hiding this comment

zeeshanlakhani commented Apr 22, 2025

FelixMcFelix left a comment

Choose a reason for hiding this comment

FelixMcFelix Apr 23, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeeshanlakhani commented Apr 1, 2025 •

edited

Loading

zeeshanlakhani Apr 16, 2025 •

edited

Loading

FelixMcFelix Apr 23, 2025 •

edited

Loading