Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for adding GenericTable #419

Closed
wants to merge 13 commits into from
Closed

Conversation

saynb
Copy link
Contributor

@saynb saynb commented Apr 14, 2023

  • Phase 1:
    • Not extend any of the current RPC updates. Use INSERT, MODIFY, DELETE and Read RPC as is
    • Define Table categories/ properties
    • Define Entry and Field properties
    • Define Type system
    • Achieve Write/Read for simple PNA specific externs like ExactValueMatchLookupTable
    • Work on generic-table-dev
  • Phase 2
    • Entry Handles
    • Table attributes (symmetric vs asymmetric)
    • New APIs
      • reset
      • add_or_mod
      • sync
      • usage_get
      • mod_inc
    • Table custom operations extensions. A way to define new operations via json modeling. For example, sync
  • Phase 3
    • String representation of fields. Like IPv4 IPv6
    • Table notifications. Model notifications in a generic way

@saynb saynb marked this pull request as draft April 14, 2023 03:28
@qobilidop
Copy link
Member

I want to better understand this proposal with a concrete use case in mind, which is to provide a P4Runtime API for custom hash extern to set its hash key from control plane. Is this an intended use case of this proposal?

The challenge there was that, as @jafingerhut commented, "if you define a new extern, there is no off-the-shelf way in P4 source code or P4Runtime API to define its control plane API". I think this proposal could enable providing such an off-the-shelf P4Runtime API for externs, right?

For example, for the following extern (adapted from pna.p4):

extern ToeplitzHash<O> {
  Hash();
  void set_rss_key<K>(in K key);
  O get_hash<D>(in D data);
  O get_hash<T, D>(in T base, in D data, in T max);
}

I would expect to model an extern ToeplitzHash object as a GenericTable in P4Runtime, and use the following entry to set its RSS key:

generic_table_entry {
  table_id: ...  // Refers to the extern object
  // No match fields in this case
  table_data_union {
    union_id: ...  // Refers to the set_rss_key method
    params {
      param_id: ...  // Refers to the key param
      value: ...
    }
  }
}

I'm not very certain about this usage because there is no match fields in this case.

Another thing I'm not clear here is how to model the generic extern methods like void set_rss_key<K>(in K key) above? My tempted answer is that, if this method is to be used in the control plane only, maybe this method level generics shouldn't be allowed, because there is no way in the P4 code to tell what K is. Generics at the extern object level like the following one should be fine though:

extern ToeplitzHash<O, K> {
  Hash();
  void set_rss_key(in K key);
  O get_hash<D>(in D data);
  O get_hash<T, D>(in T base, in D data, in T max);
}

@smolkaj
Copy link
Member

smolkaj commented Apr 14, 2023

I'll let @saynb answer himself, but regarding:

Another thing I'm not clear here is how to model the generic extern methods like void set_rss_key(in K key) above?

Normally the methods you see in P4 code should be "dataplane methods" (i.e. methods you apply in your P4 code, not via P4Runtime). @jafingerhut could you please chime in to clarify?

@saynb
Copy link
Contributor Author

saynb commented Apr 15, 2023

@qobilidop If I understand the usage of this extern correctly, then you are right. Is there an example of a control plane API call for this Hash extern? I can see in the current spec that there is Algorithm that you can pass as the constructor over here.

Hash(PNA_HashAlgorithm_t algo);

For the set_rss_key() method, let's say the P4 program looks like

 ToeplitzHash<0> hash_object;
hash_object.set_rss_key(value);

And the compiler decides to generate an object specific to the P4 function invocation, similar to what we do for digest.pack(), then, the p4runtime message (and the corresponding p4info) could look like the below

generic_table_entry {
  table_id: ...  // Refers to the extern object
  // No match fields in this case
  table_data_union {
    union_id: ...  // Refers to the set_rss_key method
    params {
      {
        param_id: ...  // Refers to the key param
        value: ...
      },
      {
        param_id: ...  // Refers to the algorithm param
        value: ... // String enum depicting the algorithm
      }
    }
  }
}

I will add a refined usecase with both p4info and p4runtime examples. If you can think of an extern with more complicated usage like having multiple key fields and data fields, then that would be great.

@qobilidop
Copy link
Member

Thanks @saynb! I don't have a more complicated usage in mind right now. I look forward to your examples to learn more.

@jafingerhut
Copy link
Contributor

Yes, GenericTable is intended to define control plane APIs for externs, including things like the ToeplitzHash extern mentioned above.

Yes, all methods defined for an extern object in P4 are methods intended to be called in the data plane, from your P4 program, while processing packets. There is NO standard way in P4 to define a control plane API for extern objects INSIDE the P4 program today. If you like the idea of being able to modify the control plane API without having to go back and update your P4 code every time (assuming the data plane API is not affected), then you might think of this as an advantage. (I am not claiming that everyone would view that as an advantage.)

I do not recall the bit and byte level details of the ToeplitzHash control plane API that we might want here, but it is basically something like specifying N different 32-bit unsigned integers as a "secret key" input to the hash.

One possible simple way to represent this as a single GenericTable would be to have unsigned integer exact match keys in the range [0,N-1], each having a value that is a 32-bit unsigned integer. There would be a separate such table for each extern object instance of a ToeplitzHash in the P4 program, with a name for the GenericTable instance that is probably some simple function of the extern object instance name, e.g. if the extern object instance name is "X", then the GenericTable instance name might be "X.SecretKeyConfig", where "SecretKeyConfig" is chosen at design time of the ToeplitzHash control plane API, and unlikely to ever be changed later unless you decide to completely change the control plane API of the extern in a backwards-incompatible way.

@smolkaj
Copy link
Member

smolkaj commented Apr 18, 2023

There is NO standard way in P4 to define a control plane API for extern objects INSIDE the P4 program today.

That's an interesting point. I do wonder if there should be. Maybe something to discuss in the P4 API WG.

@jafingerhut
Copy link
Contributor

jafingerhut> There is NO standard way in P4 to define a control plane API for extern objects INSIDE the P4 program today.

smolkaj> That's an interesting point. I do wonder if there should be. Maybe something to discuss in the P4 API WG.

jafingerhut> I agree it could be an interesting topic. One viewpoint on this: If GenericTable support is added to the P4Runtime API, then perhaps one way to specify a control plane API in a P4 program, for an extern object type, is to create a syntax for specifying one or more GenericTable instances that will be the control plane API for any extern object instance created (of that type).

That would avoid the need to specify API details for any specific programming language (e.g. C++, Java, Python, Golang, etc.) and instead it would simply be "Whatever the control plane API is for configuring these GenericTable objects, that is the control plane API for this extern."

If there were fancy restrictions on allowed values, sequence of operations, etc., that might get complicated to create a language to specify all of those, but at some point (perhaps very quickly), such restrictions would likely be just part of the documentation of the control plane API in comments.

@chrispsommers
Copy link
Collaborator

...create a syntax for specifying one or more GenericTable instances that will be the control plane API for any extern object instance created (of that type).
@jafingerhut Would this be a language construct, an annotation, or something else?

@jafingerhut
Copy link
Contributor

@chrispsommers I'm tossing ideas out from the top of my head here, not doing a careful thoughtful design process. Just warning you that there could be holes in anything I've said on this topic in this comment, and comments above, regarding such a feature.

If one wanted to make this part of the official P4 language definition of a P4 extern object, without using annotations, then making it standard would mean proposing a syntax for it and going through the language design work group. Possible, certainly, but perhaps more time consuming than using annotations.

Doing it using annotations would not require going through the language design work group for changes (at least, I cannot think of any reason language spec changes would be needed). One would just need to come up with a use of existing P4 annotations to define such control plane API tables.

@chrispsommers
Copy link
Collaborator

Doing it using annotations would not require going through the language design work group for changes (at least, I cannot think of any reason language spec changes would be needed). One would just need to come up with a use of existing P4 annotations to define such control plane API tables.

Recall that P4-16 language spec lists reserved annotations. If we define annotations for this solution discussed above, we'd likely want to add them to the spec. If we vetted a solution in this WG and got it into practice it would probably be an easier sell.

saynb added 3 commits May 11, 2023 18:50
Adding another enum alongside P4IDs for generic tables as well
* Adding varbit and a writeup
proto/p4/GenericTable.md Outdated Show resolved Hide resolved
saynb added 2 commits May 26, 2023 08:24
** list
** bag
** set
** ordered_set

* Moving GenericTable spec update to one big section to keep it
organized
* Adding more text on the data types in the spec
* Adding Details on Operations in the Table categories
### Table properties

Table properties are at table level. A combination of these define a certain
table category
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps state clearly that each property can be true or false

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Misc comments from today's meeting.

@smolkaj smolkaj self-requested a review June 2, 2023 17:19
* Adding a table to show valid combinations of table properties
* Adding default entry rules
* Read-only : No Add/Del/Mod work
* Modify-only : Table entries can only be modifed
* Reset-only : Table can only be reset. Either entire table or individual entries.
* Indexed : The entries are ordered. Handles define the indexes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Indexed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mark as provisional ?

docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
@chrispsommers
Copy link
Collaborator

Did you want to close this and mention it's superseded by #435 ?

oneof data {
bytes bitstring = 1;
GenericVarbit varbitstring = 2;
float float = 3;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a 32-bit float. If intended, it would be best to call it float32. It might be better to use float64 in a serialization spec for flexibility.

Copy link
Collaborator

@chrispsommers chrispsommers Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Protobuf defines float as 32 bits and double as 64 bits, see https://protobuf.dev/programming-guides/encoding/#structure
We can't just make up a new primitive name like float32, right?

@saynb
Copy link
Contributor Author

saynb commented Jun 16, 2023

Have opened #435 to continue working on it

@saynb saynb closed this Jun 16, 2023
repeated field is used to provide a value for the corresponding element in the
p4info.

* `union`, which indicates which of the table's unions to execute in case of

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I have a concern about the use to the word union by itself. I understand this is a C union type; but union is also a set operation. When I first read this spec, I wasn't sure if the GenericTableEntry's data could support a set of actions -- more than one action (like OpenFlow).

GenericTableEntry's data is in the form of a variant record. Each GenericTableEntry can execute exactly one variant. I feel that the word variant is an improvement on union in this context.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants