[do not merge] PSA definition #569

cc10512 · 2017-05-05T19:26:51Z

I'm providing this PR so that people can review the PSA definition as it stands today. I incorporated feedback from @hanw, @antoninbas, @ChrisDodd, @vgurevich, @mbudiu-vmw, and @jafingerhut. There are still a number of TODOs marked in this file. I would like to use this PR as the means to collect more feedback and provide a forum for the discussion that will allow us to finalize the PSA spec. Please share your thoughts.

- Comments on how to handle the semantics of multiple PRE invocations - and design option of splitting the PRE into PRE and BQE (Buffering and Queueing Engine) - added PRE as argument to Ingress and Egress - added missing 'in' qualifiers and random distribution - add size types instead of indefinite int - update with @mbudiu-vmw comments to pass through compiler - ActionProfile and ActionSelector externs - split PRE and BQE - unify user-defined metadata for defined controls

jafingerhut · 2017-05-06T17:17:35Z

p4include/psa.p4

+ * input parameter and an output parameter for each block. The
+ * compiler will have to check that the out parameter of a block
+ * matches the in parameter of the subsequent block.
+ *


This is not a suggestion to change anything, but just a comment on this choice for implementers. I suspect what you have in the code here now is easier for a compiler to optimize out any fields of user_meta that are not needed by any later code, to minimize the number of bits passed down a pipeline implementation.

Easier, that is, than having a different user_meta type passed between each pair of consecutive 'blocks', requiring the user's P4 program to copy what they want to keep (in many cases likely unchanged, but requiring the assignments anyway). Optimizing away those assignments sounds trickier (to me, not having thought about it very long) for a compiler than the 'optimize away unnecessary fields' approach aobve.

The advantage of defining metadata as subsets is that it sets clear bounds on the lifetime of the fields. Good liveness analysis in the compiler can make up for it. The disadvantage, as you say is the need to copy. However, if you have intrinsic metadata directly supported in the hardware, the copies will be unnecessary. In such cases, the analysis to remove redundant copies is quite trivial. So, I think that in terms of performance the two approaches are equivalent, the analysis are both doable, and it really comes down to a matter of taste! :) And given that we currently already support the flattened view in the compiler, I took the easy path out ...

jafingerhut · 2017-05-06T17:38:35Z

p4include/psa.p4

+ *   From this, it follows that if there is a call that sets the
+ *   multicast_group, the packet will be multicast to the group that
+ *   last set the multicast group. Otherwise, the packet will be sent
+ *   to the port set by either send_to_port.


'either' looks left over and should be removed. Or there should be additional possibilities besides 'send_to_port' mentioned as alternatives.

It is a textual representation of the if above, because the if does not actually capture the semantics of multiple calls. Not sure why you think it doesn't fit ...

The sentence makes perfect sense in English if you delete the word "either". Having the word there cannot make it clearer in any way I can think of, only make it appear like you meant to write "either A or B", but you forgot to write the "or B" part.

jafingerhut · 2017-05-06T17:41:07Z

p4include/psa.p4

+ * The semantics of calling the PRE APIs is equivalent to setting
+ * intrinsic metadata fields/bits and assumming that the PRE looks up
+ * the fields in the following order: drop, truncate, multicast,
+ * clone, output_port.


Everything in this list except 'output_port' is one of the API calls. Perhaps replace 'output_port' with 'send_to_port' for consistency?

Should recirculate, resubmit be part of this list?

they are actually the bits in the intrinsic metadata. I guess the statement needs clarification. We have intrinsic metadata defined in the v1model for most of these bits. What I intended to say is that one can think of those bits as "global state" and the API functions modifying the global state. The PRE looks at the state after all the functions have been called in the pipeline and makes the decision based on the state of the bits. I'll appreciate your help in rephrasing the paragraph accordingly.

I think my comment was more basic than that. The names drop, truncate, multicast, and clone are all names of methods in the PRE object. The only one that is not is output_port, which is the name of the argument for the method set_output_port. Is there a good reason to make that name a different kind of name than the others in the list?

Also, if you want to define the behavior in terms of state that is hidden beneath the API of an object, you may as well give a reference implementation of that API, yes? Otherwise, it seems best to define the behavior in terms of the API calls made, not the internal state variables.

jafingerhut · 2017-05-06T17:44:23Z

p4include/psa.p4

+                      out psa_ingress_output_metadata_t ostd);
+
+control Egress<H, M>(inout H hdr, inout M user_meta,
+                     PacketReplicationEngine pre,


From comments earlier, perhaps this egress PacketReplicationEngine was intended to be a BufferingQueueingEngine instead?

jafingerhut · 2017-05-06T17:48:55Z

p4include/psa.p4

+  crc32_custom,
+  crc16,
+  crc16_custom,
+  random,


Given the existence of the Random extern, of what use would a random HashAlgorithm be? I can't think of one. If it were there, it sounds like you would need to feed some packet header/metadata fields as input, which would simply be ignored, so why bother with the Hash extern?

Not sure. A random permutation of the bits in the input? As opposed to a random number from the Random's distribution? I'll put a comment so that folks who could think of how to use this can chime in.

jafingerhut · 2017-05-06T17:51:55Z

p4include/psa.p4

+}
+
+/// Counter: RFC-XXXX
+extern Counter<W, S> {


I don't know all of the RFCs by a long shot, but I'd be a little surprised if there is an RFC covering counters the way there is for meters. Counters are just too simple, unless maybe someone has gone to the trouble to make an RFC defining saturating vs. wrapping counters.

jafingerhut · 2017-05-06T18:01:13Z

p4include/psa.p4

+}
+
+struct psa_ingress_output_metadata_t {
+  PortId_t                 egress_port;


Is it intended to have both an egress_port field here, and the send_to_port() method in the PRE object? It seems simpler to have only one of the two, probably the PRE method, and eliminate this egress_port field. I could imagine having both, and specifying what happens if you both assign to the egress_port field and call send_to_port(), but why bother creating the potential for such confusion?

Good point! Lost this in translation from one approach to the other.

jafingerhut · 2017-05-06T18:06:57Z

p4include/psa.p4

+  @ControlPlaneAPI
+  {
+    reset(in MeterColor_t color);
+    setParams(in S committedRate, out S committedBurstSize


possible typo: 'out' should be 'in'?

jafingerhut · 2017-05-06T18:07:39Z

p4include/psa.p4

+  @ControlPlaneAPI
+  {
+    reset(in MeterColor_t color);
+    void setParams<S>(in S committedRate, out S committedBurstSize


Same possible 'out' -> 'in' typo here

jafingerhut · 2017-05-06T18:09:47Z

p4include/psa.p4

+ * Although registers cannot be used directly in matching, they may be
+ * used as the RHS of an assignment operation, allowing the current
+ * value of the register to be copied into metadata and be available
+ * for matching in subsquent tables.


I don't see how one could use a register in the RHS of an assignment, without a read() method call. If you mean the read() method call can be in the RHS of an assignment, then OK, but might be nice to be explicit about that in the comment.

typo: subsquent -> subsequent

Indeed, it is the read operation on the register that can be on the RHS. Fixed.

jafingerhut · 2017-05-06T18:11:13Z

p4include/psa.p4

+
+/// The set of distributions supported by the Random extern.
+enum RandomDistribution {
+  PRNG,


PRNG doesn't provide any information here. I mean, every one of these is pseudo-random, yes?

Perhaps 'Uniform' instead?

What do you think about specifying the distribution in general? It is likely that most hardware implementation of the Random extern will implement a fixed algorithm, and it is not clear how do we map all the PSA distribution to that one implementation. A software implementation could call different libraries and get the random distribution. So the question is: is it more portable to just say a random number generator with whatever algorithm is implemented in the target?

A uniform random generator is a useful thing on its own.

A uniform random generator plus some formulas using + - << >> and if's is enough to implement piecewise linear approximations of other generators.

A uniform random generator plus a lookup table with an exact key that is a uniform random value enables one to implement whatever distribution you can imagine, if you are willing to spend the table's storage to do it (and it can even be modified by the control plane at run time).

Given all of those choices, and given that a high performance target would probably implement non-uniform distributions using one of the methods above, I would think that uniform random would be sufficient, and probably all that some targets would be willing to implement anyway.

cc10512 · 2017-05-06T20:26:51Z

Andy, thanks a lot for the thoughtful feedback! I pushed another version addressing some of your comments.

vgurevich · 2017-05-22T14:58:22Z

p4include/psa.p4

+ * metadata and be available for matching in subsequent tables.
+ */
+extern Register<T, S> {
+  Register(S size);


I wonder if the constructor should be

Register<T>(S instance_count);

instead.

The reason is that the type of the value stored in the register should be provided to the constructor, not derived from usage.

Also, as a minor comment, I think that (S size) is not a very good name, because it is not clear whether this refers to the size of an individual register or the size of the register array. I propose we use (I instance_count) in the declaration and (I index) in the methods.

Constructors cannot have type parameters, they inherit the parameters of the class.

vgurevich · 2017-05-22T15:02:44Z

As a general comment, I think we need to settle on a clear naming scheme.

Should we use suffixes (e.g. _t) or not? When should we use them. What are they?
Should we use lower_case, CamelCase (or camelCase), or_moreComplex_scheme_t? (very often different objects use different naming schemes).

All these need to be outlined in the beginning.

vgurevich · 2017-05-22T15:04:47Z

p4include/psa.p4

+  ///
+  /// @param port The output port. If the port is PORT_CPU the packet
+  ///             will be sent to CPU
+  void send_to_port (in PortId_t port);


Can one query the current value (set by an action in a preceding table)? If not, why not?

vgurevich · 2017-05-22T15:06:56Z

p4include/psa.p4

+  /// @param multicast_group Multicast group id. The control plane
+  ///        must program the multicast groups through a separate
+  ///        mechanism.
+  void multicast (in MulticastGroup_t multicast_group);


What is the interaction between send_to_port()/multicast() and drop()?
Do they override each other?
How can one see that using these actions creates an action dependency (or does not)?
If one action calls send_top_port() and another one calls multicast(), how can a table later in the flow determine where the packet is currently going?

vgurevich · 2017-05-22T15:10:27Z

p4include/psa.p4

+  /// @param port          The port to send the cloned packet to.
+  ///
+  /// Note: based on discussions with Antonin, we can ignore session now
+  void clone     (in CloneMethod_t clone_method, in PortId_t port);


Are we saying that because clone() and drop() are allowed in the Egress() control, that the ReplicationEngine extern can be called in it too? That makes little sense to me, because it means that some methods of the same extern can be called only in the ingress, some in the egress and others in both.

Also, it puts a lot of additional pressure on the backend, since it has to determine the legality of a call by analyzing the actual value of clone_method. I do not think this is a good idea either

vgurevich · 2017-05-22T15:11:50Z

p4include/psa.p4

+  /// @param clone_method  The type of cloning.
+  /// @param port          The port to send the cloned packet to.
+  /// @param data additional header data attached to the packet
+  void clone<T>  (in CloneMethod_t clone_method, in PortId_t port, in T data);


For all these 3 methods (clone(), resubmit() and recirculate()) it is totally not clear what happens to the T data.

vgurevich · 2017-05-22T15:13:17Z

p4include/psa.p4

+  ///
+  /// @param port The output port. If the port is PORT_CPU the packet
+  ///             will be sent to CPU
+  void send_to_port (in PortId_t port);


Sending to a port AFTER egress is typically not possible or extremely expensive on most modern architectures.

vgurevich · 2017-05-22T15:15:21Z

p4include/psa.p4

+  /// Truncate the outgoing packet to the specified length
+  ///
+  /// @param length packet length
+  void truncate(in bit<32> length);


What's the interaction of this primitive with the deparser and checksumming?

vgurevich · 2017-05-22T15:15:44Z

p4include/psa.p4

+///   bit<56> hash_value = h.getHash(16, buffer, 100);
+///   ...
+/// }
+extern Hash<Algo, O> {


Algo is not used

vgurevich · 2017-05-22T15:17:36Z

p4include/psa.p4

+
+/// Counter: RFC-XXXX
+extern Counter<W, S> {
+  Counter(S n_counters, W size_in_bits, CounterType_t counter_type);


We have to be careful here. For example if this is packets_and_bytes, then the width for the packet and byte counter (if less than 64 bits) is typically different and thus might require two separate arguments.

vgurevich · 2017-05-22T15:19:35Z

p4include/psa.p4

+/// Counter: RFC-XXXX
+extern Counter<W, S> {
+  Counter(S n_counters, W size_in_bits, CounterType_t counter_type);
+  void count(in S index, in W increment);


Why does one need to specify the increment? It should be 1 for packets and packet_length for bytes (the last one is often done using very tricky mechanisms). If someone needs anything else, that's what registers are for.

Also, why increment has the type W? Are you saying that you can't increment a 64-bit counter by more than 64 (or 128)?

vgurevich · 2017-05-22T15:20:12Z

p4include/psa.p4

+
+/// DirectCounter: RFC-XXXX
+extern DirectCounter<W> {
+  DirectCounter(entry_key key, CounterType_t counter_type);


What is entry_key????

vgurevich · 2017-05-22T15:21:18Z

p4include/psa.p4

+    bytes
+}
+/// Meter colors
+enum MeterColor_t { RED, GREEN, YELLOW };


Enum is a bad idea here, because there is a need to store this value in both the metadata and the packet.

@vgurevich I have never heard of a use case for storing this red/green/yellow value in the packet. Have you? Mostly I have seen a 3-way branch in the code after updating the meter, that does different things depending upon the value of red/green/yellow, e.g. drop on red, modify DSCP on yellow, no-op on green. That said, I have no problem using types that are easily converted to/from bit vectors.

vgurevich · 2017-05-22T15:51:12Z

p4include/psa.p4

+///
+/// @param W    The width of the checksum
+extern Checksum<W> {
+  Checksum(HashAlgorithm hash);          /// constructor


W should probably be used in the constructor, i.e.Checksum<W>(HashAlgorithm algo)
This is probably an overkill (to use the same list of algorithms as for a hash unit).
Naming is off

vgurevich · 2017-05-22T15:54:38Z

p4include/psa.p4

+  {
+    W    read<W>      (in entry_key key);
+    W    sync_read<W> (in entry_key key);
+    void set          (in W seed);


What does seed mean in the counter context?

vgurevich · 2017-05-22T15:55:36Z

p4include/psa.p4

+ * entries (members) into the extern, they are essentially populating
+ * the corresponding table entries.
+ */
+extern ActionProfile {


Is the extern instantiated in the table or outside? If that's inside a table, then sharing of action profiles will become difficult.

vgurevich · 2017-05-22T15:58:40Z

p4include/psa.p4

+/// stream (copying or redirecting).
+extern Digest<T> {
+  Digest(PortId_t receiver); /// define a digest stream to receiver
+  void emit(in T data);      /// emit data into the stream


How do we plan to ensure that the data is "latched", i.e. that changing the data after the method has been called does not affect the results.

This is a bmv2 specific question. The current compiler always copies the data to a temporary for digest.

vgurevich · 2017-05-22T16:00:27Z

p4include/psa.p4

+parser Parser<H, M>(packet_in buffer, out H parsed_hdr, inout M user_meta,
+                    in psa_parser_input_metadata_t istd);
+
+control VerifyChecksum<H, M>(in H hdr, inout M user_meta);


What's the mechanism to pass checksum errors? user_meta?

What is the rationale to have separate controls for checksum verification/update instead of doing it in parser/deparser correspondingly?

vgurevich · 2017-05-22T16:01:44Z

p4include/psa.p4

+  PortId_t                 ingress_port;
+  InstanceType_t           instance_type;  /// Clone or Normal
+  /// set by the runtime in the parser, these are not under programmer control
+  ParserStatus_t           parser_status;


What's the relationship between error and ParserStatus_t. What about checksum errors?

cc10512 · 2017-07-04T22:38:34Z

superseded by #618

cc10512 requested a review from hanw May 5, 2017 19:26

jafingerhut reviewed May 6, 2017

View reviewed changes

addresses Andy's comments

d032a70

cc10512 force-pushed the cc/psa branch from 29793b0 to d032a70 Compare May 6, 2017 20:28

jafingerhut mentioned this pull request May 8, 2017

PSA: problem definition p4lang/p4-spec#119

Closed

vgurevich reviewed May 22, 2017

View reviewed changes

p4include/psa.p4

/// bit<56> hash_value = h.getHash(16, buffer, 100);

/// ...

/// }

extern Hash<Algo, O> {

Copy link

vgurevich May 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Algo is not used

vgurevich reviewed May 22, 2017

View reviewed changes

jafingerhut mentioned this pull request May 24, 2017

adds std arch draft p4lang/p4-spec#122

Merged

cc10512 closed this Jul 4, 2017

fefer mentioned this pull request Aug 7, 2018

"make" get's 100% without error, but "make check" fails #1433

Closed

[do not merge] PSA definition #569

[do not merge] PSA definition #569

Conversation

cc10512 commented May 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut May 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut May 6, 2017 • edited Loading

Choose a reason for hiding this comment

cc10512 commented May 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgurevich commented May 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgurevich May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgurevich May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgurevich May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc10512 commented Jul 4, 2017

jafingerhut May 6, 2017 •

edited

Loading

jafingerhut May 6, 2017 •

edited

Loading

jafingerhut May 6, 2017 •

edited

Loading

vgurevich May 22, 2017 •

edited

Loading

vgurevich May 22, 2017 •

edited

Loading

vgurevich May 22, 2017 •

edited

Loading