V1model doc enhancements #1892
V1model doc enhancements #1892
Conversation
|
@hesingh This includes a new note in the docs of the hash extern function explicitly saying that the types of the different parameters may be different. Not sure if you are interested in reviewing the other new documentation added here. |
|
I reviewed the |
|
@hesingh The "meter state" mentioned is not documented, because it is an internal detail that could differ from one implementation to another. It typically includes a "token count", and sometimes a last updated time, but that is not important for how to create or use a meter, only for implementing one. I have never seen one that contains a green/yellow/red value, because what is returned the next time the meter is updated from the data plane code will in general differ depending upon the time pattern of the update calls. Either that or perhaps I misunderstood your question? If you look at the documentation for the execute_meter call, it describes the 3 possible numeric values that can be written to the 'result' parameter. |
|
Stated another way, the 'size' parameter for the counter and meter extern are used for exactly the same purpose, and the documentation is intended to reflect this. They are the user-specified size of the array of counter/meter states. If you want a per-ingress-port meter, and you have 57 ingress ports, you should create a meter object with size 57. |
|
@jafingerhut The second reply answered my question - thanks! I think, it makes sense to add the example to documentation. "They are the user-specified size of the array of counter/meter states. If you want a per-ingress-port meter, and you have 57 ingress ports, you should create a meter object with size 57." |
|
@hesingh I have added some similar text as an example, although I felt a little odd about using the number 57, so used a power of 2 instead :) |
|
@mbudiu-vmw I am going to avoid merging these changes until after your #1704 has been successfully merged, as I believe it is easier for me to update this PR after that one has merged, than vice versa. |
p4include/v1model.p4
Outdated
| * The BMv2 implementation of the v1model architecture ignores the | ||
| * value of the receiver parameter. | ||
| * | ||
| * TBD As of 2019-Apr, the current BMv2 simple_switch implementation |
There was a problem hiding this comment.
If this is true then we have to update the resubmit patch to also handle digest in a similar way.
There was a problem hiding this comment.
for the bmv2 behavior @antoninbas is likely a better resource!
There was a problem hiding this comment.
@mbudiu-vmw I have updated this PR to say that it is definitely the case that the values sent to the control plane because of the P4 program executing a digest call are the values those fields have at the end of execution of the ingress control, not at the time the digest call is made, so it is like recirculate, resubmit, and clone3 operations in that regard, although it does not cause any values to be "restored" at the beginning of ingress or egress processing the way those other operations do.
@hanw confirmed this in private communication, and I have also examined the BMv2 simple_switch implementation to verify that it does this for P4_14 programs.
There was a problem hiding this comment.
Actually I believe that the compiler will do the right thing and copy these values to a temporary. So the semantics you get is that the fields are digested right away. This means that the comment is incorrect.
There was a problem hiding this comment.
I don't know. Just wanted to make sure people were aware of differences in behavior here.
I could change the documentation for digest to mention the difference between P4_14 and P4_16+v1model behavior, and warn people that the safest thing is to never change values given to a digest call after the digest call is made, if they want to avoid this behavior difference.
There was a problem hiding this comment.
@mbudiu-vmw I have updated the documentation about the behavior of digest, and how it can differ between the current P4_16+v1model implementation, vs. some P4_14 implementations. Let me know if it looks reasonable to you.
There was a problem hiding this comment.
@mbudiu-vmw I think this is ready to merge in now. Feel free to take a look at the current latest contents of the documentation on the digest function, which explicitly mentions how it behaves (create digest message contents from the current values of the data parameter), and also that some other implementations of P4_14 and, and perhaps some other P4_16+v1model implementations, might instead use the values of that parameter at the end of the ingress pipeline.
There was a problem hiding this comment.
I would have wanted to merge first my PR about recirculate, since otherwise I will have to rebase it again, and that's very painful. I am hoping someone will approve it.
There was a problem hiding this comment.
Understood. I had forgotten that PR also makes changes to v1model.p4. I'm happy for this one to wait a while longer, since it is only documentation comment changes.
For that other one to make progress, it seems like a few more people will need to express some kind of opinion on what approaches are viable, and which are not, and why.
| * control, only one packet is resubmitted, and only the data from the | ||
| * last such call is preserved. See the v1model architecture | ||
| * documentation (Note 1) for more details. | ||
| */ |
There was a problem hiding this comment.
If we merge the resubmit patches these will change.
There was a problem hiding this comment.
Understood. My goal here was not to get hyper-precise on future possible changes, or the existing broken behavior, but to focus on what resubmit/recirculate/clone do, independent of whether they preserve metadata or not. I am happy to update the docs when the new solution is decided upon and implemented.
jafingerhut
force-pushed
the
v1model-doc-enhancements
branch
from
0390740 to
c626c54
Compare
No description provided.