Alternate design of preserving metadata in PSA

[jafingerhut]

It is not completely different, but it does avoid many of the "extra"
parameters to the parsers and deparsers for carrying metadata.
Instead it introduces many extra control blcoks, each with a provided
"empty" or "no op" implementation, that contains the "packing" and
"unpacking" code that in PSA v1.1 are in the parsers and deparsers.

[jafingerhut]

@cc10512 This is not intended to be merged any time soon (and perhaps never). It is a somewhat fleshed-out look at what several of the PSA programs would change to with the idea I had for a slightly different way of handling preserved metadata.

[cc10512]

I like the direction. The one improvement that I would suggest is to make these additional controls optional, or simply provide a default empty control implementation, such that a P4 program that does not need them, does not refer to them at all. I believe the named arguments support that we currently have in the compiler is almost there.

BTW, I assume this is what we'll discuss later today.

[jafingerhut]

Yes, it is what I planned to talk with you about later today.

There are default "empty" packer/unpacker implementations in the propose psa.p4 include file changes, and those are used in the example programs I updated.

I did try to make these additional package variables optional, but p4c today didn't like what I tried. Either it has a bug in handling default parameter values to packages, or my experiment was wrong somehow. In writing this, I just realized that I neglected to use an @optional annotation on them, which might be why it didn't work for me. I will try that.

[jafingerhut]

One aspect of this that is perhaps more minor: it should be easier to implement the compiler for this approach.

Why? Because the PSA v1.1 approach requires checking that all assignments to output parameters like recirculate_meta are inside of an if statement with the correct condition. With this approach, everything in the RecirculatePacker is effectively inside of such an if statement, automatically, and it is impossible inside of the RecirculatePacker control to write code that affects any other packet paths, e.g. normal, clone, or resubmit.

[jafingerhut]

Created an issue for p4c asking if it is possible to have a package declaration with default values for its parameters, and then instantiate that package. I could not figure out how to get that to work: p4lang/p4c#1914

[jafingerhut]

Notes from discussing this proposal with Calin on 2019-Apr-29:

• For *Packer controls, he recommends that yes, they should take the user-defined headers type H as in parameter. Why not? The headers should be available at that point in the pipeline anyway, and if they were not in parameters to a *Packer control, and someone wanted to preserve one or more header fields, it would force the programmer to copy the header into a new user-defined metadata field for no other reason than that missing in H hdr parameter.

I tried before and after the meeting to make a package that had default values of "Empty*Packer()" for some of the parameters, but could not figure out how to get p4c to compile without an error. Created an issue, but looks like there was already an issue filed earlier for this: p4lang/p4c#1639

[jafingerhut]

Another thing that may as well be asked now, if we make changes like this: Should there be a VerifyChecksum control after IngressParser, before Ingress, like in v1model? And UpdateChecksum after Egress, before EgressDeparser? I don't have any strong for or against arguments there, but this seems like the right time to think about it.

[jafingerhut]

Brief summary of what I see as properties of this proposal, vs. PSA v1.1 way of doing things:

• If you want to write a PSA program that doesn't use "bridged metadata" for normal unicast/multicast packets sent from ingress to egress, and either does not use recirculate, resubmit, or clone operations, or it uses those operations but does not need metadata to be preserved for those operations, then this proposal will not affect how you write your program at all, except that you start from a different "skeleton template" PSA program.

That "skeleton template" would now have about a dozen fewer parameters to both parsers and the deparsers. Depending upon how this p4c issue [1] turns out, there could be about that many more parameters to the instantiation of the Ingress and Egress pipeline packages, but hopefully not.

[1] p4lang/p4c#1914

• If you want preserved metadata for one of those cases, e.g. recirculate, then you have to write a body for a RecirculatePacker and a RecirculateUnpacker control. Those controls are very restricted in their contents, basically having output parameters that are modified by copying fields from some or all of the input parameters. Similar to the v1model architecture deparser controls, they are highly restricted in their contents: they cannot have tables, extern instantiations, etc.

• A P4 developer no longer needs to remember and write the correct conditions in their parsers for branching on different kinds of packets like new-from-port vs. recirculated vs. resubmitted in the ingress parser, or normal unicast/multicast vs. clone I2E vs. clone E2E in the egress parser. All of that branching can be taken care of in a straightforward manner by the compiler (I believe).

• Similarly the deparsers in this new proposal are now left with no code in them for creating digests are filling initializing metadata to be preserved with these other packet paths. They will now be restricted to emit calls, and depending on our discussion, perhaps also code for updating checksums. All code for those other functions would move into their own small controls that do nothing but those focused things. Also, the P4 developer need not remember to wrap the proper if conditions around those things, as the compiler can do that correctly, automatically, and fairly simply.

• It should be easier to implement the compiler with this change, I believe. Without this change, a compiler should ideally check and restrict that the deparser code for initializing recirculated packet metadata is guarded by the proper if condition, and nothing else is inside of such if statements. With this change, no such checks are needed, because every line of code in a RecirculatePacker control will automatically be wrapped in the correct if condition by the compiler, and nothing else can be put into the RecirculatePacker control, because it has no access to emit calls, or the output packet, and can have no side effects on anything except the recirculated metadata.

[hanw]

Picking up the baton on this, as we are making more headways on supporting psa. This design is nice and we have more resource to make it happen both on the open-source side as well as internally. Let's resume this effort and try to make it into psa spec? @jafingerhut

[jafingerhut]

Sounds like a good idea to me. This and a few other things are probably worth having a meeting or three about to discuss and finalize.

[hanw]

Meeting of any form is fine with me. Can you send an email with items to discuss?