Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 34 commits into from
Jun 29, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
a805208
Fix dpdk regression failure: Duplicates declaration
usha1830 Apr 21, 2021
611261b
Fix cpplint errors
usha1830 Apr 22, 2021
9c8e6d2
Added checks to ensure parameters exist before accessing them
usha1830 Apr 22, 2021
b184e0b
Merge branch 'master' into ushag/fix_duplicates_declaration
usha1830 Apr 22, 2021
c03e07d
Merge remote-tracking branch 'upstream/master'
usha1830 Apr 23, 2021
c99d261
Merging with p4lang/p4c repo main branch
usha1830 May 6, 2021
c6a5c12
Merge branch 'p4lang:main' into main
usha1830 May 7, 2021
4817ea2
Fix PSA extern Register's implementation
usha1830 May 11, 2021
bfb6f02
Updated spec.cpp as per review comments
usha1830 May 12, 2021
f5cfb35
Emit a warning if Register instantiation is found outside control block
usha1830 May 12, 2021
5ca47b1
Fixed a typo
usha1830 May 12, 2021
f0c7f9c
Remove restriction for instantiation and invocation of register exter…
usha1830 May 13, 2021
f3ab6ad
Merge branch 'p4lang:main' into ushag_conditional_operator
usha1830 May 28, 2021
778cd74
Conditional operator support for P4C-DPDK.
usha1830 May 28, 2021
2b992a4
Merge branch 'p4lang:main' into ushag_conditional_operator
usha1830 May 31, 2021
61567fd
Changing ternary match to exact in test program, updating the referen…
usha1830 May 31, 2021
009fd24
Merge branch 'ushag_conditional_operator' of https://github.com/usha1…
usha1830 May 31, 2021
afa8764
Merge branch 'p4lang:main' into ushag_conditional_operator
usha1830 Jun 4, 2021
05a68ba
Added support for range and mask operations in transition select stat…
usha1830 Jun 4, 2021
d43963c
Merge branch 'p4lang:main' into origin/ushag/mask_range
usha1830 Jun 9, 2021
8b9927f
Support mask operations in dpdk backend (#1)
usha1830 Jun 9, 2021
3bde5de
Remove pass for inserting temporary mask variables
usha1830 Jun 10, 2021
f7cb04d
Merge branch 'p4lang:main' into origin/ushag/mask_range
usha1830 Jun 10, 2021
a3e6ef6
update reference output
usha1830 Jun 10, 2021
6b963ce
Merge branch 'p4lang:main' into origin/ushag/mask_range
usha1830 Jun 16, 2021
8cf5882
Address review comments
usha1830 Jun 16, 2021
00ce76f
Merge branch 'p4lang:main' into origin/ushag/mask_range
usha1830 Jun 20, 2021
32d2abe
Merge branch 'p4lang:main' into origin/ushag/mask_range
usha1830 Jun 22, 2021
7986213
DPDK backend: Add support for meter and counter PSA externs
usha1830 Jun 22, 2021
9648cce
Address review comments for Meter and Counter extern support
usha1830 Jun 24, 2021
d663dbb
Merge branch 'p4lang:main' into ushag/Meter_counter
usha1830 Jun 25, 2021
76a0557
Cleanup dpdk regression test script to avoid saving error and runtime…
usha1830 Jun 25, 2021
402ae06
Updated reference output
usha1830 Jun 26, 2021
0013778
Simpilfied the implementation to use vector instead of map
usha1830 Jun 26, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
DPDK backend: Add support for meter and counter PSA externs
  • Loading branch information
usha1830 committed Jun 22, 2021
commit 798621375b73993445eaad422f5d0da6afc91263
5 changes: 5 additions & 0 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,11 @@ add_custom_target(update_includes ALL
${CMAKE_COMMAND} -E copy_if_different ${CMAKE_CURRENT_SOURCE_DIR}/\$$h ${P4C_BINARY_DIR}/p4include \;
done
)
if (ENABLE_DPDK)
add_custom_target(dpdk_includes ALL
COMMAND ${CMAKE_COMMAND} -E make_directory ${P4C_BINARY_DIR}/p4include/dpdk
COMMAND ${CMAKE_COMMAND} -E copy_if_different ${P4C_SOURCE_DIR}/p4include/dpdk/psa.p4 ${P4C_BINARY_DIR}/p4include/dpdk)
endif ()

# Installation
# Targets install themselves. Here we install the core headers
Expand Down
9 changes: 7 additions & 2 deletions backends/dpdk/DpdkXfail.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,15 @@ p4c_add_xfail_reason("dpdk"
)

p4c_add_xfail_reason("dpdk"
"Meter Not implemented"
"LHS of meter execute"
testdata/p4_16_samples/psa-meter1.p4
)

p4c_add_xfail_reason("dpdk"
"Expected atleast 2 arguments"
testdata/p4_16_samples/psa-meter3.p4
testdata/p4_16_samples/psa-meter7-bmv2.p4
)
)

# symbolic evaluator does not support verify() statement
p4c_add_xfail_reason("dpdk"
Expand Down
12 changes: 11 additions & 1 deletion backends/dpdk/dpdk.def
Original file line number Diff line number Diff line change
Expand Up @@ -433,17 +433,27 @@ class DpdkVerifyStatement: DpdkAsmStatement, IDPDKNode {
#nodbprint
}

class DpdkMeterDeclStatement: DpdkAsmStatement {
cstring meter;
Expression size;
std::ostream& toSpec(std::ostream& out) const override;
#nodbprint
}

class DpdkMeterExecuteStatement: DpdkAsmStatement, IDPDKNode {
cstring meter;
Expression index;
Expression color;
NullOK Expression length;
Expression color_in;
Expression color_out;
std::ostream& toSpec(std::ostream& out) const override;
#nodbprint
}

class DpdkCounterCountStatement: DpdkAsmStatement, IDPDKNode {
cstring counter;
Expression index;
NullOK optional Expression incr;
std::ostream& toSpec(std::ostream& out) const override;
#nodbprint
}
Expand Down
91 changes: 79 additions & 12 deletions backends/dpdk/dpdkArch.h
Original file line number Diff line number Diff line change
Expand Up @@ -446,32 +446,95 @@ class ConvertInternetChecksum : public PassManager {
* for emitting to the .spec file later */
class CollectRegisterDeclaration : public Inspector {
std::map<const IR::Declaration_Instance *, cstring> *reg_map;

P4::TypeMap *typeMap;

public:
CollectRegisterDeclaration(
std::map<const IR::Declaration_Instance *, cstring> *reg_map, P4::TypeMap *)
: reg_map(reg_map) {}
std::map<const IR::Declaration_Instance *, cstring> *reg_map, P4::TypeMap *typeMap)
: reg_map(reg_map), typeMap(typeMap) {}

bool preorder(const IR::Declaration_Instance *d) override {
if (d->type->is<IR::Type_Specialized>()) {
auto type = d->type->to<IR::Type_Specialized>();
auto externTypeName = type->baseType->path->name.name;
if (externTypeName == "Register"){
if (d->arguments->size() != 1 and d->arguments->size() != 2 ) {
::error("%1%: expected size and optionally init_val as arguments", d);
}
reg_map->emplace(d, d->name);
if (d->arguments->size() != 1 and d->arguments->size() != 2 ) {
::error("%1%: expected size and optionally init_val as arguments", d);
}
reg_map->emplace(d, d->name);
}
}
return false;
}
};

/* This pass collects PSA meter declaration instances and push them to a map
* for emitting to the .spec file later */
class CollectMeterDeclaration : public Inspector {
std::map<const IR::Declaration_Instance *, cstring> *met_map;
P4::TypeMap *typeMap;

public:
CollectMeterDeclaration(
std::map<const IR::Declaration_Instance *, cstring> *met_map, P4::TypeMap *typeMap)
: met_map(met_map) , typeMap(typeMap) {}

bool preorder(const IR::Declaration_Instance *d) override {
if (d->type->is<IR::Type_Specialized>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

these can be combined into a single line

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

Choose a reason for hiding this comment

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

no space after (

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you want to add a test for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@mihaibudiu mihaibudiu Jun 23, 2021

Choose a reason for hiding this comment

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

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.

::warning(ErrorType::WARN_UNSUPPORTED,
"%1%: Packet metering is not supported." \
" Falling back to byte metering.", d);
met_map->emplace(d, d->name);
}
}
}
return false;
}
};

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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

std::map<const IR::Declaration_Instance *, cstring> *cnt_map;
P4::TypeMap *typeMap;

public:
CollectCounterDeclaration(
std::map<const IR::Declaration_Instance *, cstring> *cnt_map, P4::TypeMap *typeMap)
: cnt_map(cnt_map) , typeMap(typeMap) {}

bool preorder(const IR::Declaration_Instance *d) override {
if (d->type->is<IR::Type_Specialized>()) {
auto type = d->type->to<IR::Type_Specialized>();
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

this error message does not seem very clear to me.

}
cnt_map->emplace(d, d->name);
}
}
return false;
}
};

/* This passmanager is for collecting the extern declarations for register meter and counter */
class AddExternDeclaration : public PassManager {
public:
std::map<const IR::Declaration_Instance *, cstring> reg_map;
AddRegisterDeclaration(P4::TypeMap *typeMap) {
std::map<const IR::Declaration_Instance *, cstring> cnt_map;
std::map<const IR::Declaration_Instance *, cstring> met_map;
AddExternDeclaration(P4::TypeMap *typeMap) {
passes.push_back(new CollectRegisterDeclaration(&reg_map, typeMap));
passes.push_back(new CollectCounterDeclaration(&cnt_map, typeMap));
passes.push_back(new CollectMeterDeclaration(&met_map, typeMap));
}
};

Expand Down Expand Up @@ -584,6 +647,8 @@ class RewriteToDpdkArch : public PassManager {
*args_struct_map;
std::map<const IR::Declaration_Instance *, cstring> *csum_map;
std::map<const IR::Declaration_Instance *, cstring> *reg_map;
std::map<const IR::Declaration_Instance *, cstring> *cnt_map;
std::map<const IR::Declaration_Instance *, cstring> *met_map;
RewriteToDpdkArch(P4::ReferenceMap *refMap, P4::TypeMap *typeMap,
DpdkVariableCollector *collector) {
setName("RewriteToDpdkArch");
Expand Down Expand Up @@ -631,9 +696,11 @@ class RewriteToDpdkArch : public PassManager {
args_struct_map = &p->args_struct_map;
passes.push_back(p);
passes.push_back(new ConvertLogicalExpression);
auto insertRegDeclaration = new AddRegisterDeclaration(typeMap);
passes.push_back(insertRegDeclaration);
reg_map = &insertRegDeclaration->reg_map;
auto insertExternDeclaration = new AddExternDeclaration(typeMap);
passes.push_back(insertExternDeclaration);
reg_map = &insertExternDeclaration->reg_map;
cnt_map = &insertExternDeclaration->cnt_map;
met_map = &insertExternDeclaration->met_map;
}
};

Expand Down
64 changes: 55 additions & 9 deletions backends/dpdk/dpdkHelpers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,27 @@ bool ConvertStatementToDpdk::preorder(const IR::AssignmentStatement *a) {
i = new IR::DpdkGetChecksumStatement(
left, e->object->getName(), intermediate);
}
} else if (e->originalExternType->getName().name == "Meter") {
if (e->method->getName().name == "execute") {
auto argSize = e->expr->arguments->size();

// DPDK target needs index and packet length as mandatory parameters
if (argSize < 2)
::error(ErrorType::ERR_UNEXPECTED, "Expected atleast 2 arguments for %1%",
e->object->getName());
const IR::Expression *color_in = nullptr;
const IR::Expression *length = nullptr;
auto index = (*e->expr->arguments)[0]->expression;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

if (argSize == 2) {
length = (*e->expr->arguments)[1]->expression;
color_in = new IR::Constant(1);
} else if (argSize == 3) {
length = (*e->expr->arguments)[2]->expression;
color_in = (*e->expr->arguments)[1]->expression;
}
i = new IR::DpdkMeterExecuteStatement(
e->object->getName(), index, length, color_in, left);
}
} else if (e->originalExternType->getName().name == "Register") {
if (e->method->getName().name == "read") {
auto index = (*e->expr->arguments)[0]->expression;
Expand Down Expand Up @@ -402,21 +423,46 @@ bool ConvertStatementToDpdk::preorder(const IR::MethodCallStatement *s) {
}
} else if (a->originalExternType->getName().name == "Meter") {
if (a->method->getName().name == "execute") {
auto args = a->expr->arguments;
auto index = args->at(0)->expression;
auto color = args->at(1)->expression;
auto meter = a->object->getName();
add_instr(
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 " \
Copy link
Contributor

Choose a reason for hiding this comment

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

no return here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

"Use this format instead : color_out = %1%.execute(index, color_in)",
a->object->getName());
} else {
BUG("Meter function not implemented.");
}
} else if (a->originalExternType->getName().name == "Counter") {
auto di = a->object->to<IR::Declaration_Instance>();
auto declArgs = di->arguments;
unsigned value = 0;
auto counter_type = declArgs->at(1)->expression;
if (counter_type->is<IR::Constant>())
value = counter_type->to<IR::Constant>()->asUnsigned();
if (a->method->getName().name == "count") {
auto args = a->expr->arguments;
auto index = args->at(0)->expression;
auto counter = a->object->getName();
add_instr(new IR::DpdkCounterCountStatement(counter, index));
if (args->size() < 1){
::error(ErrorType::ERR_UNEXPECTED, "Expected atleast 1 arguments for %1%",
a->object->getName());
} else {
const IR::Expression *incr = nullptr;
auto index = args->at(0)->expression;
auto counter = a->object->getName();
if (args->size() == 2)
incr = args->at(1)->expression;
if (value == 2) {
if (incr) {
add_instr(new IR::DpdkCounterCountStatement(counter+"_packets",
index, incr));
add_instr(new IR::DpdkCounterCountStatement(counter+"_bytes",
index));
} else {
::error(ErrorType::ERR_UNEXPECTED,
"Expected packet length argument for %1%", a->object->getName());
}
} else {
add_instr(new IR::DpdkCounterCountStatement(counter, index, incr));
}
}
} else {
BUG("Counter function not implemented");
}
Expand Down
14 changes: 14 additions & 0 deletions backends/dpdk/dpdkProgram.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,20 @@ const IR::DpdkAsmProgram *ConvertToDpdkProgram::create(IR::P4Program *prog) {
externDecls.push_back(st);
}

/* Pushing Counter declarations */
for (auto kv : *cnt_map) {
auto s = kv.first;
auto st = new IR::DpdkExternDeclaration(s->name, s->annotations, s->type, s->arguments);
externDecls.push_back(st);
}

/* Pushing Meter declarations */
for (auto kv : *met_map) {
auto s = kv.first;
auto st = new IR::DpdkExternDeclaration(s->name, s->annotations, s->type, s->arguments);
externDecls.push_back(st);
}

return new IR::DpdkAsmProgram(
headerType, structType, externDecls, ingress_converter->getActions(),
ingress_converter->getTables(), statements, collector->get_globals());
Expand Down
4 changes: 4 additions & 0 deletions backends/dpdk/dpdkProgram.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ class ConvertToDpdkProgram : public Transform {
*args_struct_map;
std::map<const IR::Declaration_Instance *, cstring> *csum_map;
std::map<const IR::Declaration_Instance *, cstring> *reg_map;
std::map<const IR::Declaration_Instance *, cstring> *cnt_map;
std::map<const IR::Declaration_Instance *, cstring> *met_map;

public:
ConvertToDpdkProgram(BMV2::PsaProgramStructure &structure,
Expand All @@ -70,6 +72,8 @@ class ConvertToDpdkProgram : public Transform {
args_struct_map = dpdkarch->args_struct_map;
csum_map = dpdkarch->csum_map;
reg_map = dpdkarch->reg_map;
cnt_map = dpdkarch->cnt_map;
met_map = dpdkarch->met_map;
}

const IR::DpdkAsmProgram *create(IR::P4Program *prog);
Expand Down
57 changes: 51 additions & 6 deletions backends/dpdk/spec.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,10 +163,41 @@ std::ostream &IR::DpdkExternDeclaration::toSpec(std::ostream &out) const {
}
}
else if ( DPDK::toStr(this->getType()) == "Counter") {
//TODO yet to be implemented
auto args = this->arguments;
unsigned value = 0;
if (args->size() < 2) {
::error ("Counter extern declaration %1% must contain 2 parameters\n", this->Name());
Copy link
Contributor

Choose a reason for hiding this comment

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

your indentation is not consistent with our standard

} else {
auto n_counters = args->at(0)->expression;
auto counter_type = args->at(1)->expression;
if (counter_type->is<IR::Constant>())
value = counter_type->to<IR::Constant>()->asUnsigned();
if (value == 2) {
/* For PACKETS_AND_BYTES counter type, two regarray declarations are emitted and
the counter name is suffixed with _packets and _bytes */
auto regDecl = new IR::DpdkRegisterDeclStatement(this->Name()+"_packets", n_counters,
new IR::Constant(0));
regDecl->toSpec(out) << std::endl << std::endl;
regDecl = new IR::DpdkRegisterDeclStatement(this->Name()+"_bytes", n_counters,
new IR::Constant(0));
regDecl->toSpec(out) << std::endl;
} else {
auto regDecl = new IR::DpdkRegisterDeclStatement(this->Name(), n_counters,
new IR::Constant(0));
regDecl->toSpec(out) << std::endl;
}
}
}
else if ( DPDK::toStr(this->getType()) == "Meter") {
//TODO yet to be implemented
auto args = this->arguments;
if (args->size() < 2) {
::error ("Meter extern declaration %1% must contain a size parameter \
and meter type parameter", this->Name());
} else {
auto n_meters = args->at(0)->expression;
auto metDecl = new IR::DpdkMeterDeclStatement(this->Name(), n_meters);
metDecl->toSpec(out) << std::endl;
}
}
return out;
}
Expand Down Expand Up @@ -392,7 +423,9 @@ std::ostream &IR::DpdkAction::toSpec(std::ostream &out) const {
out << "{" << std::endl;
for (auto i : statements) {
out << "\t";
i->toSpec(out) << std::endl;
i->toSpec(out);
if (!i->to<IR::DpdkLabelStatement>())
out << std::endl;
}
out << "\treturn" << std::endl;
out << "}";
Expand Down Expand Up @@ -449,14 +482,26 @@ std::ostream &IR::DpdkVerifyStatement::toSpec(std::ostream &out) const {
return out;
}

std::ostream &IR::DpdkMeterDeclStatement::toSpec(std::ostream &out) const {
out << "metarray " << meter << " size " << DPDK::toStr(size);
return out;
}

std::ostream &IR::DpdkMeterExecuteStatement::toSpec(std::ostream &out) const {
out << "meter_execute " << meter << " " << DPDK::toStr(index) << " "
<< DPDK::toStr(color);
out << "meter " << meter << " " << DPDK::toStr(index) << " " << DPDK::toStr(length);
out << " " << DPDK::toStr(color_in) << " " << DPDK::toStr(color_out);
return out;
}

/* DPDK target uses Registers for implementing using Counters, atomic register add instruction
is used for incrementing the counter. Packet counters are incremented by packet length
specified as parameter and byte counters are incremente by 1 */
std::ostream &IR::DpdkCounterCountStatement::toSpec(std::ostream &out) const {
out << "counter_count " << counter << " " << DPDK::toStr(index);
out << "regadd " << counter << " " << DPDK::toStr(index) << " ";
if (incr)
out << DPDK::toStr(incr);
else
out << "1";
return out;
}

Expand Down
Loading