-
Notifications
You must be signed in to change notification settings - Fork 21
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
Clarify add-on-miss #64
base: main
Are you sure you want to change the base?
Conversation
PNA.mdk
Outdated
same key that was just looked up. | ||
same key that was just looked up. The new entry value, as in a (key,value) | ||
pair, is obtained from network headers of the packet that caused the | ||
table lookup to miss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be obtained from network headers of the packet, but it is actually obtained from the key that was looked up, as defined in the key definition of the table. It is allowed to include any fields, whether they be metadata, packet header fields, or any other expressions allowed as table key fields in a P4 table. The way you have written it here, it is overly restrictive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to say that "The new entry will have the same key that was just looked up." does not say?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't metadata refreshed for every packet? This why I skipped mentioning metadata. Maybe not. The table key is just a key in (key, value) store, and this key lookup failed, so no key, value store exists to copy its value data for new entry. I like "other expressions" text you proposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I wrote "the key that was looked up" in my original text, I meant the value that is called lookupKey
in this section of the language spec: https://p4.org/p4-spec/docs/P4-16-v-1.2.3.html#sec-mau-semantics
That value might match in the existing table entries, or it might miss. If it misses, that lookupKey
value is the new one to be added to the table by the call to add_entry
.
If any of that is clearer or more precise than what I wrote, I'm open to a PR that suggests words including the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if user-defined metadata is refreshed for every packet, your P4 program can make assignments to metadata, e.g. based upon table lookups, before reaching the table with add_on_miss=true, so it can be very useful to include in the key of such a table (e.g. a VRF id derived from a table lookup, not in the packet).
Also, some intrinsic metadata like the input port/vport of the packet might be useful in the key of an add_in_miss=true table.
PNA.mdk
Outdated
same key that was just looked up. The new entry value, as in a (key,value) | ||
pair, is obtained from sources such as packet headers that caused the | ||
table lookup to miss. Other sources are metadata and results from | ||
prior processing of the P4 program before `add_entry` is called. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree all of this new text is abolutely true, but it seems redundant to me with the definition in the P4 language spec of what the expressions in a table's key
expressions are allowed to be. Is it worth listing these possibilities out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A user may be used to configuring table entries only from the control-plane. I was confused. Such a user will scratch his head and ask where does add-on-miss get its value data from to be able to write a key-value to memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the place where the new entry gets its parameter values is from the parameters to add_entry
. Why not just say that? Why say "they might be packet header fields, they might be metadata calculated earlier, etc." The rules of P4 say how to evaluate those expressions, without needing to list them all again in the definition of add_entry.
@@ -940,10 +943,14 @@ sustain `add_entry` calls at a large fraction of their line rate, but | |||
it need not be at the same packet rate supported for processing | |||
packets that do not call `add_entry`. The new table entry will be | |||
matchable when the next packet is processed that applies this table. | |||
Use `add_on_miss` with caution because the data plane must have | |||
enough information to verify that adding an entry on miss is correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I have said this before, but why a special warning for add-on-miss? Why not for every assignment statement you write? Every action? Every if statement? For all of them, and everything else in a P4 program, the data plane must have enough information to verify that doing that thing is correct. add-on-miss isn't special in that regard.
|
||
~Begin P4Example | ||
action hit(bit<8> a0, bit<16> a1) {} | ||
action miss(bit<8> a0, bit<16> a1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This program won't compile, I'm nearly certain, because no action parameters are provided to the action miss
in the definition of the default_action
below for table foo
.
I would recommend changing the definition of miss
so that it has 0 parameters.
You could also consider adding a link to a more complete example program such as the one in the examples directory like https://github.com/p4lang/pna/blob/main/examples/pna-example-tcp-connection-tracking.p4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. I read the P4-16 working doc and see text in default_action
must supply arguments for the control-plane bound parameters because the default_action
is synthesized at compilation time
Add information for sources for value of entry. Also add an example for
add_entry
.