p4lang / pna

Portable NIC Architecture
Apache License 2.0
55 stars 21 forks source link

Function to set expire timer of an added entry. #44

Closed roop-nvda closed 2 years ago

roop-nvda commented 2 years ago

Per discussion in the call last week, I have uploaded this PR with the proposed function to set expiry timer along with some examples.

jfingerh commented 2 years ago

@roop-nvda Similar to the discussion we had on add_entry_if in this issue https://github.com/p4lang/pna/pull/43 my main comment would be to express my strong preference that if we want to create extern functions that have explicit parameters to make the side effects conditional, that we do so with a single in bool condition parameter rather than two parameters val1 and val2 that restrict the condition to val1 == val2.

I have added another comment to your PR showing how one of your example calls to set_expire_time_if examples could be rewritten if it has such a bool first parameter.

As for the discussion on https://github.com/p4lang/pna/pull/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. (a < 5). If a target device cannot support (a < 5), then that is up to the compiler for that target to say it is too complex, preferably with links to documentation on what kinds of expressions that target supports in such conditions.

roop-nvda commented 2 years ago

@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 add_entry may suffice, since the condition can be expressed in an enclosing if clause.

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 add_entry_if and set_expire_time_if.

jafingerhut commented 2 years ago

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 (a1 == b1) && (a2 == b2) && ... is equivalent to {a1, a2, ...} == {b1, b2, ...}.

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 (a1 == b1) && (a2 == b2) && ... where all a's and b's are either header or metadata fields, or constants.

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.

roop-nvda commented 2 years ago

Apologize if it was not clear, but I was not suggesting that numerical values of the size of tuple argument of set_expire_time_if be mandated in PNA. I merely gave an example with a fictitious target -- Acme -- to illustrate that the error message was simple for the programmer. Different targets are indeed expected to support different sized tuples for matching, but the semantic of a match is simple -- (data[0] == value[0]) && (data[1] == value[1]) ... && (data[N-1] == value[N-1])-- and obvious across the class of targets.

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 -- only conditionals like if (a > 5) { a = 5;} are allowed in match actions for this target. Different targets expressing this and other classes of conditionals that are supported/unsupported in an error message in a way that helps the programmer fix the program can get cumbersome.

jfingerh commented 2 years ago

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:

    action a1() {
        b1 = (hdr.ethernet.srcAddr == hdr.ethernet.dstAddr) && (hdr.ethernet.etherType == 0x0800);
    }

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 set_expire_time_if that has a first parameter with type in bool:

set_expire_time_if((hdr.tcp.flags == TCP_FLG_SYN) && (meta.direction == OUTBOUND),
                   tcp_connection_start_time_profile_id);

v1model_bool_expressions_supported1.p4.txt

jafingerhut commented 2 years ago

@roop-nvda Here is another angle on the question. Suppose we did say we were only going to implement your proposed version of set_expire_time_if, where the first two parameters must be equal in order for the side effect to be performed.

Then it is perfectly legal P4 syntax to write this:

    set_expire_time_if(a < 5, true, tcp_connection_start_time_profile_id);

because a < 5 is an expression that evaluates to a type bool value, and that is the same type as the second parameter.

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.

roop-nvda commented 2 years ago

@jfingerh -- Agreed. I will amend and resend the PR.

roop-nvda commented 2 years ago

@jfingerh, Please see new commit added to this PR -- 2e71be3cd172ea7e69d3a108d6c442014c4bd4a9.