DPDK backend: Add support for meter and counter extern #2813

usha1830 · 2021-06-22T11:48:52Z

This PR adds support for meter and counter PSA extern in DPDK backend and a minor change to fix jump statements in action body.

DPDK target's requirements

Current PSA Meter extern API does not allow the user the specify a packet length, but DPDK target expects the packet length to come from the user. To handle this, Meter execute method API is customized to take an additional optional parameter for packet length.

PSA_MeterColor_t execute(in S index, in PSA_MeterColor_t color, @optional in bit<32> pkt_len);

Similarly, for PSA counter extern, packet length is required for PACKET counter. The Counter count method API is customized to take additional optional parameter for packet length.

void count(in S index, @optional in bit<32> increment);

DPDK target implements counter using register. Register declaration "regarray" instruction is used for counter declaration and atomic addition of registers "regadd" is used for counter incrementing. The packet counters are incremented by length specified by the user as argument to count method and byte counters are incremented by 1.

1) Add missing regarray declarations 2) Fix read() method implementation

…n objects and methods in/from control blocks only

- Disabled Midend predication pass to allow conditional operator to be splitted into if-else statement - Added a new test and reference output for conditional operator

…ce output

…830/p4c into ushag_conditional_operator

…ement. Fixed few typos

mihaibudiu · 2021-06-22T17:55:57Z

backends/dpdk/dpdkArch.h

+        : met_map(met_map) , typeMap(typeMap) {}
+
+    bool preorder(const IR::Declaration_Instance *d) override {
+        if (d->type->is<IR::Type_Specialized>()) {


these can be combined into a single line

mihaibudiu · 2021-06-22T17:56:06Z

backends/dpdk/dpdkArch.h

+        if (d->type->is<IR::Type_Specialized>()) {
+            auto type = d->type->to<IR::Type_Specialized>();
+            auto externTypeName = type->baseType->path->name.name;
+            if (externTypeName == "Meter"){


no space after (

mihaibudiu · 2021-06-22T17:57:06Z

backends/dpdk/dpdkArch.h

+                if (d->arguments->size() != 2) {
+                    ::error("%1%: expected number of meters and type of meter as arguments", d);
+                } else {
+                    if (d->arguments->at(1) == 0)


Do you really want to compare with 0? Not with IR::Constant(0)?

Perhaps you want to add a test for this case.

I am facing an issue w.r.t adding a test for this case.

I added a test in testdata/p4_16_samples/ for emitting this warning and added the warning in reference stderr output "psa-example-dpdk-meter1.p4-stderr". This test compiles as expected and produces a warning when compiled with p4c-dpdk. However, make check fails while compiling this test using p4test. It fails because of output mismatch as there is no warning when compiling with p4test and reference stderr output has warning. What should I do in this case?

So far the p4 reference outputs have always been produced by p4test.
Is the dpdk backend testing framework also saving reference outputs?
Ideally it should only save the .spec output. Then there should be no conflict: the .p4 and -stderr outputs come from p4test, and the .spec output from p4c-dpdk.
If both compilers produce reference outputs we have a second problem, since the test has an inherent data race. When we run tests on multiple cores one test could overwrite the reference outputs files of the other one while they are running. So this should not be done. The likelihood of this happening is low, since we have many tests, but it's not zero.
Finally, note that we have already this problem where a test should pass with p4test and fail with a specific backend which has more constraints. Then the solution is to list the test as xfail into the makefile of the backend where it is supposed to fail, with comments explaining why.

mihaibudiu · 2021-06-22T17:58:06Z

backends/dpdk/dpdkArch.h

+            auto externTypeName = type->baseType->path->name.name;
+            if (externTypeName == "Counter"){
+                if (d->arguments->size() != 2 ) {
+                    ::error("%1%: expected n_counters and counter type as arguments", d);


this error message does not seem very clear to me.

mihaibudiu · 2021-06-22T17:58:39Z

backends/dpdk/dpdkArch.h

-class AddRegisterDeclaration : public PassManager {
+/* This pass collects PSA counter declaration instances and push them to a map
+ * for emitting to the .spec file later */
+class CollectCounterDeclaration : public Inspector {


Can't you do both of them in a single traversal? There is a lot of common code.

mihaibudiu · 2021-06-22T17:59:21Z

backends/dpdk/dpdkHelpers.cpp

+                                e->object->getName());
+                    const IR::Expression *color_in = nullptr;
+                    const IR::Expression *length = nullptr;
+                    auto index = (*e->expr->arguments)[0]->expression;


I think using arguments->at(0) is safer.

backends/dpdk/dpdkHelpers.cpp

mihaibudiu · 2021-06-22T18:01:59Z

backends/dpdk/spec.cpp

+        auto args = this->arguments;
+        unsigned value = 0;
+        if (args->size() < 2) {
+          ::error ("Counter extern declaration %1% must contain 2 parameters\n", this->Name());


your indentation is not consistent with our standard

mihaibudiu · 2021-06-22T18:02:51Z

p4include/dpdk/psa.p4

@@ -0,0 +1,863 @@
+/* Copyright 2013-present Barefoot Networks, Inc.


Is this psa.p4 different from the other one?
If not, does it have to be here?

Yes. This is slightly different from the standard psa.p4 in p4include directory.

This psa.p4 extends the API for Meter execute and Counter count methods. A new optional parameter is added to both of these methods to take packet length/counter increment value as a parameter.

Meter extern's execute method:
PSA_MeterColor_t execute(in S index, in PSA_MeterColor_t color, @optional in bit<32> pkt_len);

Counter extern's count method
void count(in S index, @optional in bit<32> increment);

This API is required to meet DPDK target's requirements.

Please also refer to #2751 and #2818 PRs which proposes similar extensions to PSA

I find this very confusing. The whole point of PSA is to be portable. If we have two different PSA.p4 I don't see how portability is helped. Only confusion can ensue.
We should find a resolution to the issues you cite before merging this file.
I would expect that extensions can be done by including psa.p4 and then additional libraries. But the externs in these libraries would have to have different names.
In the meantime we could perhaps use a different name for this file.

What is the relation of this file and the one from #2818?

usha1830 · 2021-06-23T07:24:53Z

@mbudiu-vmw Thanks for the review. I will address rest of the comments and resubmit the changes.

… file outputs

Added new test for using multiple externs in same P4 program

usha1830 · 2021-06-28T05:08:36Z

@mbudiu-vmw I have completed the requested changes. Could you please review it again?

Combined the extern declaration collection pass for Register, Counters and meters.
Added one more test for using multiple externs in same example.
Updated the dpdk test script to avoid saving error and P4 runtime outputs.

Regarding the customization of psa.p4, can we conclude on going with the proposed approach of having customized psa.p4 in target specific include directory (similar to #2818)?

mihaibudiu

I am accepting this, but I still have a couple of questions. Please take a look.

mihaibudiu · 2021-06-28T21:12:59Z

backends/dpdk/dpdkHelpers.cpp

-                    new IR::DpdkMeterExecuteStatement(meter, index, color));
+                // DPDK target requires the result of meter execute method is assigned to a
+                // variable of PSA_MeterColor_t type.
+                ::error(ErrorType::ERR_UNSUPPORTED, "LHS of meter execute statement is missing " \


no return here?

I don't think so, there is already a return just after the if-else chain.

mihaibudiu · 2021-06-28T21:15:32Z

p4include/dpdk/psa.p4

@@ -0,0 +1,863 @@
+/* Copyright 2013-present Barefoot Networks, Inc.


What is the relation of this file and the one from #2818?

usha1830 · 2021-06-29T11:52:50Z

What is the relation of this file and the one from #2818?

Both this PR and #2818 PR add the same new file "dpdk/psa.p4".

This new file is derived from the top-level psa.p4. This PR extends the top-level psa.p4 by modifying the signatures for Meter execute and Counter count methods.

The PR #2818 modifies the DPDK target data plane bit widths.

Both of these customized psa.p4 files should be merged.

* DPDK backend: Add support for meter and counter PSA externs

usha1830 added 28 commits April 22, 2021 00:28

Fix dpdk regression failure: Duplicates declaration

a805208

Fix cpplint errors

611261b

Added checks to ensure parameters exist before accessing them

9c8e6d2

Merge branch 'master' into ushag/fix_duplicates_declaration

b184e0b

Merge remote-tracking branch 'upstream/master'

c03e07d

Merging with p4lang/p4c repo main branch

c99d261

Merge branch 'p4lang:main' into main

c6a5c12

Fix PSA extern Register's implementation

4817ea2

1) Add missing regarray declarations 2) Fix read() method implementation

Updated spec.cpp as per review comments

bfb6f02

Emit a warning if Register instantiation is found outside control block

f5cfb35

Fixed a typo

5ca47b1

Remove restriction for instantiation and invocation of register exter…

f0c7f9c

…n objects and methods in/from control blocks only

Merge branch 'p4lang:main' into ushag_conditional_operator

f3ab6ad

Conditional operator support for P4C-DPDK.

778cd74

- Disabled Midend predication pass to allow conditional operator to be splitted into if-else statement - Added a new test and reference output for conditional operator

Merge branch 'p4lang:main' into ushag_conditional_operator

2b992a4

Changing ternary match to exact in test program, updating the referen…

61567fd

…ce output

Merge branch 'ushag_conditional_operator' of https://github.com/usha1…

009fd24

…830/p4c into ushag_conditional_operator

Merge branch 'p4lang:main' into ushag_conditional_operator

afa8764

Added support for range and mask operations in transition select stat…

05a68ba

…ement. Fixed few typos

Merge branch 'p4lang:main' into origin/ushag/mask_range

d43963c

Support mask operations in dpdk backend (#1)

8b9927f

Remove pass for inserting temporary mask variables

3bde5de

Merge branch 'p4lang:main' into origin/ushag/mask_range

f7cb04d

update reference output

a3e6ef6

Merge branch 'p4lang:main' into origin/ushag/mask_range

6b963ce

Address review comments

8cf5882

Merge branch 'p4lang:main' into origin/ushag/mask_range

00ce76f

Merge branch 'p4lang:main' into origin/ushag/mask_range

32d2abe

mihaibudiu requested changes Jun 22, 2021

View reviewed changes

DPDK backend: Add support for meter and counter PSA externs

7986213

usha1830 added 5 commits June 24, 2021 07:45

Address review comments for Meter and Counter extern support

9648cce

Merge branch 'p4lang:main' into ushag/Meter_counter

d663dbb

Cleanup dpdk regression test script to avoid saving error and runtime…

76a0557

… file outputs

Updated reference output

402ae06

Simpilfied the implementation to use vector instead of map

0013778

Added new test for using multiple externs in same P4 program

usha1830 requested a review from mihaibudiu June 28, 2021 05:00

mihaibudiu approved these changes Jun 28, 2021

View reviewed changes

mihaibudiu merged commit 64eb44d into p4lang:main Jun 29, 2021

mihaibudiu mentioned this pull request Jun 29, 2021

Support target-specific customizations of psa.p4 include file #2818

Merged

usha1830 deleted the ushag/Meter_counter branch July 7, 2021 08:21

usha1830 added a commit to usha1830/p4c that referenced this pull request Aug 7, 2021

DPDK backend: Add support for meter and counter extern (p4lang#2813)

ce44b6f

* DPDK backend: Add support for meter and counter PSA externs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DPDK backend: Add support for meter and counter extern #2813

DPDK backend: Add support for meter and counter extern #2813

usha1830 commented Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

usha1830 Jun 23, 2021

mihaibudiu Jun 23, 2021 •

edited

Loading

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

mihaibudiu Jun 22, 2021

usha1830 Jun 23, 2021

usha1830 Jun 23, 2021

mihaibudiu Jun 23, 2021

mihaibudiu Jun 28, 2021

usha1830 commented Jun 23, 2021

usha1830 commented Jun 28, 2021

mihaibudiu left a comment

mihaibudiu Jun 28, 2021

usha1830 Jun 29, 2021

mihaibudiu Jun 28, 2021

usha1830 commented Jun 29, 2021

		@@ -0,0 +1,863 @@
		/* Copyright 2013-present Barefoot Networks, Inc.

DPDK backend: Add support for meter and counter extern #2813

DPDK backend: Add support for meter and counter extern #2813

Conversation

usha1830 commented Jun 22, 2021

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

mihaibudiu Jun 23, 2021 • 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

usha1830 commented Jun 23, 2021

usha1830 commented Jun 28, 2021

mihaibudiu 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

usha1830 commented Jun 29, 2021

mihaibudiu Jun 23, 2021 •

edited

Loading