PacketOut based on L2 header #40

pudelkoM · 2020-08-11T02:34:01Z

This PR makes the program Tofino chip model independent by removing the dependency on the cpu port at compile time.
Instead this information is inserted at runtime by the controller into the switch_info table, and stored in the metadata for usage in the egress stages. To remove the dependency in the ingress parser (to detect PacketOuts from dataplane packets), we now use a special Ethertype in the serialized metadata header of PacketOuts.

TODOs:

Add init code for switch config (cpu port) in pipeliner
Fix PTF tests

p4src/include/header.p4

p4src/include/define.p4

robertmacdavid

My only nit is that I would prefer the 16 bit egress_port be split into 9 port bits and 7 padding bits.

p4src/include/control/packetio.p4

p4src/include/parser.p4

p4src/include/define.p4

ccascone · 2020-08-12T23:25:28Z

Added WIP label, as this cannot be merged until the Java part is updated as well.

bfrt returns error when trying to install clone groups for quad-pipe ports on a dual-pipe chip. For now, we decide whether to install 4 or 2 sessions based on the pipeconf name. This is a temporary workaround, as it prevents us from using chip-independent pipeconfs (#40). Ideally, we should be able to use gNMI to retrieve the chip type or platform name.

p4src/include/control/packetio.p4

ghost · 2020-08-18T01:16:08Z

Can one of the admins verify this patch?

p4src/include/header.p4

p4src/include/control/packetio.p4

p4src/include/header.p4

src/main/java/org/stratumproject/fabric/tna/behaviour/pipeliner/FabricPipeliner.java

p4src/include/define.p4

p4src/include/int/int.p4

pudelkoM · 2020-09-10T21:38:38Z

These PTF test currently fail:

The following tests failed:
FabricIpv4UnicastLoopbackModeTest, FabricPacketInLoopbackModeTest, FabricPacketOutLoopbackModeTest

p4src/include/control/packetio.p4

p4src/include/parser.p4

ptf/tests/ptf/fabric_test.py

p4src/include/parser.p4

pudelkoM · 2020-09-11T17:36:06Z

p4src/include/header.p4

 @controller_header("packet_out")
 header packet_out_header_t {
    PortId_t          egress_port;
    CpuLoopbackMode_t cpu_loopback_mode;
-    bit<5>            _pad0;
+    @padding bit<85>  pad0;


Adding a _ prefix excludes the parameter from the auto-generated java constants.

Include padding fields in PacketOuts Address comments Remove last instances of CPU_PORT from P4 code Move tmp variable to inner scope Remove unncessary skip assignment Add padding fields when creating metadata [DO NOT MERGE] Add metadata flag to detect to cpu in egress. This code does not work, as it can not distinguish between the orginal packet in case of clone_to_cpu or multicast groups with instance 0. Remove unnecessary check on CPU_PORT in INT control Integrate with loopback header in parser Fix parse_packet_out next transition typo Handle cpu ports with a keyless table populated at runtime Move switch info table to egress pipeline Fix createPacketMetadata Fix PacketOut tests Initialize the switch info table on module init Remove header size split workaround for Stratum Remove padding Add missing application ID and priority Remove app id constant Remove unnecessary parser args Remove unneeded constants Small fixes Fix pipeconf for new packetout header format Initialize cpu_port in egress metadata to zero Rename ETHERTYPE_PACKET_IO to ETHERTYPE_PACKET_OUT Use braindamage (hungarian) notation for variable names

src/main/java/org/stratumproject/fabric/tna/behaviour/pipeliner/FabricPipeliner.java

ptf/tests/ptf/fabric_test.py

codecov · 2020-09-18T21:15:03Z

Codecov Report

Merging #40 into master will decrease coverage by 0.64%.
The diff coverage is 34.37%.

@@             Coverage Diff              @@
##             master      #40      +/-   ##
============================================
- Coverage     66.90%   66.26%   -0.65%     
  Complexity      199      199              
============================================
  Files            17       17              
  Lines          1538     1571      +33     
  Branches        122      122              
============================================
+ Hits           1029     1041      +12     
- Misses          427      448      +21     
  Partials         82       82

Impacted Files	Coverage Δ	Complexity Δ
...abric/tna/behaviour/pipeliner/FabricPipeliner.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...roject/fabric/tna/behaviour/FabricInterpreter.java	`75.78% <100.00%> (+0.78%)`	`16.00 <0.00> (ø)`
...mproject/fabric/tna/behaviour/P4InfoConstants.java	`100.00% <100.00%> (ø)`	`1.00 <0.00> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7693be...02bc4fe. Read the comment docs.

ccascone · 2020-09-18T22:07:24Z

ptf/tests/ptf/fabric_test.py

@@ -44,6 +44,7 @@
 ETH_TYPE_PPPOE = 0x8864
 ETH_TYPE_MPLS_UNICAST = 0x8847

+ETH_TYPE_PACKET_IO = 0xBF01


Suggested change

ETH_TYPE_PACKET_IO = 0xBF01

ETHERTYPE_PACKET_OUT = 0xBF01

What's the reason to go against all other instances that use the ETH_TYPE_* naming scheme?

Ah, this about IO vs OUT

ccascone · 2020-09-18T22:08:54Z

src/main/java/org/stratumproject/fabric/tna/behaviour/FabricInterpreter.java

@@ -53,6 +53,7 @@
    private static final int CPU_LOOPBACK_MODE_DISABLED = 0;
    private static final int CPU_LOOPBACK_MODE_DIRECT = 1;
    private static final int CPU_LOOPBACK_MODE_INGRESS = 2;
+    private static final int ETHER_TYPE_PACKET_IO = 0xBF01;


Suggested change

private static final int ETHER_TYPE_PACKET_IO = 0xBF01;

private static final int ETHERTYPE_PACKET_OUT = 0xBF01;

ccascone · 2020-09-18T22:27:01Z

@pudelkoM while we wait for Jenkins to approve, and before we can merge, it would be nice if you could update the PR description with a more meaningful description of the change. Thanks!

Makes the P4 program Tofino chip model independent by removing the dependency on the cpu port at compile time. Instead this information is inserted at runtime by the controller into the switch_info table, and stored in the metadata for usage in the egress stages. To remove the dependency in the ingress parser (to detect PacketOuts from dataplane packets), we now use a special Ethertype in the serialized metadata header of PacketOuts.

ccascone requested a review from robertmacdavid August 11, 2020 04:35

ccascone reviewed Aug 11, 2020

View reviewed changes

p4src/include/header.p4 Outdated Show resolved Hide resolved

p4src/include/define.p4 Outdated Show resolved Hide resolved

ccascone requested a review from abhilashendurthi August 11, 2020 04:57

robertmacdavid approved these changes Aug 11, 2020

View reviewed changes

pudelkoM force-pushed the packet-out-l2-header branch 2 times, most recently from 329bcc3 to d86274f Compare August 11, 2020 18:18

abhilashendurthi approved these changes Aug 12, 2020

View reviewed changes

pudelkoM commented Aug 12, 2020

View reviewed changes

p4src/include/control/packetio.p4 Outdated Show resolved Hide resolved

ccascone reviewed Aug 12, 2020

View reviewed changes

p4src/include/control/packetio.p4 Outdated Show resolved Hide resolved

p4src/include/parser.p4 Outdated Show resolved Hide resolved

p4src/include/define.p4 Outdated Show resolved Hide resolved

pudelkoM changed the title ~~[WIP] PacketOut based on L2 header~~ PacketOut based on L2 header Aug 12, 2020

ccascone added the WIP Do not merge! label Aug 12, 2020

pudelkoM mentioned this pull request Aug 12, 2020

[DO NOT MERGE] Attempt at chip-independent fabric-tna #23

Closed

pudelkoM commented Aug 15, 2020

View reviewed changes

p4src/include/control/packetio.p4 Outdated Show resolved Hide resolved

Base automatically changed from int-support-report-inner-flow to master August 17, 2020 06:39

pudelkoM mentioned this pull request Aug 18, 2020

[DO NOT MERGE] Add platform specific defines that are also exposed in p4info #48

Closed

Yi-Tseng reviewed Aug 18, 2020

View reviewed changes

p4src/include/header.p4 Outdated Show resolved Hide resolved

pudelkoM force-pushed the packet-out-l2-header branch from e77afa7 to ff2abc9 Compare August 18, 2020 17:37

pudelkoM commented Aug 19, 2020

View reviewed changes

p4src/include/control/packetio.p4 Outdated Show resolved Hide resolved

pudelkoM force-pushed the packet-out-l2-header branch from addaf66 to d3429d1 Compare August 19, 2020 00:24

ccascone reviewed Aug 19, 2020

View reviewed changes

p4src/include/control/packetio.p4 Outdated Show resolved Hide resolved

stratum deleted a comment from ccascone Aug 19, 2020

pudelkoM commented Aug 26, 2020

View reviewed changes

p4src/include/header.p4 Outdated Show resolved Hide resolved

ccascone requested changes Sep 3, 2020

View reviewed changes

src/main/java/org/stratumproject/fabric/tna/behaviour/pipeliner/FabricPipeliner.java Outdated Show resolved Hide resolved

src/main/java/org/stratumproject/fabric/tna/behaviour/pipeliner/FabricPipeliner.java Outdated Show resolved Hide resolved

pudelkoM force-pushed the packet-out-l2-header branch from 2f0cf7e to 298089b Compare September 4, 2020 19:32

pudelkoM commented Sep 9, 2020

View reviewed changes

p4src/include/define.p4 Outdated Show resolved Hide resolved

pudelkoM force-pushed the packet-out-l2-header branch from 0a43161 to 998b90b Compare September 10, 2020 21:13

pudelkoM commented Sep 10, 2020

View reviewed changes

p4src/include/int/int.p4 Show resolved Hide resolved

ccascone reviewed Sep 10, 2020

View reviewed changes

p4src/include/control/packetio.p4 Show resolved Hide resolved

p4src/include/parser.p4 Outdated Show resolved Hide resolved

ptf/tests/ptf/fabric_test.py Outdated Show resolved Hide resolved

pudelkoM commented Sep 11, 2020

View reviewed changes

p4src/include/parser.p4 Show resolved Hide resolved

pudelkoM commented Sep 11, 2020

View reviewed changes

ccascone mentioned this pull request Sep 16, 2020

Missing metadata in packet-out #68

Closed

pudelkoM force-pushed the packet-out-l2-header branch from 1767a54 to 39c588d Compare September 17, 2020 01:27

Yi-Tseng reviewed Sep 17, 2020

View reviewed changes

src/main/java/org/stratumproject/fabric/tna/behaviour/pipeliner/FabricPipeliner.java Outdated Show resolved Hide resolved

ptf/tests/ptf/fabric_test.py Outdated Show resolved Hide resolved

ptf/tests/ptf/fabric_test.py Outdated Show resolved Hide resolved

pudelkoM added 4 commits September 17, 2020 11:45

Address comments

0c018c4

Remove extraneous @tvsetup and @autocleanup annotations

773372a

Add explicit nop action to switch_info table

77b09ee

Set up and clear switch_info table manually

221cd16

ccascone requested changes Sep 18, 2020

View reviewed changes

Rename PACKET_IO to PACKET_OUT

bae413d

ccascone removed the WIP Do not merge! label Sep 18, 2020

Move switch info table init to PacketIn test run function

02bc4fe

ccascone approved these changes Sep 21, 2020

View reviewed changes

ccascone merged commit 02f21ec into master Sep 21, 2020

ccascone deleted the packet-out-l2-header branch September 21, 2020 23:49

ccascone mentioned this pull request Sep 28, 2020

Update ONOS behaviors to work with chip-independent pipeconf #87

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PacketOut based on L2 header #40

PacketOut based on L2 header #40

pudelkoM commented Aug 11, 2020 •

edited

Loading

robertmacdavid left a comment

ccascone commented Aug 12, 2020

ghost commented Aug 18, 2020

pudelkoM commented Sep 10, 2020

pudelkoM Sep 11, 2020

codecov bot commented Sep 18, 2020 •

edited

Loading

ccascone Sep 18, 2020

pudelkoM Sep 18, 2020

pudelkoM Sep 18, 2020

ccascone Sep 18, 2020

ccascone commented Sep 18, 2020

	private static final int ETHER_TYPE_PACKET_IO = 0xBF01;
	private static final int ETHERTYPE_PACKET_OUT = 0xBF01;

PacketOut based on L2 header #40

PacketOut based on L2 header #40

Conversation

pudelkoM commented Aug 11, 2020 • edited Loading

robertmacdavid left a comment

Choose a reason for hiding this comment

ccascone commented Aug 12, 2020

ghost commented Aug 18, 2020

pudelkoM commented Sep 10, 2020

pudelkoM Sep 11, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 18, 2020 • edited Loading

Codecov Report

ccascone Sep 18, 2020

Choose a reason for hiding this comment

pudelkoM Sep 18, 2020

Choose a reason for hiding this comment

pudelkoM Sep 18, 2020

Choose a reason for hiding this comment

ccascone Sep 18, 2020

Choose a reason for hiding this comment

ccascone commented Sep 18, 2020

pudelkoM commented Aug 11, 2020 •

edited

Loading

codecov bot commented Sep 18, 2020 •

edited

Loading