Closed mariobaldi closed 1 year ago
Hi @mariobaldi, this is the ONF bot 🤖 I'm glad you want to contribute to our projects! However, before accepting your contribution, we need to ask you to sign a Contributor License Agreement (CLA). You can do it online, it will take only a few minutes:
✒️ 👉 https://cla.opennetworking.org
After signing, make sure to add your Github user ID mariobaldi
to the agreement.
For more information or help:" https://wiki.opennetworking.org/x/BgCUI
Thomas Calvert asked a question about: since this proposed new feature could be used to implement counters, is there a reason to keep Counter and DirectCounter externs?
I definitely think those externs should be kept, because they provide "update only" semantics from the data plane APIs, i.e. the data plane is not allowed to read the values of such extern-based counters, and this is important in enabling cheaper implementations of such counters.
I would definitely encourage anyone interested to remind us to add an appendix/footnote/paragraph/whatever to remind P4 developers of this in the PNA spec.
One simple example: fast path data plane might only represent the least significant 32 bits of a 64-bit packet/byte counter in the data plane, and periodically atomically read&clear data plane counters in a driver, and add the values to 64-bit packet/byte counters running on a hefty CPU like a Xeon nearby with lots of cheap DRAM available. Such an implementation would NOT have the full counter available in the data plane most of the time, but give correct values to control plane APIs for reading counters.
Here is a fancier technique published some time ago: https://dl.acm.org/doi/10.1145/885651.781060
I think the first 2 lines of each file should be swapped to fix the build. Otherwise LGTM. 👍
@mariobaldi It would be good to put these in a separate directory for now, until the p4test front end compiles them without errors, to avoid Github CI issues.
@mariobaldi I spent a little time editing your 2 example programs so that p4test
compiles them with 0 error messages using the latest version of p4c code, and the latest version of the pna.p4 include file in this repo. The modified files are in the attached progs.zip file on this comment.
I removed the rmw
keyword from the action that had them. I could #ifdef those if you prefer.
If you would like me or someone else to create a new PR with these versions of the programs, let us know. progs.zip
@pbhide asked: "Do we expect all types of table action parameters to be writable (e.g. TCAM tables...)?"
My personal answer: A target supports what the target developers choose to enable it to support. Some targets might enable this, and others might not.
Ideally a target's P4 compiler would reject a P4 program with a compile-time error if any part of it is not supported by that target/compiler, with an error message explaining why. That is up to the target's P4 compiler back end to check.
@mariobaldi In your program pna-example-tcp-connection-state-tracking.p4, the ct_tcp_table_miss action assigns an initial value for either the parameter n2h_seqNo
, or the parameter h2n_seqNo
, but never for both of those, and never assigns an initial value for the action parameters n2h_ackNo
or h2n_ackNo
Thus if the first packet to execute action ct_tcp_table_hit
for a flow has SYN=0, that action will read uninitialized values of n2h_ackNo
or h2n_ackNo
.
I am sure my earlier TCP connection tracking example is also full of such possible behavior, and I added a comment to it trying to make it explicit that it was primarily a demo of the add_on_miss
table property and add_entry
extern function call.
Would you consider these examples similarly as examples of using assignable action parameters that maintain their values across packets, which might work usefully if packets arrive in the expected sequence, but anyone wanting a production-quality tested version must write their own? If so, makes perfect sense to me.
@mariobaldi I spent a little time editing your 2 example programs so that
p4test
compiles them with 0 error messages using the latest version of p4c code, and the latest version of the pna.p4 include file in this repo. The modified files are in the attached progs.zip file on this comment.I removed the
rmw
keyword from the action that had them. I could #ifdef those if you prefer.If you would like me or someone else to create a new PR with these versions of the programs, let us know. progs.zip
Thanks for doing this @jfingerh. I don't quite get why we don't just commit the changes to this PR, rather than creating a new one?
@mariobaldi In your program pna-example-tcp-connection-state-tracking.p4, the ct_tcp_table_miss action assigns an initial value for either the parameter
n2h_seqNo
, or the parameterh2n_seqNo
, but never for both of those, and never assigns an initial value for the action parametersn2h_ackNo
orh2n_ackNo
Thus if the first packet to execute action
ct_tcp_table_hit
for a flow has SYN=0, that action will read uninitialized values ofn2h_ackNo
orh2n_ackNo
.I am sure my earlier TCP connection tracking example is also full of such possible behavior, and I added a comment to it trying to make it explicit that it was primarily a demo of the
add_on_miss
table property andadd_entry
extern function call.Would you consider these examples similarly as examples of using assignable action parameters that maintain their values across packets, which might work usefully if packets arrive in the expected sequence, but anyone wanting a production-quality tested version must write their own? If so, makes perfect sense to me.
Yes, you are right. I would consider these just as sample examples. In normal conditions the next packet hitting the entry should have a SYN=1.
@mariobaldi It would be good to put these in a separate directory for now, until the p4test front end compiles them without errors, to avoid Github CI issues.
I guess we don't need it if we commit in this PR @jafingerhut changes in the zip files. Otherwise, I will move them.
@mariobaldi If you want to take the edited programs in the zip file I attached to an earlier comment, and update this PR to use them, then they should compile cleanly with the latest version of open source p4test, and thus no problems with CI if the programs are in the example directory, and no reason to create a separate PR.
Example of how rewritable table entries can be used to implement state tracking of TCP connections. A full example code with creation of entries in a TCP connection table using the add_on_entry PNA feature, as well as a simplified example focusing exclusively on rewriteable entries are provided.