[dpapp] Add flow in bmv2 pipeline #608

jimmyzhai · 2024-07-23T18:47:29Z

Following dpapp HLD - #606, this is the 2nd part implementation. It updates bmv2 pipeline to have stateful packet processing.

KrisNey-MSFT · 2024-07-30T21:28:04Z

Hi @jimmyzhai - is this also ready to merge? Thank you!

KrisNey-MSFT · 2024-08-06T02:17:22Z

Hi @jimmyzhai - looks like we have conflicts here too...

dash-pipeline/bmv2/dash_headers.p4

1. Update flow table 2. Add dash headers towards DPAPP

2. refactor dash_ingress

dash-pipeline/bmv2/dash_headers.p4

r12f · 2024-08-30T04:17:25Z

dash-pipeline/bmv2/dash_headers.p4

+const bit<16> PACKET_META_HDR_SIZE=dash_packet_meta_t.minSizeInBytes();
+
+#define DASH_ETHTYPE 0x876d
+#define DPAPP_MAC 0x02fe23f0e413


will the MAC be changed on other people's machine?

It's a temporary hardcode. Mark FIXME comment to fix it later.

r12f · 2024-08-30T04:19:22Z

dash-pipeline/bmv2/dash_metadata.p4

@@ -261,7 +140,7 @@ struct ha_data_t {
    bit<16> dp_channel_src_port_max;

    // HA packet/flow state
-    dash_ha_flow_sync_state_t flow_sync_state;
+    dash_flow_state_t flow_sync_state;


since you renamed the struct, will be better to also rename the field

Rename it to ha_flow_state.

r12f · 2024-08-30T04:28:35Z

dash-pipeline/bmv2/dash_metadata.p4

@@ -303,13 +182,18 @@ struct metadata_t {

    // Flow data
    conntrack_data_t conntrack_data;
+    flow_table_data_t flow_table;
+    dash_flow_state_t flow_state;


is this duplicated w/ the one in ha_data_t?

It is NO by now. Flow state machine and ha state machine are separated. We may refine it later.

this design is weird, what's their difference? if they are not the same, can we split them into 2 types?

if they are the same, then we should merge them into one.

r12f · 2024-08-30T04:30:46Z

dash-pipeline/bmv2/dash_parser.p4

        packet.extract(hd.u0_ethernet);
        transition select(hd.u0_ethernet.ether_type) {
            IPV4_ETHTYPE:  parse_u0_ipv4;
            IPV6_ETHTYPE:  parse_u0_ipv6;
+            DASH_ETHTYPE:  parse_dash;


parse_dash -> parse_dash_hdr and etc.

r12f · 2024-08-30T04:33:24Z

dash-pipeline/bmv2/dash_parser.p4

            default: accept;
        }
    }

+    state parse_dash {
+        packet.extract(hd.packet_meta);
+        if (hd.packet_meta.packet_subtype != dash_packet_subtype_t.NONE) {


will be better to check packet_subtype against the flow_xxx instead of NONE. it will be more robust and more readable.

r12f · 2024-08-30T04:34:16Z

dash-pipeline/bmv2/dash_pipeline.p4

-    table acl_group {
+    /* This table API should be implemented manually using underlay SAI */
+    @SaiTable[ignored = "true"]
+    table appliance {


need to sync to latest code. this is changed now.

r12f · 2024-08-30T04:49:45Z

dash-pipeline/bmv2/dash_pipeline.p4

@@ -17,65 +17,94 @@
 #include "stages/metering_update.p4"
 #include "underlay.p4"

-control dash_ingress(
+control dash_inbound_routing_stage(


this seems to be doing similar things as this PR: #569. will need to resolve the conflicts...

Merged PR #569.

…_pipeline

as make p4c-dpdk-pna fails due to: terminate called after throwing an instance of 'Util::CompilerBug' what(): In file: /p4c/backends/dpdk/dpdkArch.cpp:467 Compiler Bug: header fields should be of type bit<> or varbit<>found this is_ipv6

overlay_rewrite_data_t -> meta_overlay_rewrite_data_t, as header in struct is not well supported for target dpdk-pna.

r12f · 2024-09-03T06:55:06Z

dash-pipeline/bmv2/dash_metadata.p4

@@ -134,14 +46,6 @@ enum bit<8> dash_eni_mac_type_t {
 struct conntrack_data_t {
    bool allow_in;
    bool allow_out;


the following fields are flow related and intentionally being aggregated into this structure, because conntrack_data_t is used to hold all data that is related to flow.

and many of them are moved out from this structure and some fields seem to be missing, such as reverse_flow_key (it somehow doesn't show up in my search).

it doesn't feel to be right to me. this structure should not be changed. I can understand moving some of the data types as header, but the metadata should be remained - as they are different from header conceptually.

For flow, I intend to use a new flow metadata.

r12f · 2024-09-03T06:57:09Z

dash-pipeline/bmv2/stages/conntrack_lookup.p4

+        length = length + FLOW_DATA_HDR_SIZE;
+
+        if (meta.routing_actions & dash_routing_actions_t.STATIC_ENCAP != 0) {
+#ifdef TARGET_DPDK_PNA


is it possible to remove these ifdef?

Not necessary

r12f · 2024-09-03T07:01:16Z

dash-pipeline/bmv2/stages/conntrack_lookup.p4

-        meta.conntrack_data.flow_table.max_flow_count = max_flow_count;
-        meta.conntrack_data.flow_table.flow_enabled_key = dash_flow_enabled_key;
-        meta.conntrack_data.flow_table.flow_ttl_in_milliseconds = flow_ttl_in_milliseconds;
+        meta.flow_table.max_flow_count = max_flow_count;


do you mind to keep these fields in the conntrack_data, which makes the code easy the understand and won't complicates the metadata top-level structure - it is already a mess. : (

I'll re-organize them in flow metadata with new PR

r12f · 2024-09-03T07:03:52Z

dash-pipeline/SAI/specs/dash_flow.yaml

@@ -125,6 +119,12 @@ sai_apis:
      type: sai_uint16_t
      objects: null
      valid_only: null
+    - !!python/object:utils.sai_spec.sai_struct_entry.SaiStructEntry


wow... the API changes in this file is not expected, many of them are order change. we need to get this fixed. this will break the existing flow API.

…_pipeline

…gets

r12f · 2024-10-23T01:47:14Z

dash-pipeline/bmv2/dash_headers.p4

+    ENCAP_U1 = (1 << 1),
+    SET_SMAC = (1 << 2),
+    SET_DMAC = (1 << 3),
+    NAT = (1 << 4),


will be better to split into DNAT and SNAT, same to the NAT_PORT.

r12f · 2024-10-23T01:47:40Z

dash-pipeline/bmv2/dash_headers.p4

+    NAT46 = (1 << 5),
+    NAT64 = (1 << 6),
+    NAT_PORT = (1 << 7),
+    REVERSE_TUNNEL = (1 << 8)


sync'ed offline. remove : D

…_pipeline

r12f

overall looks good to me! the change is already large enough. let's get this change in first, and leave other improvements to later changes.

zhixiongniu · 2024-10-24T11:48:19Z

dash-pipeline/SAI/utils/sai_spec/sai_spec_utils.py

@@ -24,6 +24,7 @@ def load_word_fixers() -> None:
        "pb": "protocol buffer",
        "proto": "protocol",
        "smac": "source MAC",
+        "dest": "destination",


Why is the goal of dest?

This is only for word spell check, "dest" is used as a short style of "destination" in code/document.

zhixiongniu · 2024-10-24T11:58:25Z

dash-pipeline/bmv2/routing_actions/routing_action_encap_underlay.p4

+                        meta.u0_encap_data.vni);
+        }
+        else if (meta.u0_encap_data.dash_encapsulation == dash_encapsulation_t.NVGRE) {
+            push_vxlan_tunnel_u0(hdr,


Should we change this?

zhixiongniu · 2024-10-24T11:58:48Z

dash-pipeline/bmv2/routing_actions/routing_action_encap_underlay.p4

+                        meta.u0_encap_data.vni);
+        }
+        else if (meta.u0_encap_data.dash_encapsulation == dash_encapsulation_t.NVGRE) {
+            push_vxlan_tunnel_u1(hdr,


Should we change this?

zhixiongniu · 2024-10-24T12:24:23Z

dash-pipeline/bmv2/stages/conntrack_lookup.p4


+        /* Reverse flow key is not used by now */


Should put near the Reverse flow key.

Should be right here for a placeholder of reverse flow key.

zhixiongniu · 2024-10-24T12:29:39Z

dash-pipeline/bmv2/stages/conntrack_lookup.p4

+
+    apply {
+        if (!hdr.flow_key.isValid()) {
+            flow_table.apply();


Why this line is related to hdr.flow_key.isValid()?

If the packet is recirculated from dpapp to dash pipeline, the flow key has been filled in dash parser and become valid. Here not need to set flow key again.

…_t.NVGRE

r12f · 2024-10-26T01:36:44Z

Thanks Zhixiong for reviewing it!

KrisNey-MSFT added the Ready to merge? label Jul 30, 2024

jimmyzhai force-pushed the dp_app_add_flow_in_pipeline branch from bb8a85a to 1e3fd57 Compare August 14, 2024 14:44

jimmyzhai requested a review from r12f August 21, 2024 09:02

r12f reviewed Aug 22, 2024

View reviewed changes

dash-pipeline/bmv2/dash_headers.p4 Outdated Show resolved Hide resolved

jimmyzhai force-pushed the dp_app_add_flow_in_pipeline branch 3 times, most recently from 65e401d to 7a721be Compare August 29, 2024 02:57

jimmyzhai added 4 commits August 29, 2024 03:15

Implement u8listSetVal in SAI/src/utils.h

07e51b9

Base on PR524, add flow involved packet path in pipeline:

56099bb

1. Update flow table 2. Add dash headers towards DPAPP

Add flow state FSM

735147e

1. remove the use of DPAPP_CONNTRACK

e939e3d

2. refactor dash_ingress

jimmyzhai force-pushed the dp_app_add_flow_in_pipeline branch from 7a721be to e939e3d Compare August 29, 2024 03:16

r12f reviewed Aug 30, 2024

View reviewed changes

jimmyzhai added 7 commits August 30, 2024 13:00

Manually merge sonic-net#616 and sonic-net#617 changes in bmv2

46ae92a

Merge remote-tracking branch 'sonic-net/main' into dp_app_add_flow_in…

991ef88

…_pipeline

Still use bulk_get_session_server_ip, bulk_get_session_server_port

ab45aaf

Merge remote-tracking branch 'sonic-net/main' into dp_app_add_flow_in…

2707d58

…_pipeline

Fix per comments

7389540

redefine encap_data_t -> meta_encap_data_t and

68c7deb

overlay_rewrite_data_t -> meta_overlay_rewrite_data_t, as header in struct is not well supported for target dpdk-pna.

r12f reviewed Sep 3, 2024

View reviewed changes

KrisNey-MSFT removed the Ready to merge? label Sep 4, 2024

jimmyzhai force-pushed the dp_app_add_flow_in_pipeline branch 2 times, most recently from a3b9cf2 to 0045672 Compare September 9, 2024 17:28

Revert set_flow_entry_attr params for flow SAI backward compatible

73256bc

jimmyzhai force-pushed the dp_app_add_flow_in_pipeline branch from 0045672 to 73256bc Compare September 10, 2024 01:34

Merge remote-tracking branch 'sonic-net/main' into dp_app_add_flow_in…

a57ff8b

…_pipeline

jimmyzhai added 5 commits October 18, 2024 06:13

Implement flow actions, being same as routing actions

bceb710

Rename encap_data -> u0_encap_data, tunnel_data -> u1_encap_data

443f27f

Rollback the definition of enable_reverse_tunnel_learning for all tar…

590d9fb

…gets

flow sync state is not in flow_data

2cf4047

Add param reverse_flow_dst_ip_is_v6 for flow action

c6bc6fc

r12f reviewed Oct 23, 2024

View reviewed changes

jimmyzhai added 3 commits October 24, 2024 01:55

Use concrete SNAT/DNAT to replace NAT in flow actions

cb6d2bf

Merge remote-tracking branch 'sonic-net/main' into dp_app_add_flow_in…

421704e

…_pipeline

update dash_tunnel.yaml

ecec348

r12f approved these changes Oct 24, 2024

View reviewed changes

zhixiongniu reviewed Oct 24, 2024

View reviewed changes

Correct push_vxlan_tunnel to push_nvgre_tunnel for dash_encapsulation…

4c3bdf9

…_t.NVGRE

zhixiongniu approved these changes Oct 25, 2024

View reviewed changes

r12f merged commit c3fd153 into sonic-net:main Oct 26, 2024
5 checks passed

This was referenced Oct 28, 2024

[dash-flow] Test cases for DASH flow using PTF #621

Open

[Draft] [dash-flow] Implementation of dash flow actions for bmv2 #618

Closed

jimmyzhai mentioned this pull request Oct 30, 2024

[dpapp] Initial dpapp implementation being a vpp plugin #609

Merged

jimmyzhai deleted the dp_app_add_flow_in_pipeline branch November 21, 2024 02:05

[dpapp] Add flow in bmv2 pipeline #608

[dpapp] Add flow in bmv2 pipeline #608

Conversation

jimmyzhai commented Jul 23, 2024

KrisNey-MSFT commented Jul 30, 2024

KrisNey-MSFT commented Aug 6, 2024

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

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

r12f left a 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

r12f commented Oct 26, 2024