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.

Sign up for GitHub

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

Add proposed extern function add_entry_if() #43

Merged
merged 2 commits into from
Oct 28, 2022
Merged

Add proposed extern function add_entry_if() #43

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
21bef0a
Add proposed extern function add_entry_if()
jfingerh May 17, 2022
68d0995
Address comments on restrictions to add_entry() calls
jafingerhut Oct 28, 2022
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
27 changes: 25 additions & 2 deletions examples/pna-example-tcp-connection-tracking.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ control MainControlImpl(
ExpireTimeProfileId_t new_expire_time_profile_id;

// Outputs from actions of ct_tcp_table
bool add_succeeded;
AddEntryErrorStatus_t add_status;

action tcp_syn_packet () {
do_add_on_miss = true;
Expand All @@ -159,11 +159,13 @@ control MainControlImpl(
new_expire_time_profile_id = EXPIRE_TIME_PROFILE_TCP_NEW;
}
action tcp_fin_or_rst_packet () {
do_add_on_miss = false;
update_aging_info = true;
update_expire_time = true;
new_expire_time_profile_id = EXPIRE_TIME_PROFILE_TCP_NOW;
}
action tcp_other_packets () {
do_add_on_miss = false;
update_aging_info = true;
update_expire_time = true;
new_expire_time_profile_id = EXPIRE_TIME_PROFILE_TCP_ESTABLISHED;
Expand Down Expand Up @@ -229,18 +231,39 @@ control MainControlImpl(
}

action ct_tcp_table_miss() {
#ifdef AVOID_IF_INSIDE_ACTION
// Note that this code is NOT completely equivalent to the
// code in the #else branch below, because it does not call
// drop_packet() if do_add_on_miss is false. One way to make
// the two versions of the program equivalent, not shown here,
// would be to assign a value to a metadata variable here such
// as 'ct_tcp_table_got_miss = true;', and then write
// additional code after ct_tcp_table.apply() equivalent in
// behavior to this:
//
// if (ct_tcp_table_got_miss && !do_add_on_miss) {
// drop_packet();
// }
add_status =
add_entry_if(do_add_entry = do_add_on_miss,
Copy link

Choose a reason for hiding this comment

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

Its not clear where do_add_on_miss is evaluated ?

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 do not understand why it is unclear. do_add_on_miss is the expression that is the first parameter of add_entry_if. In P4, for any call to an action, function, extern function, or extern method, expressions that are the parameters to the call are first evaluated, then once their values (or references, if it is an L-value for an out or inout parameter) are determined, then the call is made.

If that is unclear, please try asking your question in a different way, or feel free to email me at [email protected] and we can schedule time to talk live.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@shirshyad Perhaps you might not have seen the syntax in P4 that this example uses, where when you write an action like action foo(bit<4> a, bit<8> b) you are allowed to call it with syntax like this foo(a = 5, b = 7). It is much more common to write foo(5, 7), but both are allowed and have the same effect. I used the first style of syntax in this example to try to make it clearer what each parameter meant, but may have ended up confusing readers in the process. We can remove the parameter_name = occurrences from this example if you think it would be clearer.

Copy link

Choose a reason for hiding this comment

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

I am aware of foo(a=5, b=7) usage. At line 248, I see do_add_on_miss. I was searching for do_add_on_miss as a action local variable, action parameter etc. Hence the question.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do_add_on_miss in that example P4 program is a variable defined inside the control, before the action where it is used. P4 actions can refer to any variable that is in scope, including local variables declared earlier in the control that they are defined within (if the action is defined within a control), or any parameters of the control, as well as parameters of the action.

action_name = "ct_tcp_table_hit", // name of action
action_params = (ct_tcp_table_hit_params_t)
{},
expire_time_profile_id = new_expire_time_profile_id);
#else
if (do_add_on_miss) {
// This example does not need to use allocate_flow_id(),
// because no later part of the P4 program uses its return
// value for anything.
add_succeeded =
add_status =
add_entry(action_name = "ct_tcp_table_hit", // name of action
action_params = (ct_tcp_table_hit_params_t)
{},
expire_time_profile_id = new_expire_time_profile_id);
} else {
drop_packet();
}
#endif // AVOID_IF_INSIDE_ACTION
// a target might also support additional statements here, e.g.
// mirror the packet
// update a counter
Expand Down
82 changes: 69 additions & 13 deletions pna.p4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -665,19 +665,75 @@ extern void mirror_packet(MirrorSlotId_t mirror_slot_id,
// probably be a parameter that specifies the new entry's initial
// expire_time_profile_id.

extern bool add_entry<T>(string action_name,
in T action_params,
in ExpireTimeProfileId_t expire_time_profile_id);

// If we support the following variant of add_entry, with no
// expire_time_profile_id parameter, then we need to define its
// behavior in terms of what the added entry's value of
// expire_time_profile_id is. One option would be to define the
// behavior to be the same as the 3-parameter version of add_entry
// above, with expire_time_profile_id equal to 0.

//extern bool add_entry<T>(string action_name,
// in T action_params);
// The bit width of this type is allowed to be different for different
// target devices. It must be at least a 1-bit wide type.

typedef bit<1> AddEntryErrorStatus_t;

const AddEntryErrorStatus_t ADD_ENTRY_SUCCESS = 0;
const AddEntryErrorStatus_t ADD_ENTRY_NOT_DONE = 1;

// Targets may define target-specific non-0 constants of type
// AddEntryErrorStatus_t if they wish.

// add_entry() must be called within the body of a P4 action that is
// the default_action of a table with the table property `add_on_miss`
Copy link

Choose a reason for hiding this comment

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

should we not show in the example, the usage of the property 'add_on_miss = true' ?

Copy link
Contributor Author

@jfingerh jfingerh Oct 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is such an example, here: https://github.com/p4lang/pna/blob/main/examples/pna-example-tcp-connection-tracking.p4#L273

Is there some other example program you are referring to that is missing add_on_miss = true?

// equal to true.
//
// The key of the entry added will always be the same as the key that
// was just looked up in the table, and experienced a miss.
//
// `action_name` is the name of an action of this table. It must be
Copy link

Choose a reason for hiding this comment

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

If we mean the action associated with add_entry is an action that is NOT default action, then we can say "action_name" is the name of non default action of this table.

// possible for this action to be the action of an entry added to the
// table, i.e. it is an error if the action has the annotation
// `@defaultonly`.
Copy link

Choose a reason for hiding this comment

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

Should we not say "default_action" table property versus annotation "@deafult_only" ?

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 tried to be very precise about what I meant there, and I think what I wrote is what I meant, but see if the explanation below clarifies anything:

Some actions can be both hit actions and miss actions for a table, i.e. if they have neither a @tableonly nor a @defaultonly annotation them. According to the text, it is OK for such an action to be the first parameter of an add_entry call. That is true EVEN IF such an action is ALSO currently the default (miss) action of the table. Why should we disallow that? It might be unusual for an action to be a table's default action, and also installed as the action of one or more table entries, but the P4 language has allowed this since it was defined.

Copy link

Choose a reason for hiding this comment

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

Then may be description can also include both "@defaultonly" annotation or has table property "default_action".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if a table is defined like this?

table t1 {
    key = { /* key fields not important for this example, so omitted */ }
    actions = {
        a1;
        a2;
    }
    default_action = a2;
}

I would argue that action a2 is a perfectly acceptable argument to a call to add_entry, because it does not have a @defaultonly annotation.

The fact that it is the value of the default_action property does not stop the control plane from adding entries with action a2, so why should add_entry be disallowed from adding entries with action a2?

Copy link

Choose a reason for hiding this comment

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

From the description I understood as if default_action adds entry with a1 as action, then it should be accepted. However if default_action adds entry with "a2" as match-action for the new entry , then when "a2" happens to be default_action (@default_action annotation), then it should not be allowed. Let me re-read the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be clearer if the restriction were documented in the following way instead?

action_name is the name of an action that must meet the following conditions:

  • it must be an action in the actions property for the table to which it is adding the entry
  • it must be allowed for the action to be an action associated with a table entry, i.e. it must not have the @defaultonly attribute
  • it must be an action that does not contain a call to add_entry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shirshyad Do you have any concerns or questions remaining on this PR? Any suggested changes before you would be willing to approve it? If folks were happy with using the DPDK software switch for PNA (which fully supports if in P4 actions), or enhancing the BMv2 one to support if fully in P4 actions, I am happy never defining add_entry_if, by the way. The only reason this PR exists is because several people were concerned that some P4 targets don't support if` statements within actions.

Copy link

Choose a reason for hiding this comment

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

The earlier description is clear. I cannot close/resolve comments. Pls consider any comments with "like" from my end are resolved from my end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, do you mind copying and pasting what you consider to be the "earlier description"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind this question, as I believe you have answered it in this comment: #43 (comment) I will update the PR to reflect that updated description.

//
// Type T must be a struct type whose field names have the same name
// as the parameters of the action being added, in the same order, and
// have the same type as the corresponding action parameters.
//
// `action_params` will become the action parameters of the new entry
// to be added.
//
// `expire_time_profile_id` is the initial expire time profile id of
// the entry added.
//
// The return value will be ADD_ENTRY_SUCCESS if the entry was
// successfully added, otherwise it will be some other value not equal
// to ADD_ENTRY_SUCCESS. Targets are allowed to define only one
// failure return value, or several if they wish to provide more
// detail on the reason for the failure to add the entry.

extern AddEntryErrorStatus_t add_entry<T>(
string action_name,
in T action_params,
Copy link

Choose a reason for hiding this comment

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

Is this single action parameter ? Can we provide a list of action parameters here ? if not, then we should call action_param ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type T is intended to be a struct containing as many fields as the corresponding action being added has action parameters, and for those struct fields to match one-to-one with the names and types of the action parameters.

There are likely other ways this could be defined in P4, but I do not know another good way. It is not straightforward to define the signature of a function or method that takes any number of parameters today in P4, e.g. unlike Python, and probably C++.

Copy link

Choose a reason for hiding this comment

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

@jfingerh I get that. Hence the suggestion/wording to be "action_param" versus "action_params"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if the action has 3 parameters? You would not object if it was called action_param in that case, even though that one parameter to add_entry represents 3 parameters to the action?

in ExpireTimeProfileId_t expire_time_profile_id);

// The following call to add_entry_if():
//
// add_entry_if(expr, action_name, action_params, expire_time_profile_id);
//
// has exactly the same behavior as the following expression:
//
// (expr) ? add_entry(action_name, action_params, expire_time_profile_id)
// : ADD_ENTRY_NOT_DONE;
//
// and it has the same restrictions on where it can appear in a P4
// program as that equivalent code.
//
// Rationale: At the time PNA was being designed in 2022, there were
// P4 targets, including the BMv2 software switch in the repository
// https://github.com/p4lang/behavioral-model, that did not fully
// support `if` statements within P4 actions. add_entry_if() enables
// writing P4 code without `if` statements within P4 actions that
// would otherwise require an `if` statement to express the desired
// behavior, if only the extern function add_entry() were provided.

extern AddEntryErrorStatus_t add_entry_if<T>(
Copy link

Choose a reason for hiding this comment

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

Is this needed ? If BMV2 does not support if handling , then we should get BMV2 support it instead of adding a new extern.

Copy link
Contributor

Choose a reason for hiding this comment

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

Section 13.1 of the p4_16 language specification [4] mentions portability considerations for switch statements and conditionals in actions – “The body of an action consists of a sequence of statements and declarations. No switch statements are allowed within an action—the grammar permits them, but a semantic check should reject them. Some targets may impose additional restrictions on action bodies—e.g., only allowing straight-line code, with no conditional statements or expressions.”.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shirshyad I agree that it is best if P4 compilers for programmable NICs more often support if statements within actions, as compared to BMv2's support. I believe the DPDK target DOES support more general if statements within actions already today.

BMv2 could be generalized, of course, but unless someone volunteers (or more likely, is paid) to do that work, it won't happen. In this case, the BMv2 support is already there -- it requires enhancing the BMv2 back end for the open source p4c compiler to add support for more general if statements within P4 actions.

I would definitely prefer if we could simply assume that all P4 targets support if statements within actions, but the unfortunate current reality is they do not. The add_extern_if is ADMITTEDLY a workaround for this limitation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Following up, I have created this public issue to record some details about BMv2 and DPDK and their support (or not) for if conditions within P4 actions. See there if you are interested: #63

in bool do_add_entry,
string action_name,
in T action_params,
in ExpireTimeProfileId_t expire_time_profile_id);

extern FlowId_t allocate_flow_id();

Expand Down