PSA Proposed description of PortId_t translation in PSA #562

jafingerhut · 2018-01-25T21:56:48Z

No description provided.

jafingerhut · 2018-01-25T21:58:13Z

Note: This is not the only way to achieve the goals described with these changes, of including PSA-specific field values in packet headers between controller and data plane, and handling PortId_t values. Comments are welcome. I just want to get this out there in its current form to see if it is anywhere near the ballpark of what we want in PSA.

jafingerhut · 2018-01-25T22:00:47Z

@samar-abdi I am not sure if I am able to add you as a reviewer in Github -- didn't find your user name on the list of possible reviewers to add, so notifying you with this comment. Please take a look and provide your comments.

cc10512

minor potential discrepancy with what was decided in P4Runtime WG w.r.t. ternary masks and LPM prefixes.

Looks good otherwise.

cc10512 · 2018-01-31T20:25:56Z

p4-16/psa/PSA.mdk

+brevity.
+
+As mentioned in section [#sec-psa-type-definitions], different PSA
+implementations are expected to differ in the minimum number of bits


Suggestion on rephrasing: different PSA implementations are expected to customize the size of the data types that refer directly to physical entities, such as ports.

I have replaced it with this "are expected to customize the size of the data types that refer directly to those objects, i.e. ports, multicast group ids, etc.". Something just seems a little off to me to refer to multicast group ids and instance ids as physical entities.

cc10512 · 2018-01-31T20:35:14Z

p4-16/psa/PSA.mdk

+to error.  When matching values of type `PortId_t` as part of a table
+key, always match a complete value exactly, or wildcard every bit of
+the value (i.e. a `ternary` match kind with all bits wildcard, or an
+`lpm` match kind with prefix length 0).  If you attempt to do any of


I believe P4Runtime disallows such behavior: all wildcards or LPM with prefix of 0. @antoninbas can confirm.

@cc10512 I am pretty sure P4Runtime allows one to create table entries that completely wildcard ternary fields, or do length 0 prefix of lpm fields -- it is just that they represent it in a way where the field isn't even mentioned in the P4Runtime API message.

My reference for this is this recent PR, including my clarifying question and Antonin's answer: p4lang/PI#293

@jafingerhut is correct; we do allow wildcards for ternary & LPM but require a certain formatting for the messages.

cc10512 · 2018-01-31T20:37:29Z

p4-16/psa/psa.p4

+typedef bit<8>  ClassOfServiceInHeader_t;
+typedef bit<16> PacketLengthInHeader_t;
+typedef bit<16> EgressInstanceInHeader_t;
+typedef bit<64> TimestampInHeader_t;


Should we explicitly state that we do not need translation annotations for these other types?

@vgurevich @antoninbas @samar-abdi Any thoughts on whether any of these other types would need conversion of values?

Can the control plane API just use whatever multicast group numbers and egress instance numbers it wants? I assume different implementations will have different numbers of these things that they support, and some might have more restricted numerical ranges than others. Does a P4Runtime API controller have a way to discover such restrictions on these values for different PSA devices in a heterogenous network?

Do we expect ClassOfService_t values to be numerically consecutive in a range [0, N] for all PSA devices, and should say that in PSA?

Same question for CloneSessionId values.

I think we are safe from such issues for PacketLength_t and Timestamp_t values, i.e. some implementations might have different maximum PacketLength_t values they actually support, but all of them will be [my_min, my_max] for packet length support for some my_min, my_max integers, and timestamps should be all X-bit-wide values for the value of X supported by the implementation.

Very likely class of service will need translation. It has the same issue as ports. Different controllers may have different notions of classes of service and their own numbering scheme for them. The switch config handler on the target will map the controller's class of service to the ASIC's. Other metadata values are either programmed by the controller (mcast_group/egress instance) or have universally agreed values (packet_length/timestamp) IMO.

So we should explicitly document that type ClassOfService_t fields will be translated between controller and P4 data plane programs, as PortId_t fields will be, but no others will ?

Do we expect, and should we perhaps document, that for all of the following types, the data plane and the control plane will both support all numerical values in the range [0, N] for whatever maximum value of N is supported by the PSA device (which can vary from one PSA implementation to another)?

typedef bit<unspecified> MulticastGroup_t; typedef bit<unspecified> CloneSessionId_t; typedef bit<unspecified> EgressInstance_t;

Yes for documenting translation of ClassOfService_t field values. The P4Runtime annotation will be cos_translation("controller_bitwidth:32").

As for your second point, I suggest : "The values of multicast_group, clone_session and egress_instance are completely in the purview of the controller, and are limited only by the bitwidth of the respective types on the PSA device(s) being controlled by the controller."

I believe it is implicit that if a psa.p4 has bit<7> multicast_group_id, it will allow all values from 0 to 127.

I know it is common for things in computers to have sizes that are powers of 2, but it sometimes happens that people implement SRAMs with a number of entries that aren't powers of 2 (or a collection of them whose total number of entries is not a power of 2, more likely, but have a common address space spanning all of them).

I would hate for a controller to have an easily avoided bug if this turns out to be the case, because it made such power-of-2 size assumption.

samar-abdi

Thanks Andy!! Just a few minor comments.

samar-abdi · 2018-02-02T06:26:09Z

p4-16/psa/PSA.mdk

+
+A PSA data plane implementation that supports the P4 Runtime
+API[^P4RuntimeAPI] is expected to include software called a "P4
+Runtime API agent" that encapsulates details specific to the PSA


suggest: ...include software called a "P4 Runtime Agent" that enables runtime programming of the PSA device from a controller. A controller may control multiple devices with different PSA implementations.

I fall into the passive voice too easily by habit. For that and other reasons, taking your suggested wording here.

samar-abdi · 2018-02-02T06:28:05Z

p4-16/psa/PSA.mdk

+
+[^CacheCost]: While 10 Mbits sounds tiny to one accustomed to computers with hundreds of gigabytes of DRAM, the highest speed PSA implementations are ASICs that must keep tables in on-chip memories, similar to caches in general purpose CPUs.  The Intel i9-7980XE released in 2017 has 198 Mbits of on-chip L3 cache shared by its CPU cores.  Among Intel processors in Intel's 7th generation Core released in 2017 with at least 100 Mbits of L3 cache, they all cost close to $9 per Mbit of L3 cache.  https://en.wikipedia.org/wiki/List_of_Intel_microprocessors
+
+The P4 Runtime API is planning to use 64-bit quantities to hold values


s/is planning to use/uses

samar-abdi · 2018-02-02T06:30:12Z

p4-16/psa/PSA.mdk

+[^CacheCost]: While 10 Mbits sounds tiny to one accustomed to computers with hundreds of gigabytes of DRAM, the highest speed PSA implementations are ASICs that must keep tables in on-chip memories, similar to caches in general purpose CPUs.  The Intel i9-7980XE released in 2017 has 198 Mbits of on-chip L3 cache shared by its CPU cores.  Among Intel processors in Intel's 7th generation Core released in 2017 with at least 100 Mbits of L3 cache, they all cost close to $9 per Mbit of L3 cache.  https://en.wikipedia.org/wiki/List_of_Intel_microprocessors
+
+The P4 Runtime API is planning to use 64-bit quantities to hold values
+of the types listed in section [#sec-psa-type-definitions].  For


suggest: ...listed in section [#sec-psa-type-definitions] to simplify the manipulation of the metadata quantities in the agent software.

samar-abdi · 2018-02-02T06:32:08Z

p4-16/psa/PSA.mdk

+  [#sec-packet-digest]).
+ Fields in the data contents of a `Register` array (section
+  [#sec-registers]).
+ Match fields of an entry in a `ValueSet` extern instance (section


Is this still applicable given that we were expecting to remove value_set from PSA 1.0?

good catch. Removing that from this PR.

samar-abdi · 2018-02-02T06:36:39Z

p4-16/psa/PSA.mdk

+P4 program generating packets with such headers should fill in any
+most significant "padding" bits with 0.  In a P4 program, this can be
+done with a normal assignment statement, where the value on the right
+hand side is cast to the wider `InHeader` type.


suggest: Similarly, for an assignment of a PortIdInHeader_t field to a PortId_t field, there will be a truncation of the excess most significant bits as part of the cast.

I added a sentence like this. Thanks.

samar-abdi · 2018-02-02T06:56:48Z

p4-16/psa/psa.p4

+typedef bit<8>  ClassOfServiceInHeader_t;
+typedef bit<16> PacketLengthInHeader_t;
+typedef bit<16> EgressInstanceInHeader_t;
+typedef bit<64> TimestampInHeader_t;


Yes for documenting translation of ClassOfService_t field values. The P4Runtime annotation will be cos_translation("controller_bitwidth:32").

As for your second point, I suggest : "The values of multicast_group, clone_session and egress_instance are completely in the purview of the controller, and are limited only by the bitwidth of the respective types on the PSA device(s) being controlled by the controller."

I believe it is implicit that if a psa.p4 has bit<7> multicast_group_id, it will allow all values from 0 to 127.

jafingerhut · 2018-02-02T08:16:32Z

@samar-abdi
You suggested this annotation for values of type ClassOfService_t: cos_translation("controller_bitwidth:32")
and this annotation is currently in the PSA spec for values of type PortId_t:
@port_translation("controller_port_bitwidth : 32")

Two questions for you:

Is it intentional that one has "controller_bitwidth" but the other "controller_port_bitwdith"? Would it be reasonable if they both used "controller_bitwidth"? Either that, or perhaps cos_translation should use "controller_cos_bitwidth"?

Why 32 bits for the bitwidths in your examples? This seems a bit confusing, given that we state that the controller and agent will use 64-bit types for these in their code, and we currently propose these widths for the InHeader variant of the types in this PR:

typedef bit<16> PortIdInHeader_t;
typedef bit<8>  ClassOfServiceInHeader_t;

Should the bitwidths be 64, to match the controller/agent software? Or perhaps they should match the 16, 8 numbers in the InHeader_t type definitions?

jafingerhut · 2018-02-02T08:19:47Z

@cc10512 @samar-abdi In a few minutes I will push a commit to this PR saying that only PortId_t and ClassOfService_t (and their InHeader_t variants) require numerical translation between controller and data plane.

For all other types, I am adding a requirement that the data plane support consecutive values in the range [0, max], where max is PSA-implementation-specific, can be different for each of the types, and need not be a power of 2. Take a look if any of that sounds wrong.

Also for the ClassOfService_t type. These annotations were suggested by Samar Abdi, to minimize the amount of information required in the annotation.

jafingerhut · 2018-02-07T21:53:31Z

@samar-abdi I have updated the one example P4 program that had an annotation on a PortIdInHeader_t field in a header to use the new simpler annotation you suggested in private email, i.e. @p4runtime_translation("port"). I also added the @p4runtime_translation("cos") annotation you suggested for values of type ClassOfService_t (and its 'InHeader' variant).

I like removing knobs/details that I don't know how to explain what they do :-)

jafingerhut · 2018-02-07T21:56:27Z

@cc10512 @samar-abdi I am ready for this to be merged in. The biggest remaining question for whether any of this changes for PSA v1.0 in my mind is whether people are comfortable with the bit widths proposed for the types that are intended to be big enough to handle all PSA implementations.

There is an open issue for that here: #571 and any changes suggested there are a small edit to the existing document.

Calin, feel free to merge it if you want, or let me know and I will do it (or create comments on other updates we want before merging).

jafingerhut · 2018-02-10T01:12:32Z

Going ahead and merging this in. We haven't labeled anything version 1.0 yet, so further edits are still possible to this before then.

cc10512 · 2018-02-10T01:15:16Z

@jafingerhut since we haven't heard any complaints on the sizes in a bit more than a week, I think we're good to go with this. I an pretty sure we'll revisit this once we have more experience with applications using the feature.

jafingerhut mentioned this pull request Jan 25, 2018

[PSA] Default Port Id type #370

Closed

jafingerhut requested review from cc10512 and vgurevich January 25, 2018 21:59

jafingerhut requested a review from ccascone January 25, 2018 22:10

cc10512 approved these changes Jan 31, 2018

View reviewed changes

samar-abdi approved these changes Feb 2, 2018

View reviewed changes

jafingerhut added 4 commits February 2, 2018 00:22

PSA Proposed description of PortId_t translation in PSA

8c56f18

PSA Small corrections to PortId_t translation description

202f711

PSA Address review comment on device-specific types

f82dc41

PSA Address review comments for controller<->data plane type differences

7e84eef

jafingerhut force-pushed the psa-notes-on-use-of-portid-fields branch from 36ef3a5 to 7e84eef Compare February 2, 2018 08:23

PSA Update annotations required for translation of PortId_t type

2e2a8a9

Also for the ClassOfService_t type. These annotations were suggested by Samar Abdi, to minimize the amount of information required in the annotation.

jafingerhut merged commit 5721769 into p4lang:master Feb 10, 2018

hesingh mentioned this pull request Nov 6, 2018

P4_16: Allow 'bit-vector structs' to be used everywhere that bit<W> type can #383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PSA Proposed description of PortId_t translation in PSA #562

PSA Proposed description of PortId_t translation in PSA #562

jafingerhut commented Jan 25, 2018

jafingerhut commented Jan 25, 2018

jafingerhut commented Jan 25, 2018

cc10512 left a comment

cc10512 Jan 31, 2018

jafingerhut Feb 1, 2018

cc10512 Jan 31, 2018

jafingerhut Feb 1, 2018 •

edited

Loading

antoninbas Feb 5, 2018

cc10512 Jan 31, 2018

jafingerhut Feb 1, 2018

samar-abdi Feb 1, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi left a comment

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut Feb 2, 2018

samar-abdi Feb 2, 2018

jafingerhut commented Feb 2, 2018

jafingerhut commented Feb 2, 2018

jafingerhut commented Feb 7, 2018 •

edited

Loading

jafingerhut commented Feb 7, 2018 •

edited

Loading

jafingerhut commented Feb 10, 2018

cc10512 commented Feb 10, 2018


		[^CacheCost]: While 10 Mbits sounds tiny to one accustomed to computers with hundreds of gigabytes of DRAM, the highest speed PSA implementations are ASICs that must keep tables in on-chip memories, similar to caches in general purpose CPUs. The Intel i9-7980XE released in 2017 has 198 Mbits of on-chip L3 cache shared by its CPU cores. Among Intel processors in Intel's 7th generation Core released in 2017 with at least 100 Mbits of L3 cache, they all cost close to $9 per Mbit of L3 cache. https://en.wikipedia.org/wiki/List_of_Intel_microprocessors

		The P4 Runtime API is planning to use 64-bit quantities to hold values

PSA Proposed description of PortId_t translation in PSA #562

PSA Proposed description of PortId_t translation in PSA #562

Conversation

jafingerhut commented Jan 25, 2018

jafingerhut commented Jan 25, 2018

jafingerhut commented Jan 25, 2018

cc10512 left a 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 Feb 1, 2018 • 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

samar-abdi left a 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

Choose a reason for hiding this comment

jafingerhut commented Feb 2, 2018

jafingerhut commented Feb 2, 2018

jafingerhut commented Feb 7, 2018 • edited Loading

jafingerhut commented Feb 7, 2018 • edited Loading

jafingerhut commented Feb 10, 2018

cc10512 commented Feb 10, 2018

jafingerhut Feb 1, 2018 •

edited

Loading

jafingerhut commented Feb 7, 2018 •

edited

Loading

jafingerhut commented Feb 7, 2018 •

edited

Loading