P4Tools: Implement default action override for BMv2 STF. #3685
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fruffy
force-pushed
the
fruffy/default_action
branch
2 times, most recently
from
856c781 to
18993ee
Compare
fruffy
requested review from
pkotikal,
vhavel,
jnfoster and
mihaibudiu
as code owners
fruffy
commented
Nov 11, 2022
| @@ -452,7 +452,7 @@ def parse_table_set_default(self, cmd): | |||
| actionArgs.set(k, v) | |||
| command = "table_set_default " + tableName + " " + actionName | |||
| if actionArgs.size(): | |||
| command += " => " + str(actionArgs) | |||
| command += " " + str(actionArgs) | |||
There was a problem hiding this comment.
There was a bug in the STF test framework. Default action overrides were not using the correct syntax.
fruffy
force-pushed
the
fruffy/default_action
branch
3 times, most recently
from
fc27d6f to
8740303
Compare
pkotikal
reviewed
Nov 22, 2022
| properties.tableIsImmutable = isConstant || table->getAnnotation("hidden") != nullptr; | ||
| const auto* defaultAction = table->properties->getProperty("default_action"); | ||
| CHECK_NULL(defaultAction); | ||
| properties.defaultIsImmutable = defaultAction->isConstant; |
pkotikal
reviewed
Nov 22, 2022
| @@ -14,7 +16,7 @@ namespace P4Tools { | |||
| namespace P4Testgen { | |||
|
|
|||
| /// THe default base class for the various test frameworks (TF). Every test framework has a test | |||
| /// name and a seed associated with it. | |||
| /// name and a seed associated with it. Also contains a variety of common utility functions. | |||
There was a problem hiding this comment.
Do all test frameworks support ActionProfiles and ActionSelectors - do we want to move it to common tf?
There was a problem hiding this comment.
I think it is shared between enough back ends that we can add it to common.
pkotikal
reviewed
Nov 22, 2022
| @@ -302,9 +245,14 @@ static std::string getTestCase() { | |||
| ## if control_plane | |||
| ## for table in control_plane.tables | |||
| # Table {{table.table_name}} | |||
| ## if existsIn(table, "default_override") | |||
| setdefault {{table.table_name}} {{table.default_override.action_name}}({% for a in table.default_override.act_args %}{{a.param}}:{{a.value}}{% if not loop.is_last %},{% endif %}{% endfor %}) | |||
There was a problem hiding this comment.
Very nice, now it's very convenient, thank you!
pkotikal
approved these changes
Nov 22, 2022
fruffy
force-pushed
the
fruffy/default_action
branch
from
8740303 to
342412e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements default action overrides for P4Testgen's BMv2 extension. Only for the STF back end so far.
If a table does not have a key, it can not match and pick an action. However, it might still be possible to override the default action and choose a different path. This PR implements the logic to override a table's default action.
Also fixes #3557.