Changes for language spec update? Clarified behavior of table with no key property, or if its list of keys is empty

[jafingerhut]

This issue should be closed exactly when the bullet item “Clarified behavior of table with no key property, or if its list of keys is empty” in Section 1.1 "P4 Language Version Applicability" is addressed and removed.

This issue is related to the following change made from v1.2.3 to v1.2.4 of the P4_16 language specification:

• Clarified behavior of table with no key property, or if its list of keys is empty (Section 14.2.1.1).

In the following paragraph, the emphasized text was added in version 1.2.4 of the language spec:

If a table has no key property, or if the value of its key property is the empty tuple, i.e. key = {}, then it contains no look-up table, just a default action—i.e., the associated lookup table is always the empty map.

(see https://p4.org/p4-spec/docs/P4-16-v1.2.4.html#sec-summary-of-changes-made-in-version-124 for the item in context of the full list of changes, but there are separate Github issues for each that might impact the P4Runtime API specification).

I believe this is only a clarification of what existing implementations did, but good to discuss in API work group to make sure. At least BMv2 and/or p4lang/PI may have a bug in this area, according to p4lang/behavioral-model#1208

[jafingerhut]

2023-Sep-08: Decision made that no P4Runtime spec changes needed, so remove this item from bullet list in spec.

[jafingerhut]

This issue can be closed if/when PR #459 is merged.

EDIT 2023-Sep-12: Based on further discussion below, I have added this item back into the unresolved issues list, and it should NOT be closed if PR #459 is merged.

[antoninbas]

@jafingerhut I didn't see a note about this in the meeting minutes (I may have missed it). Is the consensus that such tables have a size of 0 and that the server should return RESOURCE_EXHAUSTED if a client tries to insert an entry?

[jafingerhut]

@antoninbas The discussion was brief on this topic, and I do not believe that anyone thought to raise these questions there about precisely which error code the server should return on an attempted insert to such a table.

I did some quick experiments with a table having either an explicit empty key = { } list, or no key table property at all, in the P4_16 source code, with size = 512, size = 0, and no size attribute at all.

The results were not quite what I was expecting:

• If I gave size = 512, the compiler gave a warning message saying warning: ingressImpl.mac_da: size 512 specified for table without keys, but it still generated output files with size 512 in the P4Info file for the table.
• If I gave size = 0, the compiler gave a warning message saying warning: ingressImpl.mac_da: size 0 specified for table without keys, but it then used a size of 1024 in the P4Info file. That seems like a bug to me.
• If I gave no size table property at all, there was no warning, but the P4Info file contained size 1024. This 1024 appears to be a default size used for every table that does not have a size table property in the source code that p4c generates.

When I tried inserting an entry into thesimple_switch_grpc process for such a table, I got back an error that had a Python exception saying UNKNOWN, 'Error when adding match entry to target', where if I am interpreting that output correctly, appears to be an UNKNOWN gRPC error status, not RESOURCE_EXHAUSTED.

It seems worth filing a PR on p4c to always consistently output a size of 0 for such tables.

When I tried manually editing the P4Info file to delete the size field of the Table message for the table, simple_switch_grpc did load that P4Info file in, but an attempted insert operation on the table still gave the same error message mentioned above, UNKNOWN, 'Error when adding match entry to target', not RESOURCE_EXHAUSTED.

[jafingerhut]

I also found that if you give size = 1 explicitly for a keyless table, there is no warning, and size becomes 1 in the P4Info file for the table.

This p4c PR introduced the warning and the "no warning if size 1 is given explicitly" logic: p4lang/p4c#1503

It was in response to this issue: p4lang/p4c#1478

In the comments on that PR, Antonin did explicitly mention that size 0 seemed like a better choice for the back-end to see for such tables. Mihai mentioned creating a separate issue for something related to this, but I do not know if that ever occurred.

It does seem worth letting as many p4c back-end writers know about such a front/mid-end change, if we change the size to 0 for such tables, since it has been forced to be non-0 since 2018.

[jafingerhut]

And this is the code in p4c that generates the size used for a table in the P4Info file, I believe: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeArchHandler.cpp#L91-L117

There are TODO comments from Antonin in several places in this file regarding the table size. I'd suggest a scrub of TODO messages in the p4c compiler throughout, but there are 342 of them :-)

[jafingerhut]

OK, it took a bit longer than I expected to come up with a survey of various kinds of P4 table definitions, what size that today's (2023-Sep-13) version of p4c source code generates for each of them, and a proposal for what I think would be an improvement for consideration by others, but here it is:

• https://github.com/jafingerhut/p4-guide/tree/master/table-size-tests

It seems worth getting consensus amongst interested folks before creating a PR for the p4c compiler to change this behavior, especially since the current behavior has been either completely the same, or changed very little, since 2018.

@antoninbas @smolkaj @chrispsommers Please take a look at the link above if you have some time. It is not exactly a quick read, I know, but it does seem worth changing the behavior of some of the cases here.