create dpdk specific pna.p4 and extend it #3658

kamleshbhalui · 2022-11-03T04:32:54Z

dpdk target requires packet length in count and execute methods part of Counter and Meter externs.

usha1830 · 2022-11-03T05:03:27Z

jafingerhut · 2022-11-03T13:51:14Z

p4include/dpdk/psa.p4

@@ -533,7 +533,8 @@ enum PSA_CounterType_t {
 @noWarn("unused")
 extern Counter<W, S> {
  Counter(bit<32> n_counters, PSA_CounterType_t type);
-  void count(in S index, @optional in bit<32> increment);
+  void count(in S index);


What does DPDK do when a PSA program calls the original count method that does not have a pkt_len parameter? Nothing? It would be good to have a comment here if it does anything other than what PSA defines.

And also for the corresponding method in the DirectCounter extern. And for Meter and DirectMeter.

added the comments

jafingerhut · 2022-11-03T13:53:43Z

testdata/p4_16_samples/pna-dpdk-bvec_union.p4

@@ -1,5 +1,5 @@
 #include <core.p4>
-#include <pna.p4>
+#include <dpdk/pna.p4>


For PSA, I thought we had something in the p4c-dpdk front end script that made it automatically look in the dpdk directory for the psa.p4 include file, so that P4 developers could simply write #include <psa.p4> and it would find it in the p4include/dpdk directory. It would be nice to do the same for p4c-dpdk for PNA architecture programs.

yes it only works for p4c-dpdk but not with p4test. When running tests(make check) every testfiles compiles with p4test and p4c-dpdk.

usha1830 · 2022-11-08T15:13:37Z

testdata/p4_16_samples_outputs/pna-dpdk-direct-meter-learner.p4.spec

@@ -33,7 +33,6 @@ struct main_metadata_t {
 	bit<8> local_metadata_timeout
 	bit<32> local_metadata_port_out
 	bit<32> pna_main_output_metadata_output_port
-	bit<32> table_entry_index


This is a valid test for Direct Meter for tables with add_on_miss. These instructions/metadata field shouldn't be removed

usha1830 · 2022-11-08T15:18:27Z

backends/dpdk/dpdkArch.cpp

@@ -2526,7 +2526,7 @@ bool CollectDirectCounterMeter::preorder(const IR::P4Table* tbl) {
                        return false;
                    }
                }
-                if (!ifMethodFound(defaultActionDecl, "execute", meterExternName)) {
+                if (!ifMethodFound(defaultActionDecl, "dpdk_execute", meterExternName)) {
                    if (meterInstance) {
                        ::error(ErrorType::ERR_EXPECTED, "Expected default action %1% to have "
                                "'execute' method call for DirectMeter extern instance %2%",


'execute' ->'dpdk_execute'

Same name change is required in P4Action preorder

Looks fine to me now

jafingerhut · 2022-11-10T04:35:53Z

testdata/p4_16_samples_outputs/psa-example-dpdk-meter-execute-err.p4

+
+IngressPipeline(IngressParserImpl(), ingress(), IngressDeparserImpl()) ip;
+EgressPipeline(EgressParserImpl(), egress(), EgressDeparserImpl()) ep;
+PSA_Switch(ip, PacketReplicationEngine(), ep, BufferingQueueingEngine()) main;


Why is the expected output file testdata/p4_16_samples_outputs/psa-example-dpdk-meter-execute-err.p4-stderr empty? I thought these changes claimed to give an error message if you tried to call the execute method without a packet length parameter? Doesn't it?

@jafingerhut
Once #3680 is merged, we will move this test to p4_16_dpdk_errors directory and then the error output will also be fixed. Currently, it is added in XFAIL as all other negative tests

sounds good

jafingerhut · 2022-11-10T04:37:33Z

backends/dpdk/README.md

@@ -74,6 +74,13 @@ In P4 the second argument of the extract method is the number of bits.
 Compiler generates instructions which compute the size in bytes from the value in bits.
 If the value in bits is not a multiple of 8, the value is rounded down to the lower
 multiple of 8 bits.
+- Currently dpdk target does not support standard count and execute methods for Counter and Meter externs as defined in PSA and PNA specifications. It requires packet length as parameter in count and execute methods.
+```Meter


Does DPDK development have any plans in the future to support the standard count and execute methods, without a packet length parameter?

There is no plan in the near future. There was an earlier discussion of providing ethernet frame size as default packet length, however it seems that it wouldn't be correct as DPDK implements trCM which do not work on ethernet frame length.

"which do not work on ethernet frame length" Meters/policers can work on whatever length in bytes you give for a packet, whether it matches the packet's length or not, so it isn't clear to me what your statement means. If DPDK implements meters by default with ethernet frame length, then I would recommend enabling them to do so, and DOCUMENTING that the methods that do not take a packet length as input, use the ethernet frame length in bytes. If they work as documented, then I'd say that they work :-)

I will take this up with the DPDK target team again and see if they can have this in their worklist. We can enable the p4c support when we have the default packet length available from the target side.

jfingerh

With the future plans for additional changes later mentioned in comments, this looks good to me.

jafingerhut

LGTM Adding review from my Github account that has write permission this time.

as required by dpdk target

kamleshbhalui · 2022-11-11T09:00:46Z

@jfingerh please merge it.

jafingerhut reviewed Nov 3, 2022

View reviewed changes

kamleshbhalui force-pushed the count_meter_dpdk branch from 0dda52c to c91c47f Compare November 8, 2022 08:51

kamleshbhalui requested a review from jafingerhut November 8, 2022 11:29

usha1830 reviewed Nov 8, 2022

View reviewed changes

kamleshbhalui force-pushed the count_meter_dpdk branch from 70673df to c307557 Compare November 10, 2022 03:42

kamleshbhalui requested review from usha1830 and jafingerhut and removed request for jafingerhut and usha1830 November 10, 2022 03:42

kamleshbhalui force-pushed the count_meter_dpdk branch from c307557 to 47f8274 Compare November 10, 2022 04:07

jafingerhut reviewed Nov 10, 2022

View reviewed changes

kamleshbhalui force-pushed the count_meter_dpdk branch from 47f8274 to e7f10e0 Compare November 10, 2022 08:57

jfingerh approved these changes Nov 10, 2022

View reviewed changes

jafingerhut approved these changes Nov 10, 2022

View reviewed changes

kamleshbhalui added 8 commits November 10, 2022 21:51

Add dpdk specific pna.p4 and change methods like count and execute

50a23ce

as required by dpdk target

change names to resolve ambiguity

5a8b2d4

rename execute to dpdk_execute

4f1c8fa

Added test and updated readme

e22a36d

update tests

0e8b523

added comments and improve readme

37d5b9e

Added comments in pna.p4 and psa.p4

dab1b25

rebased with main

5f8b648

kamleshbhalui added 3 commits November 10, 2022 21:51

fixed and update testcases

7ddb1c4

replace execute with dpdk_execute

c2a2dc8

rebased and updated tests

cf29965

kamleshbhalui force-pushed the count_meter_dpdk branch from e7f10e0 to cf29965 Compare November 11, 2022 07:44

jafingerhut merged commit b7f0d13 into p4lang:main Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create dpdk specific pna.p4 and extend it #3658

create dpdk specific pna.p4 and extend it #3658

kamleshbhalui commented Nov 3, 2022 •

edited

Loading

usha1830 commented Nov 3, 2022

jafingerhut Nov 3, 2022

jafingerhut Nov 3, 2022

kamleshbhalui Nov 8, 2022

jafingerhut Nov 3, 2022

kamleshbhalui Nov 8, 2022

usha1830 Nov 8, 2022

kamleshbhalui Nov 10, 2022

usha1830 Nov 8, 2022

usha1830 Nov 8, 2022

kamleshbhalui Nov 10, 2022

usha1830 Nov 10, 2022

jafingerhut Nov 10, 2022

usha1830 Nov 10, 2022

jfingerh Nov 10, 2022

jafingerhut Nov 10, 2022

usha1830 Nov 10, 2022

jfingerh Nov 10, 2022

usha1830 Nov 11, 2022

jfingerh left a comment

jafingerhut left a comment

kamleshbhalui commented Nov 11, 2022

create dpdk specific pna.p4 and extend it #3658

create dpdk specific pna.p4 and extend it #3658

Conversation

kamleshbhalui commented Nov 3, 2022 • edited Loading

usha1830 commented Nov 3, 2022

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

jfingerh left a comment

Choose a reason for hiding this comment

jafingerhut left a comment

Choose a reason for hiding this comment

kamleshbhalui commented Nov 11, 2022

kamleshbhalui commented Nov 3, 2022 •

edited

Loading