-
Notifications
You must be signed in to change notification settings - Fork 333
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
Add extra checks on next tables #1287
base: main
Are you sure you want to change the base?
Add extra checks on next tables #1287
Conversation
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
Add a bit more error checking on the contents of BMv2 JSON file contents. I have successfully run all p4c tests with these changes to bmv2, so I know that none of those hundreds of test programs violate these additional checks that cause exceptions to be thrown. |
Signed-off-by: Andy Fingerhut <[email protected]>
if (next_is_hit_miss) { | ||
table->set_next_node_hit(get_next_node(cfg_next_nodes["__HIT__"])); | ||
table->set_next_node_miss(get_next_node(cfg_next_nodes["__MISS__"])); | ||
} | ||
|
||
if (cfg_table.isMember("base_default_next")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jafingerhut do you think this is a good opportunity to mention this attribute (base_default_next
) in the bmv2 JSON format document? Not sure if p4c uses it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be happy to document it, if I could tell what it was for :-)
I see it in the JSON files generated by p4c in table definitions, pretty much most of the tables, but I also see in most tables [1] that the next_tables
key defines a next node for every action name of the table. It isn't clear to me when the value of base_default_next
is ever used by BMv2 code. Even if it is sometimes used, it seems to me that it might be for a feature that P4 the language does not need.
[1] the ones that don't use table.apply().hit
or table.apply().miss
features, which are most tables in the test suite (and typical P4 programs in general only use that feature occasionally).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the original PR: #78
I think this attribute was necessary at the time to implement the reset_default_entry
API.
Because in P4_16, there is always a default action with arguments (as the compiler will use NoAction
if the programmer didn't provide one), I do think that this attribute is actually never used [1], unless I am missing something.
Maybe we could try to stop generating it in the p4c bmv2 backend, assuming nothing breaks (if tests break as a result, we need to come back and check what it's actually used for...). It's always good to simplify the emitted JSON. If we stop using it in p4c, we should not include it in the documentation here (discarding my initial comment), but we should keep it in the code for backwards compatibility. Maybe with a comment in P4Objects.cpp that it is no longer used by p4c.
[1] This is based on the assumption that this code is always executed for all tables when the JSON is generated by p4c:
behavioral-model/src/bm_sim/P4Objects.cpp
Line 1889 in 2f82eaf
table->set_default_default_entry(action, std::move(adata), |
This assumption is only true if the
default_entry
attribute is set for all tables, and always includes the action_data
attribute (see my other comment). Could you confirm that it is indeed true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default_entry
is not present for tables with type indirect
(corresponding to implementation=action_profile) and indirect_ws
(corresponding to implementation=action_selector). default_entry
seems to always be present for tables with type simple
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I am not aware of any strong reason, other than similarity to Tofino perhaps, that we could not go ahead and implement full support for default_action in tables that have action profiles or action selectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that the code is actually able to handle "regular" default actions with action data, even for indirect tables. It is actually mentioned in the PR description here: #551. It is possible that p4c was never updated in response to that to generate the proper default_entry
in the JSON for such tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is the code in the p4c BMv2 back end that specifically checks for, and omits, any default_action definition in the P4_16 source code for tables with action profiles or action selectors: p4lang/p4c@96ec4d5#diff-e1b9c4c9f99e44821b652694b5a72f9a58784dd8e05572850c0a08b7f7c1c87fR1215-R1221
(search for the string "Target does not support default_action" in that commit's diff)
The commit comment by Mihai was "bmv2 does not support default_action for all tables", so apparently he believed that was true at that time. For all I know, perhaps it was true for bmv2 in May 2016.
I am happy to try experimenting with a modification to p4c that removes that restriction, and test bmv2 to see if it seems to behave correctly with default_action definitions on tables with action profiles and action selectors. I agree that if bmv2 does not support them as it is written today, I suspect it would be a very small change to enable it to work for default_action's on such tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit comment by Mihai was "bmv2 does not support default_action for all tables", so apparently he believed that was true at that time. For all I know, perhaps it was true for bmv2 in May 2016.
Yes that would have been true then given that #551 is from 2018
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are OK reviewing this PR as it is right now, I am happy to make a new issue to track the following things, perhaps modified in later PRs:
(a) consider modifying p4c bmv2 back end so that it supports default_action in P4 source code for tables with action profiles or action selectors.
(b) Document the key base_default_next
, including mentioning what it was for originally, and any known cases where its value affects the behavior of bmv2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p4c issue to track idea (a) above: p4lang/p4c#5101
behavioral-model issue to track idea (b) above: #1289
docs/JSON_format.md
Outdated
@@ -634,14 +634,14 @@ attributes for these objects are: | |||
- `action_id`: the id of the default action | |||
- `action_const`: an optional boolean value which is `true` iff the control | |||
plane is not allowed to change the default action function. Default value is | |||
`false`. It can only be set to `true` for `simple` tables. | |||
`false`. It can only be set to `true` for `simple` tables. See Note 2 below. | |||
- `action_data`: an optional JSON array where each entry is the hexstring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this attribute (action_data
) always set by p4c nowadays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed some additional checks that threw exceptions if action_id
or action_data
were ever absent inside of a default_entry
object value, and they were always found for all p4c tests, so it seems very likely that they are guaranteed to be present. default_entry
is always present for tables with type simple
, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to throw an error if default_entry
is not present in a table with type simple
led to updating more test cases than I expected. I can back out those changes if they seem like busy-work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change these old test cases, it just creates noise. However, I think you should mention it in the Node 2 below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just so I am clear on your meaning, your recommendation is:
(a) document that default_entry
is included in all simple tables in today's p4c
(b) remove the check that throws an exception if default_entry
is not present in a simple table
(c) remove all changes to old tests
Yes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the code that threw an error if default_entry
was absent in a table definition, restoring it back to optional, as it was before this commit. I also removed all the commits in the old unit test cases that were needed to make those old unit tests pass with such a change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should change these old test cases, it just creates noise. However, I think you should mention it in the Node 2 below.
I have added to Note 2 in the JSON docs a mention that p4c always include key default_entry
for tables with type
equal to simple
, and always omits default_entry
for other types of tables.
Signed-off-by: Andy Fingerhut <[email protected]>
Signed-off-by: Andy Fingerhut <[email protected]>
dafcf39
to
c660463
Compare
It requires changing many old unit tests if we require default_entry as a key in all table definitions. Signed-off-by: Andy Fingerhut <[email protected]>
inside of default_entry value. Signed-off-by: Andy Fingerhut <[email protected]>
…a table Signed-off-by: Andy Fingerhut <[email protected]>
docs/JSON_format.md
Outdated
- `action_data`: an optional JSON array where each entry is the hexstring | ||
value for an action argument. The size of the array needs to match the | ||
number of parameters expected by the action function with id `action_id`. | ||
This is required if `default_entry` is present. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct.
The request was more about including in Note 2 that:
p4c always include the
action_data
attribute in thedefault_entry
object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do add this new restriction, we should update the phrasing above ("an optional JSON array").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in commit 9.
} else { | ||
throw json_exception( | ||
EFormat() << "'default_entry' of table '" << table_name | ||
<< "' should have key 'action_data'", | ||
cfg_default_entry); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above. I am not sure we need to enforce this new restriction. But I don't feel very strongly about it, so if you think this is more meaningful this way, we can add it. I assume it won't break the test JSON files included in this repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All behavioral-model CI tests are passing, as you can see on Github page for this PR.
I have also run all p4c tests with behavioral-model with the changes on this PR, and this exception never occurs.
I am OK removing the check if you prefer. The main reason I even include it is that it makes it more clear what behavioral-model can rely on going forward, and reduce the number of cases that we need to think about when modifying code related to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated docs in commit 9 to make it clearer that action_data
key is required inside of default_entry
.
… required Signed-off-by: Andy Fingerhut <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
In theory because we are making the rules more restrictive for what constitutes a valid JSON (I'm thinking of default_entry.action_data
being required), we should update the JSON format version:
behavioral-model/src/bm_sim/P4Objects.cpp
Lines 46 to 49 in 87e6bf3
constexpr int required_major_version = 2; | |
constexpr int max_minor_version = 23; | |
// not needed for now | |
// constexpr int min_minor_version = 0; |
In theory, the version would become 2.24, and both max_minor_version
and min_minor_version
would need to be set to 2.24. That would create a bit of an headache potentially, as you would need to update p4c as well and regenerate all the JSON files there to use version 2.24.
Now given that all JSON files nowadays are generated by p4c, and the updated requirements for the JSON format should match what's p4c has been doing for several years, updating the version in this way may not be worth the trouble...
I haven't reviewed the code in detail yet, but there is a min_minor_version and max_minor_version, which could be set to a range of allowed minor versions that the current program expects, and perhaps a different range for what the updated BMv2 from this PR would accept? I can think later about specific suggestions there, after reviewing what the range checking code does in BMv2. |
No description provided.