[P4-arch] First round of comments and questions on draft PSA

Andy Fingerhut andy.fingerhut at gmail.com
Sun Jun 25 12:24:08 EDT 2017


These comments are all for the version published here, as of 2017-Jun-25:

http://p4lang.github.io/p4-spec/docs/PSA.html

The HTML had the date "May 15, 2017" in the title page.

Andy



Typos:

"pipleline" -> "pipeline"
"templetized" -> "templatized"  (root English word is "template")
"NORMALINSTANCE_" -> "NORMAL_INSTANCE" ?
"ingres," -> "ingress,"
"bit<56>" -> "bit<16>" in example usage of hash function. A crc16 has
    a 16-bit output, not a 56-bit output.
"ivoked" -> "invoked"


This is not correct P4_16 syntax, according to the latest p4c-bm2-ss
as of 2017-Jun-16:

    typedef enum { NORMALINSTANCE_, CLONE_INSTANCE } InstanceType_t;

Perhaps use this instead?  It allows values to be defined later with
type "InstanceType_t".

    enum InstanceType_t { NORMALINSTANCE_, CLONE_INSTANCE }


What does this statement mean?

    "The PSA offers flexibility in defining the user metadata,
    however, one may chose to use a common metadata types for subsets
    of the objects."

"It is an error to instantiate the PRE multiple times."  From later
text, it appears that PRE is an extern object with no constructor, so
it cannot be instantiated by the user's P4 program.  It seems much
clearer to instead say: "The PRE extern object has no constructor, and
thus it cannot be instantiated in the user's P4 program.  The
architecture instantiates it exactly once, without requiring the
user's P4 program to instantiate it."

The same comments of the previous paragraph apply to the BQE.

What does the paragraph below mean?  Does it have any effect on how a
user writes P4_16 programs using psa.p4, or is it only intended to be
a note to implementers?

    Note: some of these operations may not be implemented as primitive
    operations on a certain target. However, All of the operations can
    be implemented as a combination of other operations. Applications
    that rely on non-primitive operations may incur significant
    performance penalties, however, they should be functionally
    correct.


For the text below defining the behavior of the drop operation, if the
intent is that any side effects from invoking other extern method
calls (e.g. counter updates, meter updates, register writes) that are
in the ingress control block after the drop operation is invoked still
occur, then it would be best to say that.  Also good to specify that a
packet dropped in ingress will never execute code in the egress
pipeline (if that is the intent).

    "While the drop operation can be invoked anywhere in the ingress
    pipeline, the semantics supported by the PSA is that the drop will
    be at the end of the pipeline (ingress or egress)."


The sentence below describing the truncate operation should specify
the units of the length, e.g. bits?  bytes?

    "The length parameter represents the packet length."


Why is there a send_to_port() operation defined for the BQE?  I would
have guessed that for the PSA, the egress port is determined during
ingress (via an ingress send_to_port() call for unicast packets, or
for multicast packets, each copy's output port is determined by the
PRE and how its multicast groups have been configured by the control
plane).  The packet scheduling algorithms that select which packet to
send to the egress pipeline next can be thrown off if the egress
processing can change the output port from what ingress determined,
yes?  Also, there is no 'out' metadata from the Egress control block
in Section 4.

Regarding this question: "are random hash algorithms useful?"  It is
not clear to me what is meant by a random hash algorithm.  If you mean
"a random value that is independent of the content of the packet and
its metadata", then that sounds like a pseudo-random number generator,
which is useful for things like implementing RED and similar
algorithms with randomized choices.  If you mean a hash algorithm that
takes as input some kind of "seed" value that is randomly generated,
plus some header fields, but the result is a deterministic function of
the seed and the header/metadata values, then that can be described as
a normal deterministic hash function with some input bits that were
randomly generated, and I would recommend it be implemented that way.
If something else is meant, better to add more detail to the question.


Section on "Hash function".  There is no description of what the 'max'
parameter of getHash does.


If the intent is for hashes and checksums to share the type
'HashAlgorithm', then HashAlgorithm should have the 16-bit 1's
complement checksum used by the IPv4 header to its choices.

The spec might want to note that 'remove' will only work for some hash
functions, and not others, e.g. it is easy to remove some data from a
1's complement checksum calculator, but much more complex, and
position-dependent, to remove it from a CRC calculation.


For counters that have a number of counters that is an exact power of
2, e.g. 1024, a single type S that is bit<10> is just right for the
'index' parameter to 'count', but it is 1 bit too small for the
'n_counters' parameter of the constructor.  Might be good to have a
separate type for the 'n_counters' paremeter?

For a 'bytes' or 'packets_and_bytes' counter type, count() having a
second parameter 'increment' that specifies how much to add to the
byte counter is a very good thing.  It is best if the P4 program has
the flexibility to choose the length in bytes it wants to use for the
packet, e.g. in case it is increasing or decreasing the received
packet's size before transmitting it.

For counters that have type 'packets', presumably every call to
'count' will add exactly 1, yes?  In that case, having a separate
method call that only takes an 'index' parameter would be best, since
the 'increment' parameter value would be ignored, anyway.

The behavior of the control plane API calls should be documented.

What is the intended meaning of 'sync_read'?

What do control plane calls 'start' and 'stop' do?

Suggested alternate names, if I understand the meanings correctly:
'write' instead of 'set'.  'clear' instead of 'reset'.  A
'read_and_clear' atomic operation is very common for counters, and
should be considered for addition, unless one of the other operations
is intended to do that.

If direct counters with a byte count are intended to be supported, it
should be specified what byte count is used for a packet.  Note:
Someone will be unhappy with the choice in at least some situations,
no matter what you choose.  You could recommend using simply packet
counters if they don't like the specified choice, or use an explicit
counter that is not direct, where they get to choose the length
themselves.


For meters, the length of the packet used for meters with type 'bytes'
must be specified.  Even better, add a 'length' parameter to the
'execute' call that lets the P4 program author choose what length to
use.  That will lead to the same issue for direct meters as was
mentioned for direct counters above, with the same solution -- don't
use direct meters when you want to choose a custom length.


"PRNG" as a choice for "RandomDistribution" doesn't mean anything to
me.  "Uniform" has a well-defined meaning in pseudo-random number
generation, and seems a better name to use, if that is what "PRNG" was
intended to mean.  I have commented before elsewhere that supporting
_only_ uniform seems quite reasonable.  If a P4 program writer wants a
non-uniform distribution, there are multiple ways to achieve that,
such as doing arithmetic on the uniformly generated value to make the
results non-uniform, or also performing a table lookup using the
uniform random value as the search key, so the control plane can
create whatever distribution it wishes.


In the section on 'Programmable blocks', it would be best if the
following things were specified explicitly.

+ The original value of fields inside 'user_meta' when the Parser is
  invoked.  Are they all undefined?

+ Is every field of 'hdr' and 'user_meta' given as input to
  VerifyChecksum supposed to be identical to the result output by
  Parser?  If so, best to say it explicitly.  Similarly for the rest
  of the pipeline.

+ An implementation will likely have limits on how much data it can
  pass through the packet buffer in the PRE.  Should the PSA spec say
  any words about this, and how it affects P4 program writers?  For
  example, it could affect portability of programs quite significantly
  if some programs expect they can carry 1000 bits of user-defined
  metadata with values written in the ingress control block, and have
  them all available in the egress block for use.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.p4.org/pipermail/p4-arch_lists.p4.org/attachments/20170625/a880e6cf/attachment-0002.html>


More information about the P4-arch mailing list