search

p4lang / p4runtime

Specification documents for the P4Runtime control-plane API
Apache License 2.0
140 stars 86 forks source link

Making sure that the latest P4Runtime spec is compatible with P4_16 v1.2.3 #424

Closed jafingerhut closed 10 months ago

jafingerhut commented 1 year ago

This issue is similar to https://github.com/p4lang/p4runtime/issues/389, except addresses any changes made to the P4_16 language spec from version 1.2.2 to version 1.2.3, which can be found in bullet list form here: https://p4.org/p4-spec/docs/P4-16-v-1.2.3.html#sec-summary-of-changes-made-in-version-123-released-july-11-2022

I have copied and pasted those brief descriptions below.

Summary of changes made in version 1.2.3, released July 11, 2022. Extended minSizeInBits and minSizeInBytes to apply to more expressions (Section 9). Added support for maxSizeInBits and maxSizeInBytes (Section 9). Added support for empty lists of const entries in tables (Section 14.2.1.4). Added support for switch statements in actions (Section 14.1). Added support for direct invocation of controls and parsers (Section 15). Added parser value_set to list of control-plane visible names (Section 18.3). Added match_kind as a base type (Section 7.1.3). Removed structure initializers as they are subsumed by structure-valued expressions (Section 8.13). Specified operations on values typed as type variables (Section 8.22). Clarified semantics of compile-time known values (Section 18.1). Clarified semantics of directionless parameters (Section 6.7). Clarified semantics of infinite precision integers (Section 7.1.6.5). Clarified semantics of bit slices, shifts, and concatenation (Section 8.5). Clarified semantics of optional parameters (Section 6.7.2). Clarified restrictions on extern method and function invocation (Section F). Clarified semantics of implicit casts (Section 8.10.2).

I would recommend that a later comment on this issue take all of those bullet items, and make a separate comment on each one as to whether we believe it will have any effect on the P4Runtime API. I suspect that most of them will be "no known effect on P4Runtime API specification", but it would be good to explicitly say that in a later comment, rather than being silent about a language spec change, for possible future reference, and review by others.

jafingerhut commented 1 year ago

Here are some item-by-item comments on the list of updates from version 1.2.2 to 1.2.3:

[jafingerhut] minSizeInBits and minSizeInBytes are P4_16 data plane functions that have no control plane API, so there should be nothing that they affect in P4Runtime API.

[jafingerhut] maxSizeInBits and maxSizeInBytes are P4_16 data plane functions that have no control plane API, so there should be nothing that they affect in P4Runtime API.

[jafingerhut] There is a test case in p4c/testdata/p4_16_samples named issue2905-bmv2.p4 that has a table named t_exact with const entries, and an empty list of entries. The expected output files mentioned here are in p4c/testdata/p4_16_samples_outputs. issue2905-bmv2.p4.p4info.txt contains a P4Info file generated by open source p4c with a definition for t_exact that has is_const_table: true, as all tables with const entries do in their P4Info file. The file issue2905-bmv2.p4.entries.txt is empty, because there are 0 total const entries in all tables of that P4 program. That all looks correct to me, and is tested on every commit to p4c.

[jafingerhut] P4Runtime API currently defines action names and the directionless action parameters, but as far as I know it says nothing about the bodies of actions, and this change in the spec simply allows more kinds of P4_16 statements to be included in the definition of action bodies, so I cannot imagine any affect on the P4Runtime API spec.

[jafingerhut] Direct invocation of controls and parsers is simply another way to invoke controls and parsers in a P4_16 source program. I am not aware of any affect on P4Runtime API for any invocations of controls and parsers in the P4_16 source code, regardless of whether it is a direct invocation, or any other kind of invocation.

[jafingerhut] This appears to be correcting an ommission in the P4_16 language spec. There are already existing test cases in the p4c test suite that test generation of P4Info files for P4 programs containing parser value sets, e.g. issue1955.p4 and many others, and their expected P4Info output files contain definitions of those value_sets, so this was not an omission in the P4Info generation in p4c, but only an omission in the spec.

[jafingerhut] This was a clarification in the P4_16 type system. No known affect on the P4Runtime API.

[jafingerhut] A clarification in the language spec, removing some redundant definitions. No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

[jafingerhut] No known affect on the P4Runtime API.

Summary: If all of the above evaluations are correct, then none of the changes from version 1.2.2 to 1.2.3 of the language spec have any impact on the P4Runtime API.

chrispsommers commented 1 year ago

Thanks @jafingerhut for the timely and thorough review! We'll let this sit for a while to give others a chance to weigh in.

smolkaj commented 1 year ago

Thanks, @jafingerhut!

chrispsommers commented 1 year ago

Seems like there have been no objections to Andy's thorough analysis, so I think we should do a PR to the spec declaring it compliant with P4_16 v1.2.3, approve it and close this forthwith.

jafingerhut commented 1 year ago

@chrispsommers The main issue with that idea is that depending upon the results of these issues, P4Runtime API spec might not yet be compliant with P4_16 v1.2.2:

The good news is that once those are resolved, then we can jump all the way to being compliant with v1.2.2 to v1.2.3.

chrispsommers commented 1 year ago

Andy, thanks for reminding me!

jafingerhut commented 10 months ago

Closing this issue, as I believe there are no changes required in the P4Runtime API specification due to any of the changes made from version 1.2.2 to version 1.2.3 of the P4_16 language spec, as detailed above.