Implement the eBPF extension for P4Tools' P4Testgen #3510
Conversation
|
@fruffy If I have new source code under bmv2 from which a library is built, how can I use your tool to test the library's code? I do have several P4 programs to test the library and I use Cisco trex for traffic testing. I guess I first create stf files to test the P4 code? |
|
If you have a test script that can execute STF tests on your target than you can just use the tests generated by P4Testgen to test your target. For example, the p4c repository has run-bmv2-test.py and run-ebpf-test.py. However, P4Testgen only support |
|
Will try to address the comments in the other PR. There is actually several follow-up PRs coming after to this one (which will be reviewed by others). Breaking up the current tool would have made the development of those PRs quite difficult. We would have needed to segment and stack several PRs. Each of these PRs would then continuously need to be synced with the current base PR under review. Under these circumstances it made more sense to release the core executor plus the BMv2 back end all at once and have the PRs develop against that monolith. Otherwise things would have gotten very messy. |
| /// standard: standard implementation - use normal control plane entries. | ||
| /// constant: The table is constant - no control entries are possible. | ||
| /// skip: Skip the implementation and just use the default entry (no entry at all). | ||
| enum class TableImplementation { standard, selector, profile, constant, skip }; |
There was a problem hiding this comment.
I don't think ebpf supports action selectors or action profiles. In fact, they are not even listed in the comment.
| namespace EBPF { | ||
|
|
||
| /// Encapsulates the STF commands and assembles the arguments to be printed to the STF File. | ||
| class STFTest { |
There was a problem hiding this comment.
What in this file is specific to EBPF?
| @@ -0,0 +1,207 @@ | |||
| #ifndef TESTGEN_TARGETS_BMV2_BACKEND_STF_STF_H_ | |||
There was a problem hiding this comment.
how is this file different from the one in the ebpf backend?
what in stf is specific to a target?
|
I am not reviewing two PRs of 50K lines each. I am reviewing just this one. |
|
Once the other PR is merged, I will rebase it. There should be considerably less lines of code then. This PR is still a draft, it was not meant to be reviewed yet. Unfortunately, I can not change the base to fruffy-bfn/p4c. |
backends/p4tools/testgen/core/exploration_strategy/exploration_strategy.h
Outdated
Show resolved
Hide resolved
backends/p4tools/testgen/core/exploration_strategy/exploration_strategy.h
Outdated
Show resolved
Hide resolved
| const ExecutionState& prevState, | ||
| gsl::not_null<ExecutionState*> nextState) | ||
| : constraint(IRUtils::getBoolLiteral(true)), nextState(nextState) { | ||
| if (c) { |
There was a problem hiding this comment.
in general it is uncommon to do deep processing in constructors, since if that fails the object is left in an indeterminate state.
backends/p4tools/testgen/core/exploration_strategy/incremental_stack.h
Outdated
Show resolved
Hide resolved
backends/p4tools/testgen/core/exploration_strategy/incremental_stack.h
Outdated
Show resolved
Hide resolved
fruffy
force-pushed
the
ebpf_tools
branch
2 times, most recently
from
d5f10ab
to
23a8f75
Compare
| template <typename... T> | ||
| explicit TestgenUnimplemented(const char* format, T... args) | ||
| : P4CExceptionBase(format, args...) { | ||
| // Check if output is redirected and if so, then don't color text so that |
There was a problem hiding this comment.
This is actually from the original /lib/exceptions.h.
backends/p4tools/testgen/core/exploration_strategy/incremental_max_coverage_stack.h
Outdated
Show resolved
Hide resolved
| /// Typically, this is equivalent to the size of the input packet. However, there may be | ||
| /// functions such as lookahead that peek into the program packet without actually advancing the | ||
| /// cursor. The cursor ensures that parsing remains consistent in these cases. | ||
| int inputPacketCursor = 0; |
There was a problem hiding this comment.
Because this variable is also incremented by width_bits(), which has the return type int. It might make sense to return size_t for width_bits in general. This is a very common type mismatch in the compiler.
fruffy
force-pushed
the
ebpf_tools
branch
4 times, most recently
from
cf38a46
to
d098d84
Compare
|
I left unresolved comments open. Do any of them still need to be addressed? I will work on the eBPF comments after #3495 is merged. |
With so many commits it is impossible for me to keep track of which comments have been addressed. |
fruffy
force-pushed
the
ebpf_tools
branch
3 times, most recently
from
f7d27cf
to
c774e9d
Compare
I left all the comments that I have not addressed unresolved. It should be only about a dozen. Any other things that still need to be done? |
|
|
We can share a draft paper with you privately (would love your feedback!) We are working on some public documentation -- more focused on the UI than the internals, but can include a bit of that too. |
|
Papers are great, but a short README is still necessary, especially if the paper does not talk about the code structure. |
|
Agree! |
| const ExecutionState& state, SmallStepEvaluator::Result& result) { | ||
| uint64_t recirculateCount = 0; | ||
| // Grab the recirculate count. Stop after more than 1 circulation loop to avoid | ||
| // infinite recirculation loops. |
There was a problem hiding this comment.
Is this part of the bmv2 spec?
There was a problem hiding this comment.
No, in fact BMv2 seems to circulate more rounds. I do not have the exact count. Will add this as a TODO.
There was a problem hiding this comment.
I am pretty sure that bmv2 does not enforce any limit on the number of recirculations or clones of a packet.
| auto recirculateIndex = args->at(2)->expression->checkedTo<IR::Constant>()->asUint64(); | ||
| boost::optional<const Constraint*> cond = boost::none; | ||
|
|
||
| if (cloneType == BMv2Constants::CLONE_TYPE_I2E) { |
There was a problem hiding this comment.
does this make sure that the effects of the clone happen at the end of the pipeline?
There was a problem hiding this comment.
Practically speaking clone can only be called in ingress or egress. This is enforced by the compiler. So we have some leeway here since the egress block will immediately follow in the I2E case.
|
|
||
| if (cloneType == BMv2Constants::CLONE_TYPE_I2E) { | ||
| // Pick a clone port var. For now, pick a random value from 0-511. | ||
| const auto* rndConst = |
There was a problem hiding this comment.
In some sense this is an executable specification of BMv2.
I wonder whether one could take advantage of that in some way besides test generation. For example, why do you need bmv2 at all, can you replace it with this implementation?
There was a problem hiding this comment.
Well, we can only give you the expected output for a given input (packet and control plane config) but BMv2 also models all the other parts of the stack such as the control plane API etc. So are a little bit more limited here.
fruffy
force-pushed
the
ebpf_tools
branch
9 times, most recently
from
1464c3f
to
0430b5c
Compare
Fix up some eBPF programs. Typo. Implement parser errors. Smaller fixes. Smaller fixes. Implementing the first version of a custom checksum extern for eBPF. eBPF can not write to a packet. Fix. More cleanup. Fix a couple more issues. more work on the checksum. Minor. Revert. Implement correct checksum function for eBPF. Implement simple version of conntrack extern and fix up header indices. Remove file.
asdasda More edits.
There was a problem hiding this comment.
@mbudiu-vmw I cleaned and fixed up the eBPF back end for P4Testgen. Maybe if you have time could you give this a second check? Otherwise I will merge it. It seems to expose some problems with the test framework. I am thinking of fixing these in a separate PR.
| @@ -189,7 +189,11 @@ find_python_module (re REQUIRED) | |||
| # other packages | |||
| find_package (Doxygen) | |||
| find_package (BMV2) | |||
|
|
|||
| # If we have found simple switch or psa switch, we also need scapy and ipaddr. | |||
There was a problem hiding this comment.
There were spurious warnings emitted by CMake on newer versions. I fixed this in this PR but can move this into a separate PR, if requested.
| @@ -51,7 +51,10 @@ | |||
| "default is test") | |||
| PARSER.add_argument("-e", "--extern-file", dest="extern", default="", | |||
| help="Specify path additional file with C extern function definition") | |||
|
|
|||
| PARSER.add_argument("-tf", "--testfile", dest="testfile", | |||
There was a problem hiding this comment.
The eBPF test script had no means to specify an input test file. I added this.
| auto const& fieldMatch = match.second; | ||
|
|
||
| // Replace header stack indices hdr[<index>] with hdr$<index>. | ||
| // TODO: This is a limitation of the stf parser. We should fix this. |
There was a problem hiding this comment.
The STF parser can not handle header stacks as key names. We have to rewrite them. This is a strange limitation we should fix at some point.
| p4tools_add_xfail_reason( | ||
| "testgen-p4c-ebpf" | ||
| "Error 2: Failed to build the filter:" | ||
| lpm_ebpf.p4 |
There was a problem hiding this comment.
These are compilation failures triggered by P4Testgen. It seems like there are issues with the synthesized control plane interface.
|
How about removing the "Draft" label? |
| @@ -106,8 +108,9 @@ void CmdStepper::initializeBlockParams(const IR::Type_Declaration* typeDecl, | |||
| if (const auto* ts = paramType->to<IR::Type_StructLike>()) { | |||
| declareStructLike(nextState, paramPath, ts); | |||
| } else if (paramType->is<IR::Type_Base>()) { | |||
| auto paramRef = nextState->convertPathExpr(paramPath); | |||
| nextState->set(paramRef, programInfo.createTargetUninitialized(paramRef->type, false)); | |||
| // If the type is a flat Type_BAse, convert it to a member with a "*" prefix. | |||
| @@ -347,6 +347,11 @@ void ExprStepper::evalInternalExternMethodCall(const IR::MethodCallExpression* c | |||
| generateCopyIn(nextState, fieldargRef, fieldGlobalRef, dir, forceTaint->value); | |||
| } | |||
| } else if (assignType->is<IR::Type_Base>()) { | |||
| // If the type is a flat Type_Base, convert it to a member with a "*" prefix. | |||
There was a problem hiding this comment.
this looks very much like a utility function.
| p4tools_add_xfail_reason( | ||
| "testgen-p4c-ebpf" | ||
| "Error 2: Failed to build the filter:" | ||
| lpm_ebpf.p4 |
|
|
||
| class EBPFTableStepper : public TableStepper { | ||
| private: | ||
| /// Specifies the type of the table implementation: |
There was a problem hiding this comment.
I don't understand what is the relationship between these enums and ebpf.
Ebpf has only 2 implementation properties, array or hash.
There was a problem hiding this comment.
Yes, eBPF does not have a lot of additional table properties or implementations. We could add hash and array to this enum, but they are not changing the semantics of entry generation for tests if I recall correctly.
| namespace EBPF { | ||
|
|
||
| /// Extracts information from the @testSpec to emit a STF test case. | ||
| class STF : public TF { |
There was a problem hiding this comment.
I repeat the question about what in this class is specific to EBPF. It looks to me like STF should be the same for all backends.
There was a problem hiding this comment.
There are subtle difference between how the STF tests are processed for each back end. For example, BMv2 does not need this and does support $ as a postfix to indicate exact packet length matching. Furthermore, BMv2 supports various externs such as clone, action profiles, registers, which are handled in this stf class.
fruffy
changed the title
fruffy
changed the title
This is a proof of concept implementation for P4Testgen to demonstrate its extensibility for other targets. The implementation is a little janky but works.
The eBPF test failures actually do expose flaws in the test back end.
This PR will be cleaned up and rebased once the P4Tools PR is merged in.