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

Clarify add-on-miss #64

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Clarify add-on-miss #64

wants to merge 3 commits into from

Conversation

hesingh
Copy link
Contributor

@hesingh hesingh commented Oct 26, 2022

Add information for sources for value of entry. Also add an example for add_entry.

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.
Copy link
Collaborator

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.

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Collaborator

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.
Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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.
Copy link
Collaborator

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) {
Copy link
Collaborator

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

Copy link
Contributor Author

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

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.

3 participants