adopted and modified P4-14 description on parser value set to P4-16 #602

hanw · 2018-03-26T17:06:36Z

P4-14 spec has a section about parser value set that is still applicable to P4-16 with some modifications. I added that section in this PR

mihaibudiu · 2018-03-27T00:21:48Z

p4-16/spec/P4-16-spec.mdk

+set name may be referenced in parse state transition conditions (the value list
+in a case entry).
+
+Parser Value Sets contain values only, no header types or state transition


I think that this paragraph is no longer necessary; the way you use value sets would allow combinations which make this statement untrue.

mihaibudiu · 2018-03-27T00:22:17Z

p4-16/spec/P4-16-spec.mdk

+Value sets are declared locally within a parser. They should be declared before
+being referenced in parser `keysetExpression`.
+
+Given a type `T` for the type parameter of the value set, the type of the value


I would move this in the description of the select expression.

mihaibudiu · 2018-03-27T00:22:41Z

p4-16/spec/P4-16-spec.mdk

+`keysetExpression` in the same `select` expression. If there is a mismatch, the
+compiler must raise an error.
+
+Parser Value Sets support a `size` annotation to provide hints to the compiler


You just changed this in another proposal, so it is best to describe here the argument.

mihaibudiu · 2018-03-27T00:23:25Z

p4-16/spec/P4-16-spec.mdk

+to reserve hardware resource to implement the value set. The semantics of the
+size annotation is TBD.
+
+The run time API for updating parser value sets must allow value and mask pairs


We can probably here a bit less specific: we can just say that the value set is populated by the control-plane by a method specified in the other spec - the control-plane API.

mihaibudiu

Can you also please include a complete code example in the spec?

vgurevich · 2018-04-16T13:03:35Z

p4-16/spec/P4-16-spec.mdk

+runtime, one could write:
+
+~ Begin P4Example
+value_set<bit<16>>(4) pvs;


This definition is still not enough to generate a good API, because there is no way to name the fields, the values for which are going to be passed when value_set entries are going to be populated. This is especially important in case value_set is used to match against a tuple. Neither it is clear whether the values passed should be single or value/mask pair, range or anything else.

In this example there is just one field.
We can allow a value_set take a struct type, and in that case the fields can be named like the struct fields.

@vgurevich : this spec does not specify anything about the control-plane API. The way values are specified should be part of that other document.

mihaibudiu

We may want to support struct types for the type of the value set.

mihaibudiu · 2018-04-16T15:44:58Z

p4-16/spec/P4-16-spec.mdk

+runtime, one could write:
+
+~ Begin P4Example
+value_set<bit<16>>(4) pvs;


In this example there is just one field.
We can allow a value_set take a struct type, and in that case the fields can be named like the struct fields.

mihaibudiu · 2018-04-16T15:46:18Z

p4-16/spec/P4-16-spec.mdk

+runtime, one could write:
+
+~ Begin P4Example
+value_set<bit<16>>(4) pvs;


@vgurevich : this spec does not specify anything about the control-plane API. The way values are specified should be part of that other document.

vgurevich · 2018-04-16T16:19:04Z

@mbudiu-vmw ,

We can allow a value_set take a struct type, and in that case the fields can be named like the struct fields.

This is not a bad approach as long as the struct is flat. For more complicated structs it might be a little too much.

vgurevich · 2018-04-16T16:23:38Z

@mbudiu-vmw

this spec does not specify anything about the control-plane API. The way values are specified should be part of that other document.

I believe that one of the goals of P4 is the ability to generate control plane APIs for all the objects, specified in the program. While it is true that the spec does not have to describe the APIs, we need to ensure that the object description in P4 contains all the information required for such API generation. If it does not, then the other spec cannot be written correctly anyways.

In this particular case, the match type is required to properly generate the APIs, but is missing (in my opinion). The same can be said about the field names unless we converge on the struct or some other approach.

mihaibudiu · 2018-04-16T16:29:02Z

I think that the match_type is really very target-specific; as a proof, v1model.p4 contains the definition of match_type range. Probably this should really be an annotation:

@implement(ternary)
value_set<S>(10) set;

vgurevich · 2018-04-16T16:39:18Z

@mbudiu-bfn ,

I think that the significance of match_type in value_sets is no different than the one in tables and thus we need to use a very similar mechanism. It should be a part of the language, IMHO.

jafingerhut · 2018-04-16T17:24:39Z

Regarding match_kind for value sets, do you think for multi-field value sets, e.g. that match on a struct, that it should be possible to specify a different match_kind for each field, like you can in a table?

If so, it seems we are inching our way towards a value set specified much like a P4 table, except with no list of actions.

hanw · 2018-04-16T17:27:58Z

Attempted to write down what Vlad is saying,

header data_h {
  bit<32> da;
  bit<32> db;
}
struct my_packet {
  data_h data;
}
struct my_metadata {
  data_h[2] data;
}
parser MyParser(packet_in b, out my_packet p, inout my_metadata m, inout standard_metadata_t s) {
    value_set pvs {
        key = { p.data.da : exact; }
    }
    state start {
        b.extract(p.data);
        transition select(p.data.da) {
            pvs: accept;
            0x810: foo;
        }
    }
    state foo {
        transition accept;
    }
}

hanw · 2018-04-16T17:50:02Z

Here is a link to the solution that the P4runtime working group came up to solve the naming problem for field list which is not part of P4-16.
https://github.com/p4lang/PI/blob/master/proto/docs/p4runtime_type_serialization.md

The associated proto file for p4types.
https://github.com/p4lang/PI/blob/master/proto/p4/p4types.proto

To summarize, the issue with naming the field in value_set is not unique to value_set, it is also an issue with digest. The solution proposed by the P4 runtime work group should work for both primitives. The current implementation for value_set follows the guideline provided by the p4runtime working group.

vgurevich · 2018-04-16T18:13:35Z

@hanw This might work, but will limit the ability to use the same value_set in multiple parser states. I would suggest using names that are not bound to anything, more like formal parameters for a function.

vgurevich · 2018-04-17T04:07:18Z

@hanw ,

Here is a link to the solution that the P4runtime working group came up to solve the naming problem for field list which is not part of P4-16. https://github.com/p4lang/PI/blob/master/proto/docs/p4runtime_type_serialization.md

I might be wrong but I do not see a solution for naming individual members of an unnamed field list there. If these are just numbers, this is not a good API solution for value_sets.

hanw · 2018-05-07T17:33:01Z

I updated p4c frontend to allow struct type for the value set p4lang/p4c#1271
to address the issue of the naming objects in the value set in runtime API.

We (Vlad, Calin and I) had an offline discussion on the PR. To summarize the discussion, the proposal for value_set must satisfy the following constraints:

it must have a ‘size’.
it must have a ‘name’.
it must support simple datatypes, such as BitType and TupleType.
it must be able to generate meaningful API.
it must support more complex datatype, such as StructType(to be able to name the fields)
it must support sharing between parser states.
it must support multiple match_kind: exact, ternary, range.

The current proposal of

value_set<T> name (size);

satisfies all 7 requirements, but needs an annotation on the value_set instance to specify the match_kind. An option was proposed to integrate the match_kind in the struct as:

struct vsk_t {
    bit<16> field : ternary;
}

But the discussion on this new syntax for struct should happen in a different thread as part of the modularity discussion.

We suggest the working group to accept this PR.

mihaibudiu · 2018-05-07T17:35:14Z

You should then use annotations on struct fields:

struct vsk_t {
    @match(ternary)
    bit<16> field;
}

hanw · 2018-05-09T00:49:31Z

@mbudiu-vmw can we merge this PR?

mihaibudiu · 2018-05-09T15:57:04Z

p4-16/spec/P4-16-spec.mdk

+a type `T` for the type parameter of the value set, the type of the value set
+is `set<T>`. The type of the value set must match to the type of all other
+`keysetExpression` in the same `select` expression. If there is a mismatch, the
+compiler must raise an error. A parser value set accepts bit type, tuple type


The type of the values in the set must be one of bit<>, tuple or struct

mihaibudiu · 2018-05-09T15:58:11Z

p4-16/spec/P4-16-spec.mdk

+functionality, P4 supports the notion of a Parser Value Set. This is a named
+set of values with a run time API to add and remove values from the set.
+
+Value sets are declared locally within a parser. They should be declared before


... and can be used as a label in a select expression.

mihaibudiu · 2018-05-09T15:58:47Z

p4-16/spec/P4-16-spec.mdk

+Value sets are declared locally within a parser. They should be declared before
+being referenced in parser `keysetExpression`.
+
+Parser Value Sets support a `size` argument to provide hints to the compiler to


Maybe you can either cross-reference the previous section or give another example here.

mihaibudiu · 2018-05-09T15:59:21Z

I would appreciate if @vgurevich and @jafingerhut would take a look as well.

…truct type

jafingerhut · 2018-05-10T18:56:44Z

p4-16/spec/P4-16-spec.mdk

+
+~ Begin P4Example
+struct vsk_t {
+    @match(ternary)


Would it be a problem for the compiler, and/or the language definition, if this annotation name were 'match_kind' instead of 'match', so it was the same as 'match_kind' used elsewhere in P4_16? This is a low priority issue, in my mind. 'match' is fine if there are any problems using 'match_kind' as an annotation name.

match_kind is a keyword used to declare new match kinds. We may be able to tweak the grammar to make it work, but it may be confusing.

Use match_kind triggers a frontend error. I will use match for now.

syntax error, unexpected MATCH_KIND @match_kind ^^^^^^^^^^ error: 1 errors encountered, aborting compilation```

jafingerhut · 2018-05-10T18:58:21Z

p4-16/spec/P4-16-spec.mdk

+}
+~ End P4Example
+
+The above example allows the runtime API to populate upto 4 different


recommend "up to" instead of "upto" for English prose.

jafingerhut · 2018-05-10T19:04:45Z

p4-16/spec/P4-16-spec.mdk

+~ Begin P4Example
+struct vsk_t {
+    @match(ternary)
+    bit<16> port;


What if there is no @match annotation on a field?

Two possibilites I can think of:
(a) It is an error
(b) It defaults to @match(exact)

(b) seems reasonable to me, but whatever the choice, it would be good to say in the spec.

Is this syntax expected to be allowed? value_set<@match(ternary) bit<16>>(4) pvs; Or should we simply say that if you want a single non-exact field, you must put that field into a struct by itself, with the desired @match annotation?

I modified the text to specify default is match(exact) and single non-exact field must be in a struct by itself.

jafingerhut · 2018-05-10T19:06:01Z

p4-16/spec/P4-16-spec.mdk

+compiler must raise an error. The type of the values in the set must be one of
+bit<>, tuple, and struct.
+
+For example, to allow control plane API to specify TCP reserved ports at


recommend adding "the" here: "to allow the control plane API"

jafingerhut · 2018-05-10T19:10:32Z

@mbudiu-vmw I have added my review comments.

jafingerhut · 2018-05-10T19:25:04Z

@hanw It would also be good to edit Appendix D "Restrictions on compile time and run time calls" to mention value_set's and their proposed restrictions, everywhere that a P4 table is mentioned already. value_set's might not have identical restrictions to tables, but they are a new stateful element that is very similar to them, and people might look to that section for ideas on where they need to support it, vs. need not.

mihaibudiu · 2018-05-16T22:51:24Z

@hanw : I think we are close to merging this; @jafingerhut makes some good points that maybe you can address.

hanw · 2018-05-17T20:11:14Z

I modified Appendix D to specify the restrictions on value-set, please take a look and verify the text is accurate.

jafingerhut · 2018-05-17T21:14:32Z

p4-16/spec/P4-16-spec.mdk

@@ -6680,6 +6752,7 @@ e.g. `bit<32> x = rand.get();`.
 | control     |  no       | yes     | no        | no      | no     |
 | extern      | yes       | yes     | yes       | yes     | no     |
 | table       |  no       | yes     | no        | no      | no     |
+| value-set   |  no       | no      | no        | no      | no     |


I think it might be reasonable to put a "yes" in the "parser state" column for the value-set row, if you also say above after the sentence:

"Calling a parser, control, or table means invoking its apply() method."

something like this:

"Calling a value-set means using it in a select expression."

hanw · 2018-05-17T21:18:15Z

make sense, I fixed that column.

jafingerhut · 2018-05-17T21:24:12Z

@hanw @mbudiu-vmw Newest changes all look good to me.

mihaibudiu requested changes Mar 27, 2018

View reviewed changes

mihaibudiu reviewed Mar 27, 2018

View reviewed changes

vgurevich reviewed Apr 16, 2018

View reviewed changes

mihaibudiu requested changes Apr 16, 2018

View reviewed changes

hanw mentioned this pull request May 4, 2018

Support struct as type parameter in value_set<T> p4lang/p4c#1271

Merged

hanw force-pushed the description-on-value-set branch from 71615d9 to 278afae Compare May 7, 2018 17:10

hanw mentioned this pull request May 7, 2018

Use a struct type as T in value_set<T> p4lang/p4c#1270

Closed

mihaibudiu requested changes May 9, 2018

View reviewed changes

hanw-bfn added 5 commits May 9, 2018 14:00

adopted and modified P4-14 description on parser value set to P4-16

2c5e649

address comments

f476044

added description to pvs for struct_type and changed example to use s…

eef2e0b

…truct type

add annotation on pvs struct

90953e9

address comments

df27eb7

hanw force-pushed the description-on-value-set branch from 6cd50a4 to df27eb7 Compare May 9, 2018 21:13

jafingerhut reviewed May 10, 2018

View reviewed changes

mihaibudiu approved these changes May 16, 2018

View reviewed changes

address andy's comment

e865c56

jafingerhut reviewed May 17, 2018

View reviewed changes

fix text regarding where value-set can be called

3cec817

hanw merged commit a0a9f4d into master May 17, 2018

hanw deleted the description-on-value-set branch May 17, 2018 22:22

This was referenced May 17, 2018

There is no specification for value_set in P4-16 #593

Closed

Parser Value Set in PSA #565

Closed

jnfoster mentioned this pull request May 31, 2018

P4-16 Restrictions on new proposed value_set language construct #586

Closed

adopted and modified P4-14 description on parser value set to P4-16 #602

adopted and modified P4-14 description on parser value set to P4-16 #602

Conversation

hanw commented Mar 26, 2018

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

mihaibudiu 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

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vgurevich commented Apr 16, 2018

vgurevich commented Apr 16, 2018

mihaibudiu commented Apr 16, 2018 • edited Loading

vgurevich commented Apr 16, 2018

jafingerhut commented Apr 16, 2018

hanw commented Apr 16, 2018 • edited Loading

hanw commented Apr 16, 2018 • edited Loading

vgurevich commented Apr 16, 2018

vgurevich commented Apr 17, 2018

hanw commented May 7, 2018 • edited Loading

mihaibudiu commented May 7, 2018

hanw commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mihaibudiu commented May 9, 2018

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 May 10, 2018

jafingerhut commented May 10, 2018

mihaibudiu commented May 16, 2018

hanw commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

hanw commented May 17, 2018

jafingerhut commented May 17, 2018

mihaibudiu commented Apr 16, 2018 •

edited

Loading

hanw commented Apr 16, 2018 •

edited

Loading

hanw commented Apr 16, 2018 •

edited

Loading

hanw commented May 7, 2018 •

edited

Loading

hanw commented May 17, 2018 •

edited

Loading