Change "private" v1model standard_metadata fields to BMv2 packet regi… #768
Conversation
…sters Remove the use of the following 4 fields from BMv2 simple_switch and simple_switch_grpc. They no longer need to be in the BMv2 JSON file for such programs, and clone, recirculate, resubmit, and digest operations are still able to work without them there, because they are now stored in a BMv2 "packet register", similar to how the packet length field currently is. + clone_spec + resubmit_flag + recirculate_flag + lf_field_list
|
A first draft of modifying simple_switch and simple_switch_grpc so that they no longer access clone_spec, lf_field_list, resubmit_flag, and recirculate_flag in the BMv2 JSON file, and those fields thus need not be in the JSON file at all in order for the corresponding features to work. If any of this looks like bad C++, it probably is. Let me know what I've done badly, and hopefully I will learn. |
|
Forgot to mention: I have run all of the p4c automated tests on my local dev machine with this modified version of simple_switch, and except for clone-bmv2.p4 they all pass. That test appears fairly old, and has no STF test, so could probably be removed. I also got all of those tests to pass when run with some slightly different changes that continued to maintain the fields the old way and the new way simultaneously, and crashed the program if the values ever differed when they were read in order to be used. That gives me pretty high assurance that it behaves identically with the old way of doing things. |
Codecov Report
@@ Coverage Diff @@
## master #768 +/- ##
==========================================
+ Coverage 74.32% 74.33% +<.01%
==========================================
Files 115 115
Lines 10136 10136
==========================================
+ Hits 7534 7535 +1
+ Misses 2602 2601 -1
Continue to review full report at Codecov.
|
targets/simple_switch/primitives.cpp
Outdated
| f_clone_spec.add(f_clone_spec, clone_spec); | ||
| void operator ()(const Data &mirror_session_id, const Data &field_list_id) { | ||
| auto &packet = get_packet(); | ||
| // Assume mirror_session_id < (1u << 15) so that we can use most |
There was a problem hiding this comment.
Could we enforce this when the control-plane creates a session here: https://github.com/p4lang/behavioral-model/blob/master/targets/simple_switch/simple_switch.cpp#L78, i.e. log an error and return false is the session id is >= (1 << 15)?
Also add range checking on mirror session id values when control plane tries to add or delete one.
| @@ -75,13 +76,23 @@ class SimpleSwitch::MirroringSessions { | |||
| bool add_session(mirror_id_t mirror_id, | |||
| const MirroringSessionConfig &config) { | |||
| Lock lock(mutex); | |||
| sessions_map[mirror_id] = config; | |||
| return true; | |||
| if (0 <= mirror_id && mirror_id <= RegisterAccess::MAX_MIRROR_SESSION_ID) { | |||
There was a problem hiding this comment.
you must be the kind of person that uses 100[array] for array access :)
There was a problem hiding this comment.
Only in an obfuscated code contest :) But I'm missing what about that code change was odd enough to make you think that?
There was a problem hiding this comment.
I think it's just a mater of habit, I never write 0 <= x instead of x >= 0, so my brain was frozen for a second.
…sters
Remove the use of the following 4 fields from BMv2 simple_switch and
simple_switch_grpc. They no longer need to be in the BMv2 JSON file
for such programs, and clone, recirculate, resubmit, and digest
operations are still able to work without them there, because they are
now stored in a BMv2 "packet register", similar to how the packet
length field currently is.