DPDK Backend: Direct counter and direct meter support #3631

usha1830 · 2022-10-28T04:54:38Z

This PR adds support for DirectCounter and DirectMeter extern in .spec file and context json output files for dpdk compiler

In the spec file

For direct counter , a new regarray is declared for direct counter extern with size equal to the owner table size + 1 for regular tables and table_size *4 + 1 for learner tables.
Similarly, for direct meter, a new metarray is declared for direct counter extern with size equal to the owner table size + 1 for regular tables and table_size *4 + 1 for learner tables.

For the count method of direct counter, the following instructions should be emitted.

entryid m.table_entry_index
regadd per_prefix_pkt_bytes_count_0_packets m.table_entry_index 1

In case of PACKETS_AND_BYTES counter, two regarrays should be emitted with the counter name suffixed with "_packets" and "_bytes"
Similarly, the count method also should have "regadd" instruction for packets and bytes counter.

For the execute method of direct meter, the following instructions should be emitted.

entryid m.table_entry_index
meter <metername> m.table_entry_index <packet_length> <color_in> <color_out>

Following tests are added:

Valid case for regular table using Direct Counter
Valid case for regular table using Direct Meter
Valid case for learner table using Direct Counter
Valid case for learner table using Direct Meter
Error cases for coexistence of action selector and direct counter/direct meter
Error for default action not having count/execute method
Error for calling count/execute of multiple extern instances in same action
Error for calling count/execute from non-owner table action
Error for using Direct Meter/Direct Counter extern with tables having wildcard/lpm match kind
Error for calling count/execute from apply block

jfingerh · 2022-10-28T05:28:48Z

@usha1830 Out of curiosity, why "table_size *4 + 1 for learner tables" ? Why not "table_size + 1" for learner tables?

usha1830 · 2022-10-28T09:55:58Z

@usha1830 Out of curiosity, why "table_size *4 + 1 for learner tables" ? Why not "table_size + 1" for learner tables?

This is based on internal implementation in dpdk target. There are 4 buckets for each index.
Reference : https://git.dpdk.org/dpdk/tree/lib/table/rte_swx_table_learner.c#n243

usha1830 · 2022-10-28T10:04:06Z

testdata/p4_16_samples_outputs/pna-add-on-miss.p4.spec

@@ -61,7 +61,7 @@ action next_hop2 args instanceof next_hop2_arg_t {

 action add_on_miss_action2 args none {
 	mov m.MainControlT_tmp 0x0
-	mov m.MainControlT_tmp_0 0x4d2
+	mov m.MainControlT_tmp_0 0x4D2


I don't know why this change is occurring.
After syncing with main, I regenerated the output file and see this. All hex constants are changed to uppercase.

mihaibudiu · 2022-10-28T18:14:21Z

backends/dpdk/dpdkArch.cpp

+                found = true;
+            }
+            if (oneInstance == "")
+            oneInstance = instanceName;


indentation

mihaibudiu · 2022-10-28T18:15:23Z

backends/dpdk/dpdkArch.cpp

+                        cstring method, cstring instancename) {
+    cstring oneInstance = "";
+    bool found = false;
+    for (auto stmt : a->body->components) {


you could do this by using a small visitor. it would be more robust and handle nested blocks, etc.

I didn't get this comment correctly.
A visitor for BlockStatement and then calling visit(a->body) here?
Visitors for MethodCallStatement and AssignmentStatement would also be needed I guess.

You should probably create a new visitor instead of a loop

mihaibudiu · 2022-10-28T18:16:07Z

backends/dpdk/dpdkArch.cpp

+}
+
+void CollectDirectCounterMeter::postorder(const IR::P4Action* a) {
+    ifMethodFound(a, "count");


you ignore the result, that's strange

ifMethodFound function is called from 2 places.
One for making sure that the default action of owner table of the Direct Meter/Counter extern instance has a execute/count method. In this case, the return value is checked to issue error if specified method call doesn't exist in default action.
Second, for making sure that an action only contains count/execute method calls for only one Direct counter/meter instance. In this case, the error is emitted in the ifMethodFound function itself and hence we are not using the return value here.

This would be a good comment

mihaibudiu · 2022-10-28T18:19:52Z

backends/dpdk/dpdkArch.cpp

+        return;
+    }
+    if (table_type == InternalTableType::LEARNER)
+            table_size *= 4;


inconsistent indentation

mihaibudiu · 2022-10-28T18:22:07Z

backends/dpdk/dpdkArch.cpp

+    if ((a->originalExternType->getName().name == "DirectCounter" &&
+        a->method->getName().name == "count") ||
+        (a->originalExternType->getName().name == "DirectMeter" &&
+        a->method->getName().name == "execute")) {


what if the method does not have this name?

I suspect the answer is "this is implementing the PSA/PNA architecture definitions of DirectCounter and DirectMeter, and the method names are defined in the spec". So the methods won't have any other names.

that's why an error should be given

Sorry, I misunderstood the reason for your question. Carry on :-)

Yes. I will add error here

mihaibudiu · 2022-10-28T18:22:31Z

backends/dpdk/dpdkArch.cpp

+            for (auto action : ownerTable->getActionList()->actionList) {
+                auto action_decl = refMap->getDeclaration(action->getPath())->to<IR::P4Action>();
+                    if (act->name.name == action_decl->name.name) {
+                    invokedFromOwnerTable = true;


indentation

mihaibudiu · 2022-10-28T18:25:14Z

backends/dpdk/dpdkHelpers.cpp

+
+                    // DPDK target needs packet length as mandatory parameters
+                    if (argSize < 1) {
+                        ::error(ErrorType::ERR_UNEXPECTED, "Expected atleast 1 argument "


missing space

mihaibudiu · 2022-10-28T18:30:38Z

backends/dpdk/dpdkHelpers.cpp

+            auto declArgs = di->arguments;
+            unsigned value = 0;
+            auto counter_type = declArgs->at(0)->expression;
+            if (counter_type->is<IR::Constant>())


usually you write if (auto c = counter_type->toIR::Constant()) value = c->asUnsigned().

mihaibudiu · 2022-10-28T18:30:52Z

backends/dpdk/dpdkHelpers.cpp

+                value = counter_type->to<IR::Constant>()->asUnsigned();
+            if (a->method->getName().name == "count") {
+                auto args = a->expr->arguments;
+                if (args->size() > 1){


we usually use spaces before {

mihaibudiu · 2022-10-28T18:32:34Z

backends/dpdk/spec.cpp

+        } else {
+            IR::Expression *n_meters =nullptr;
+            if (directMeterCounterSizeMap.count(this->Name())) {
+                n_meters = new IR::Constant(directMeterCounterSizeMap.at(this->Name()));


we usually omit this-> if it is not ambiguous

mihaibudiu · 2022-11-03T21:01:34Z

You should rebase this

…w tests to cover these error checks

…ter extern

usha1830 · 2022-11-04T05:01:29Z

@mbudiu-vmw Addressed all review comments. Please check.
Last commit ea75b39 is a minor change on top the existing PR. It adds table id for the table with which the direct meter or counter is associated. Please review.

usha1830 commented Oct 28, 2022

View reviewed changes

mihaibudiu reviewed Oct 28, 2022

View reviewed changes

backends/dpdk/dpdkArch.cpp Outdated

found = true;

}

if (oneInstance == "")

oneInstance = instanceName;

Copy link

Contributor

mihaibudiu Oct 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indentation

mihaibudiu approved these changes Oct 28, 2022

View reviewed changes

usha1830 added 7 commits November 4, 2022 04:09

Support for direct counter in .spec and context.json

077ddc6

Add unit tests and update reference output

d5ebe8c

Code cleanup

4b1caaf

Add support for direct meter

90fb01e

Add more constraints on direct meter and direct counter usage, add ne…

8c151c4

…w tests to cover these error checks

update reference outputs

634c2ca

Rebase and address review comments

37eabd3

usha1830 force-pushed the DirectCounter branch from 33fc0b2 to ea75b39 Compare November 4, 2022 04:56

Add table_id in context json for owner table of direct meter and coun…

ea75b39

…ter extern

mihaibudiu approved these changes Nov 4, 2022

View reviewed changes

mihaibudiu merged commit 1bf14ca into p4lang:main Nov 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DPDK Backend: Direct counter and direct meter support #3631

DPDK Backend: Direct counter and direct meter support #3631

usha1830 commented Oct 28, 2022

jfingerh commented Oct 28, 2022

usha1830 commented Oct 28, 2022

usha1830 Oct 28, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

usha1830 Oct 29, 2022

mihaibudiu Oct 29, 2022

mihaibudiu Oct 28, 2022

usha1830 Oct 29, 2022

mihaibudiu Oct 29, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

jafingerhut Oct 28, 2022

mihaibudiu Oct 28, 2022

jafingerhut Oct 28, 2022

usha1830 Oct 29, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

mihaibudiu Oct 28, 2022

mihaibudiu commented Nov 3, 2022

usha1830 commented Nov 4, 2022

DPDK Backend: Direct counter and direct meter support #3631

DPDK Backend: Direct counter and direct meter support #3631

Conversation

usha1830 commented Oct 28, 2022

jfingerh commented Oct 28, 2022

usha1830 commented Oct 28, 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

Choose a reason for hiding this comment

mihaibudiu commented Nov 3, 2022

usha1830 commented Nov 4, 2022