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 initial entries #432

Merged
merged 24 commits into from
Aug 11, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c80d691
Define P4Runtime API support for tables with initial entries
jafingerhut May 28, 2023
3db556a
Add TODO asking whether the format for the contents of entries files
jafingerhut May 28, 2023
a280b70
Fix a couple of things found by linter and compiling protobuf
jafingerhut May 28, 2023
6c09da3
Update autogenerated files
jafingerhut May 29, 2023
3b83f50
Document that TableEntry const field must be false in write requests
jafingerhut May 30, 2023
b8a8724
Merge branch 'master' into support-for-initial-entries-v1
jafingerhut Jul 8, 2023
1632245
Add an appendix describing the contents of entries files generated by…
jafingerhut Jul 8, 2023
005df11
Clarify some wording.
jafingerhut Jul 8, 2023
6625dbc
Fix Madoko lint check
jafingerhut Jul 8, 2023
15a5911
Replace TODO with cross reference to new appendix on entries files
jafingerhut Jul 9, 2023
3a10775
Replace TODO with an optimistic footnote.
jafingerhut Jul 14, 2023
0202c3b
Propose that TableEntry has new field const true for const entries
jafingerhut Jul 14, 2023
4264daf
Update auto-generated files
jafingerhut Jul 14, 2023
25aadc5
Define has_initial_entries to be true for tables with `const entries`
jafingerhut Jul 14, 2023
ddc2af1
Update auto-generated files
jafingerhut Jul 14, 2023
2c8b986
Address several review comments
jafingerhut Jul 15, 2023
e26f9c5
Address some more review comments.
jafingerhut Jul 15, 2023
3f6d536
Update auto-generated files again
jafingerhut Jul 15, 2023
4620bb9
Slight change in definition of has_initial_entries flag
jafingerhut Jul 15, 2023
34eb819
Update auto-generated files
jafingerhut Jul 15, 2023
5c82d35
Add "added in 1.4.0" notes to the two new fields
jafingerhut Jul 19, 2023
56e5a1d
Clarify the description of the content of an entries file
jafingerhut Jul 19, 2023
254ad30
Fix a typo, and add is_const field to list of TableEntry fields
jafingerhut Jul 21, 2023
3f0c390
Address review comment in new appendix
jafingerhut Jul 29, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 124 additions & 7 deletions docs/v1/P4Runtime-Spec.mdk
Original file line number Diff line number Diff line change
Expand Up @@ -3043,6 +3043,10 @@ limitation. It is recommended that, for the sake of portability, P4Runtime
clients do not try to insert additional entries once the size indicated in
P4Info has been reached.

The `const` field must be `false` in any `INSERT`, `MODIFY`, or
`DELETE` write request of a table entry. If it is true, the server
must reject the operation and return an `INVALID_ARGUMENT` error.

### Match Format { #sec-match-format}

The bytes fields in the `FieldMatch` message follow the format described in
Expand Down Expand Up @@ -3270,10 +3274,13 @@ indirect tables --- tables with an ActionProfile or ActionSelector
`implementation` property --- to a constant `NoAction` action entry, with the
hope that it would simplify the implementation of the P4Runtime service.

### Constant Tables
### Constant Tables { #sec-constant-tables }

Constant tables are defined as tables whose match entries are immutable. They
are identified by the `is_const_table` flag in P4Info.
Constant tables are defined as tables whose match entries are
immutable. They are identified by the table property `const entries`
in the P4~16~ source code, and the `is_const_table` flag in P4Info.
For tables declared with the `entries` property, without `const`
before `entries` see Section [#sec-tables-with-initial-entries].

The only write updates which are allowed for constant tables are `MODIFY`
operations on direct resources, and the default action (assuming the default
Expand All @@ -3293,10 +3300,71 @@ direct resources that are being queried. Idle timeouts are not supported for
static entries. If the table requires a priority value for entries, the server
must populate the `priority` field appropriately, starting at 1 for the lowest
priority entry and incrementing the value by 1 for each successive entry. Note
that P4~16~ does not support assigning explicit priorities to static
entries. When a priority value is required (⪚ for tables including `RANGE`,
`TERNARY` or `OPTIONAL` matches), it is inferred based on the order in which
entries appear in the table declaration.
that P4~16~ does not support assigning explicit priorities to entries
declared with `const entries`[^ConstEntriesPriorities]. When a
priority value is required (⪚ for tables including `RANGE`,
`TERNARY` or `OPTIONAL` matches), it is inferred based on the order in
which entries appear in the table declaration.

[^ConstEntriesPriorities]: This is not yet explicit in the P4~16~
language specification, but will become so if this or a similar
clarification is added to it:
<https://github.com/p4lang/p4-spec/pull/1259>

### Tables with initial entries { #sec-tables-with-initial-entries }

Tables with initial entries are those identified by the table property
`entries` in the P4~16~ source code (with no `const` qualifier before
`entries`), and the `has_initial_entries` flag in P4Info. For tables
declared with `const entries`, see Section [#sec-constant-tables].

For every P4 table, it will fall into one of these three categories:
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved

* Neither `entries` nor `const entries` are declared in the source
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
code, and thus `is_const_table` and `has_initial_entries` will both
be false. This will be called a "normal table" in this section
below.
* The table has `const entries` declared, and thus a separate
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
`entries` property is not permitted by the language. Such a table
will have `is_const_table` true and `has_initial_entries` false.
* The table has `entries` declared, and thus a separate `const
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
entries` property is not permitted by the language. Such a table
will have `is_const_table` false and `has_initial_entries` true.

Since a table may not have both `entries` and `const entries`
declared, no table will have `is_const_table` true and
`has_initial_entries` true.

A table with initial entries is allowed to have a mix of some entries
marked `const`, and other entries not marked `const`.

Entries not marked `const` may be modified or deleted, just as a
client may do for any entry in a normal table.

Entries marked `const` behave like entries in a constant table,
&ie; only `MODIFY` operations on direct resources are allowed.

Unlike a table with `is_const_table = true`, a client may insert
entries into a table with `has_initial_entries = true`, subject to
capacity constraints on the number of entries supported by the target
for the table.

The contents of tables with initial entries can be queried by the
client through a `ReadRequest`. When reading entries from a table
with initial entris, the following fields must be set by the server:
Copy link
Member

Choose a reason for hiding this comment

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

Is this any different than for other tables?
If not, perhaps better not to say anything to keep things consistent?

Copy link
Member

Choose a reason for hiding this comment

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

More generally, I would suggest only explaining what is different, so we don't have two sources of truth that may go out of sync.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. My next commit should have changes that avoid this redundancy.

`table_id`, `match`, `action`, `is_default_action`, and `priority` (if
required). This is in addition to any direct resources that are being
queried. Idle timeouts are not supported for such tables.

If the table requires a priority value for entries, the priorities of
the initial entries are determined according to the P4~16~ language
specification. After the P4 program is initially loaded, the entries
not marked `const` can be modified at run time just as table entries
Copy link
Member

Choose a reason for hiding this comment

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

We already have a sentence explaining what we can and cannot do for const entries above -- would it make sense to move one of the two sentences so we can see the difference between const vs non-const in a single place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe commit 16 removes this redundancy.

in a normal table can.

The contents of all table entries within `entries` table properties in
Copy link
Member

Choose a reason for hiding this comment

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

within a / within the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am changing "within entries" to "within the entries'"

a P4 program can be written to a separate output file by the open
source `p4c` compiler. See Section [#sec-entries-files] for details.

### Wildcard Reads { #sec-table-wildcard-reads}

Expand Down Expand Up @@ -6598,4 +6666,53 @@ clients are aware that the server is using a larger maximum receive message
size. The gRPC server running the P4Runtime service must not set the maximum
receive message size to a value smaller than the default (4MB).

## P4Runtime Entries files { #sec-entries-files }

The open source P4 compiler `p4c` [@p4c] implements an option to
generate an "entries file", &ie; a file that contains all table
entries declared via the `entries` table property within the program.

An example P4~16~ program that can be used to demonstrate this
capability is `table-entries-ternary-bmv2.p4` [@p4cTestProgramForConstEntries]:

git clone https://github.com/p4lang/p4c
cd p4c/testdata/p4_16_samples
mkdir tmp
p4test --arch v1model \
--p4runtime-files tmp/p4info.txt \
--p4runtime-entries-files tmp/entries.txt \
table-entries-ternary-bmv2.p4

You can replace the `.txt` suffix of the file name `tmp/entries.txt`
in the example command above with `.json` or `.bin`. The `.bin`
format is a binary P4Runtime API protobuf message format. The `.txt`
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
format is the text encoding of the same Protobuf messages.

Target devices are *not* required to use this file. For example, if a
target has a P4 compiler back end that encodes all of the necessary
details from the P4 source program, including the `entries` of tables,
in a target-specific binary format, then that target might hae no
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
reason to generate these entries files.

Some target devices might choose to generates entries files, and also
jafingerhut marked this conversation as resolved.
Show resolved Hide resolved
to require doing so in order to have a correct implementation. For
example, a target runtime implementation might take a target-specific
binary format for the compiled P4 program that does *not* contain any
data describing the `entries` of tables, plus the entries file
generated by p4c, and use the entries file to load the initial entries
of tables into the device.

The format of the entries file is a sequence of 0 or more `Update`
messages. All of them have `type` equal to `INSERT`, and `entity` is
Copy link
Member

Choose a reason for hiding this comment

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

I suspect just a WriteRequest?
The protobuf parsers don't really support sequences of messages out of the box AFAIK, so if it was really just a concatenation of parsable protobuf blobs that would make things pretty hard to consume.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is an example entries file written by p4c in text format: https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples_outputs/table-entries-ternary-bmv2.p4.entries.txt

It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there, only a sequence of 0 or more Update messages. If you tell me that is a valid WriteRequest message, I am happy to document it that way in the P4Runtime spec.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, the link you provided does show a valid WriteRequest.

It doesn't look like a WriteRequest message to me, since there are no fields of a WriteRequest message present there

Actually it does: the repeated updates field is a WriteRequest field [1], and the textproto you linked to contains a bunch of updates { ... }.

This is admittedly subtle, because the text proto format doesn't include an outer wrapper for the message that is being encoded. Instead, it just shows the fields of the message at the top-level.

That being said, the text format is ambiguous, this could be a dump of any proto message with a repeated Update field. And for the binary format, this distinction matters. So we shouldn't blindly claim this is a WriteRequest. Do you happen to know where in the p4c source code this output gets created? From there it will be straightforward to tell the correct message type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the p4c code that writes the entries file: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L973-L976

in case the line numbers go askew, the method name is P4RuntimeEntriesConverter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you would like to examing the binary and/or JSON format of the entries file generated by current open source p4c, in addition to the text format, they are all in the zip file attached to this comment.
tmp.zip

Copy link
Member

Choose a reason for hiding this comment

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

I didn't find the place where things get serialized, but I would consider the following a good-enough hint that the message is a p4.v1.WriteRequest: https://github.com/p4lang/p4c/blob/main/control-plane/p4RuntimeSerializer.cpp#L1327-L1328

I also opened p4lang/p4c#4070 so people don't have to guess in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there an easy way for you to parse the contents of the binary format entries file in the file tmp.zip attached in my previous comment, to determine whether it is a valid binary WriteRequest protobuf message? I suspect it probably is, but do not know any quick way to check.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed with a Google-internal CLI tool that entries.bin can be parsed as a p4.v1.WriteRequest.

a `TableEntry` message containing the data for one table entry. The
file contains entries for all tables in the P4 program with at least
one entry defined via an `entries` table property.

Note that for a table with `const entries`, or with `entries` having
one or more individual entries declared `const`, it would be an error
for a P4Runtime client to send a `WriteRequest` to a P4Runtime server
with the contents of the entries file, because the server should

Choose a reason for hiding this comment

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

It may be good to further clarify that error applies to the entries with const only, not the whole contents of the entries file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the latest commit on this PR as of writing this comment, I have replaced the last paragraph of the new appendix with the following, in an attempt to clarify:

Note that if a P4Runtime client attempted to send a `WriteRequest` to
a P4Runtime server with the contents of the entries file, the server
must return an error for each entry that has `is_const` true, as
described in Section [#sec-table-entry].

reject and return an error when attempting to insert entries that are
immutable at run time.

[BIB]
10 changes: 10 additions & 0 deletions docs/v1/references.bib
Original file line number Diff line number Diff line change
Expand Up @@ -210,3 +210,13 @@ @ONLINE { ArenaAllocation
title = "C++ Arena Allocation Guide",
url = "https://developers.google.com/protocol-buffers/docs/reference/arenas"
}

@ONLINE { p4c,
title = "P4_16 reference compiler",
url = "https://github.com/p4lang/p4c"
}

@ONLINE { p4cTestProgramForConstEntries,
title = "P4_16 reference compiler test program table-entries-ternary-bmv2.p4",
url = "https://github.com/p4lang/p4c/blob/main/testdata/p4_16_samples/table-entries-ternary-bmv2.p4"
}
23 changes: 21 additions & 2 deletions go/p4/config/v1/p4info.pb.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading