Add ExactMap and TernaryMap extern definitions to pna.p4 #52
Conversation
|
See #45 for some notes on an earlier discussion of these externs. |
|
I think this should be a separate library file, which can be included in pna.p4 if it is deemed to be standard. |
|
This was discussed in the LDWG in May 2021: p4lang/p4c#2739 |
|
"I think this should be a separate library file, which can be included in pna.p4 if it is deemed to be standard." Making it part of the PNA standard is exactly the reason this is being proposed and discussed. It might not meet with wide enough approval to become part of pna.p4, but that is my hope. Of course we could make N optional parts of PNA. If that is met with reasonably wide agreement, we might do that. In practice, the larger N is, and the more often P4 developers use the optional features that are not implemented by all target devices, the less portable their programs will be. That doesn't automatically make every optional thing "bad", just less desirable than something all vendors agree to implement. |
|
@apinski-cavium Is it quick for you to point out the way in which this code violates the current P4 language spec? @mbudiu-vmw If you or Andrew have any thoughts on changes to this proposal that would enable an argument like As you can tell from this proposal, I am OK if a proposal does not enable the p4c front/mid-end code to type check the value of const_entries, and any such type checking is relegated to the back end code of the target. Once that checking is done for one target in open source, other implementations could hopefully reuse it. |
|
So it looks like I was wrong about _ types. See 7.2.13 Don't care types but I am still not 100% understand the semantics here. |
|
@apinski-cavium Do you mean that Section 7.2.13, and/or Section 17.1, were different in earlier versions of the spec that had a different explanation/example of _ types? I did a quick spot-check of the corresponding sections back to version 1.0.0 of the spec (available in the "Archives" section of this page https://p4.org/specs/) but did not notice any differences there. |
I looked into the git history (rather than published version of the spec) on this one to find where it changed. It changed when p4lang/p4-spec@84db458 commit was pulled in. The example was never right to begin with anyways (because control type definition vs control definition). |
|
The right solution is to extend the p4 type system to support generic lists. The entries is a list literal. The braces notation is somewhat overloaded now: it is used for tuples and structs too. |
|
@apinski-cavium Do you think that with a clarification in the language spec, that this proposed extern definition would be more clearly according to that clarified definition? |
|
Why don't we actually add List or Vector types and literals to the language? May not be that difficult. We can talk about it at the next design working group. I will try to prototype something to make sure it works. |
|
@mbudiu-vmw @jnfoster How likely do you think extending the P4 type system to support generic lists would be in, say 3 to 6 months? I know that of course the answer depends upon how much effort is put into it, but wondering if you have some kind of guesstimate. If a smaller change to the language spec would make this PR legal using |
|
This could be done in a couple of days of work if there are no unexpected surprises. |
|
Agree. Just adding polymorphic lists would be an easy task. |
|
Here is a possible example: p4lang/p4c#3520. |
|
p4c actually accepts the vectors of tuples, but there's a bug in the tuple elimination pass in the midend which replaces tuples with structs, that one causes the errors reported. |
|
I have amended the PR to accept the vectors of tuples correctly. |
|
Commit 2 on this PR updates the proposed new externs so that they use the new As of 2022-Nov-18, this PR has been merged into open source p4c p4lang/p4c#3520 and I have verified that with those changes, this code compiles without any errors. |
| TernaryMap<tmap2_key, tmap2_val>( | ||
| size = 1024, | ||
| largest_priority_wins = true, | ||
| initial_entries = |
There was a problem hiding this comment.
This initial_entries parameter is intended to correspond with the new ability to define initial entries (without const qualifier before the entries table property name) proposed in this PR for the language spec: p4lang/p4-spec#1180
Here I do not see an easy way to make the per-entry const property or the per-entry priority optional, so they are required for each entry.
| } | ||
|
|
||
| /** | ||
| * The type K must be a struct type. Each of its members becomes a |
There was a problem hiding this comment.
K as struct type: It is possible for user to pass any type for this template parameter. There is no restrictions on number of fields, nesting the struct type. Hope, it is up to the target to restrict on these semantics?
There was a problem hiding this comment.
Yes, targets can limit the total size and/or type of fields within the struct type K, e.g. if the user wants a 10,000 bit key, that is too much for most targets.
| */ | ||
| ExactMap<emap1_key, bit<16>>( | ||
| size = 1024, | ||
| const_entries = (list<exactmap_const_entry_t<emap1_key, bit<16>>>) { |
There was a problem hiding this comment.
Isn't this initialization expected to work without cast ((list<exactmap_const_entry_t<emap1_key, bit<16>>>))?
There was a problem hiding this comment.
The current p4c compiler implementation of this feature might NOT work without the (list<...>) there. Maybe it will be enhanced later to work without that there, but it isn't there yet.
There was a problem hiding this comment.
These are so similar to existing tables (the comments even show the equivalence), I think it would be nicer if we could somehow implement this using a table property. A couple ideas:
table emap2 {
key = {
val1: exact;
val2: exact;
}
// notice lack of "actions =" line here
// lookup would be a special action recognised by the compiler
default_action = lookup(emap2_val);
// alternatively, we could introduce this new table property:
direct_value = emap2_val;
}
(...)
bit<4> my_val1 = ...;
bit<12> my_val2 = ...;
emap2_val my_resp = emap2.lookup(my_val1, my_val2);
Personally I have a slight preference for direct_value, as this is an action-less table (and so default_action doesn't really make sense).
The underlying BCAM/TCAM primitives aren't something which is specific to a NIC as far as I know.
I think this should be a separate library file, which can be included in pna.p4 if it is deemed to be standard.
Of course we could make N optional parts of PNA. If that is met with reasonably wide agreement, we might do that. In practice, the larger N is, and the more often P4 developers use the optional features that are not implemented by all target devices, the less portable their programs will be. That doesn't automatically make every optional thing "bad", just less desirable than something all vendors agree to implement.
The flip side is that once something is in pna.p4 it becomes very hard to change our minds down the road. However going the other way is easier, i.e. moving an optional extern into the main pna.p4 file.
|
|
||
| // As far as the control plane API is concerned, there is | ||
| // exactly one table. | ||
| L2_fwd.apply(); |
There was a problem hiding this comment.
Would be good if this example went on to actually use the maps (i.e. call the lookup() function).
There was a problem hiding this comment.
Agreed. I wanted to get a variety of instantiations into the program first, with heavy comments to explain what is going on. I do not have any functionally useful example that uses these 4 instantiations in the same program, but I can at least use one of them in some made-up sample code.
I am perfectly OK if we end up just creating more general P4 tables for this purpose, but note one of the original motivations for this in #45, repeated here: (a) their lookup method can be called from inside the body of a P4 action. The P4_16 language spec explicitly disallows table_name.apply() calls within P4 action calls today, and always has since version 1.0. I believe it was Roop who asked recently about this same issue, and I pointed out to him this limitation in the current P4 language spec, and our use case for wanting to call these externs within actions, and I also pointed out the following issue on the public p4-spec Github repo where the P4 language spec is discussed: p4lang/p4-spec#947 Roop said he is motivated to raise this issue again, hopefully in the next P4 language design work group meeting on 2022-Dec-05, and I urge you to also attend that meeting to make your case, or support Roop however you can, if you want such a change made in the spec. |
| } | ||
|
|
||
| struct exactmap_initial_entry_t<K,V> { | ||
| bool const_entry; |
There was a problem hiding this comment.
Adding this bool "const" member to the struct complicates the already challenging semantics of const. It would be worth documenting a bit more clearly what it means. Does it mean it can't be deleted by any runtime API? What about automatic add/del ops? Does it change the table props in any other way (like current const does on tables)?
There was a problem hiding this comment.
I would assume it would be the same as the table entries handling which is handled in the pull request p4lang/p4-spec#1180 .
There was a problem hiding this comment.
A short answer is that const entries in P4 tables means all entries are const, and none can be removed, modified, NOR added (this is in the language spec). The constructor with const_entries is for these externs is intended to be like that.
The pull request that Andrew Pinski linked to is a proposed new thing in the language spec defining entries for a P4 table without a const qualifier. See there for details, but the intent is that each of the entries can be independently marked as const or not, and only those marked const are prevented from being removed or modified by the control plane, but the control plane is free to add new entries to the table (up to the size limitation). The constructor for these externs with initial_entries parameter is intended to work like that, and the true/false value for const takes the place of the const keyword.
While it would be nice if the same syntax could be used for P4 tables as here, that would be a bigger change to the spec, e.g. perhaps to define the current syntax in P4 tables as a syntax for a new type of object, perhaps called a 'table entry' object, but I wasn't hoping to go that far here.
There was a problem hiding this comment.
If any or all of the text in my previous comment could be usefully added to this PR, let me know. I think some of it is said in comments in the pna.p4 include file, e.g. it says elsewere in this PR:
const_entries is a list of entries, similar to the 'const
* entries' table property for tables. This set of entries cannot
* be removed or modified by the control plane, and also, the
* control plane is not allowed to add any entries to an ExactMap
* instance created using this constructor.
And for initial_entries it is commented as:
The same as the ExactMap constructor with a parameter named
* const_entries, except that the control plane is allowed to
* add entries to an ExactMap instance constructed using this
* constructor (subject to capacity constraints, as usual), and
* the control plane can modify or remove any entries that has a
* const_entry field equal to false. Any entries with a
* const_entry field value equal to true cannot be modified or
* removed by the control plane.
| defined within a control that have the same name as each other. | ||
| Thus we can generate control plane API files, e.g. P4Info files, | ||
| assuming that extern instances of ExactMap and TernaryMap will | ||
| never have the same names as any P4 tables in the same control. |
There was a problem hiding this comment.
@jamescchoi See the paragraph comment I just added earlier today (2022-Nov-29) following up on a question in a meeting we had earlier today.
|
The 2022-Dec-05 P4 language design work group meeting resulted in the I am mindful of Thomas Calvert's comment here #52 (review) Unfortunately the topic of generalizing P4 tables was not discussed at the 2022-Dec-05 language design work group meeting, at least partly due to a full plate of topics. @thomascalvert-xlnx If you would like that issue discussed at the next (2nd Monday of January) language design work group meeting, we may as well suggest adding it to the agenda now. Would you like me to propose the discussion topic there, or would you like to? |
|
@jafingerhut thanks for the poke, I've just sent Mihai an email to suggest adding it to the agenda. |
|
@jafingerhut Add link to P4 LDWG issue related to this topic for reference. |
|
Here is link to issue proposing generalizing P4 tables in language design work group, which was discussed early in 2023, but not accepted: p4lang/p4-spec#1209 |
No description provided.