-
Notifications
You must be signed in to change notification settings - Fork 22
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
Function to set expire timer of an added entry. #44
Conversation
// Examples: | ||
// set_expire_time_if({hdr.tcp.flags, meta.direction}, | ||
// {TCP_FLG_SYN, OUTBOUND}, | ||
// tcp_connection_start_time_profile_id); |
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 the new parameters for set_expire_time_if
were a single additional parameter in bool condition
, then the example above could be written as shown below:
set_expire_time_if((hdr.tcp.flags == TCP_FLG_SYN) && (meta.direction == OUTBOUND),
tcp_connection_start_time_profile_id);
@roop-nvda Similar to the discussion we had on I have added another comment to your PR showing how one of your example calls to As for the discussion on #43, it seems to me that doing it this way lets one express the functionality you want, as well as more general conditional expressions, e.g. |
@jfingerh Yes: A conditional would work. A conditional is also more expressive, like you point out, where it can use "greater than", "greater or equal", etc. However, a conditional can have sub-clauses that can be arbitrarily deep (as represented in an IR tree), and it's evaluation complexity is greater than a simple field match (because of clauses like "greater equal"). Since we are targeting line rate match actions, and starting from a place where almost no conditionals are supported in match actions in several targets, economy of evaluation inside the match action should be considered. For targets where such economy is not needed, and evaluation of boolean expressions as conditionals is feasible, the current spec of Considering targets where match actions have limitations: it has been suggested that a compiler can, during compilation, reject a boolean expression argument that is too complex for a particular target. While that is a valid recourse, expressing the reason for rejecting the expression, and particularly, the way to fix the expression would be hard to convey to the programmer. A tuple of fields to match, as proposed by this PR seems to us to be a simpler for a P4 programmer to use and an implementation to specify -- Acme network card supports a maximum size 4 tuple as the first arguments to |
So suppose in the kind of target that you are thinking of, the back end of the P4 compiler checked the conditional expression's IR to see if it met the following conditions. This seems like a fairly straightforward thing for a back-end to check in time linear in the number of IR nodes of the expression, i.e. it would be very fast and simple:
If those were true, then it is a straightforward linear time operation (at compile time, not run time in the data plane) to know that any expression If those conditions were not true, the back end of the target you have in mind would reject the condition as unsupported (for that target device). Compilers for particular P4 targets are allowed to reject programs as too complex. They do it for any number of reasons all the time, differently for each back end target device, depending on the details of the target's capabilities. In a case like this, the error message for rejected expressions could even be very helpful in the back end: "supported conditional expressions must be of the form The restrictions of such a back end need not restrict OTHER P4 compiler back ends for other targets that might have different restrictions, though. Is that 100% portable? No, of course it isn't. But we know that different targets will have restrictions that differ between them like: maximum number of recommended or hardware-permitted recirculate operations per packet, maximum number of table apply operations supported in a single pass, etc. I don't think anyone involved in PNA wants to try to mandate particular numerical values for those things that all targets must comply with -- they will vary today, and they will likely vary across different devices from a single vendor, too. |
Apologize if it was not clear, but I was not suggesting that numerical values of the size of tuple argument of When compiling for bmv2 simple switch backend with a conditional in a match action we get an error message implying that only C/C++ ternary assignment type conditionals are allowed -- |
Maybe I am answering a different concern than you are worried about, but note that the bmv2 back end today can support assignment statements like this inside of P4 actions, as demonstrated by the attached program:
since it can implement that with no changes, I suspect that it should also be able to implement an action like this with the only change required to be defining and implementing an extern
|
@roop-nvda Here is another angle on the question. Suppose we did say we were only going to implement your proposed version of Then it is perfectly legal P4 syntax to write this:
because So trying to require two parameters that must be equal, trying to restrict the generality of conditional expressions that can be expressed, actually does not restrict it at all. |
@jfingerh -- Agreed. I will amend and resend the PR. |
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.
Per discussion in the call last week, I have uploaded this PR with the proposed function to set expiry timer along with some examples.