PSA - Add notes about empty groups for the ActionSelector extern #662

jafingerhut · 2018-08-09T00:08:43Z

No description provided.

jafingerhut · 2018-08-09T00:13:38Z

@konne-google I do not seem to be able to add you as a reviewer for this change, but if I recall correctly you were interested in this, so wanted to notify you of this proposed change in case you had comments.

Also adding @vgurevich

ghost · 2018-08-09T01:28:52Z

This looks good. Thanks for the update!

antoninbas

I left a few comments

antoninbas · 2018-08-15T00:23:36Z

p4-16/psa/PSA.mdk

@@ -2069,7 +2069,16 @@ specified as a parameter when instantiating the action selector.
 Action selector members may only specify action types defined
 in the `actions` attribute of the implemented table. All actions in a group must be
 of the same type. The action parameters for actions in the same group are allowed
-to differ, and the action of different groups in a selector may be different.
+to differ, and the action of different groups in a selector may be different.  More precisely, a PSA implementation is allowed to, but need not, support an action selector group where different members use different action types.


Line wrap?
Doesn't the precision contradict the statement above:

All actions in a group must be of the same type.

If the way it is written now is confusing, I would prefer to modify the sentence "All actions in a group must be of the same type." Except in rare circumstances where we actually want to prevent something, I think PSA should specify "implementations should support at least this", and should usually not say "an implementation cannot do X".

I think the first sentence should be changed. Right now IMO they strictly contradict each other.

Eliminated the contradiction, by changing the whole paragraph. Hopefully the meaning is still reasonably clear.

antoninbas · 2018-08-15T00:24:01Z

p4-16/psa/PSA.mdk

+PSA implementations are allowed to support adding entries in a table
+that point at groups which currently have no members, but they are
+also allowed to have non-portable implementation specific behavior if
+this occurs in the data plane.  This limitation of a PSA data plane


double whitespace

antoninbas · 2018-08-15T00:25:41Z

p4-16/psa/PSA.mdk

@@ -2422,7 +2431,11 @@ In particular, ActionProfile and ActionSelector single operations,
 such as adding a member to a group, removing a member from a group,
 adding an empty group, deleting an empty group, or modifying the
 action parameters of an action added earlier to a group, should all be
-atomic.
+atomic.  One possible exception is a PSA implementation that uses the


double whitespace again it seems (and there are other occurrences as well). This is funny because I have seen it in a PR from Tom Everman as well; does it come from the text editor?

I checked Wikipedia on this, out of curiosity. Quoting from the first paragraph of the Wikipedia page below: "Many people prefer double sentence spacing for informal use because that was how they were taught to type." <-- That is my reason.

https://en.wikipedia.org/wiki/Sentence_spacing

if you use emacs it has some very nice commands to operate on phrases.
phrases in emacs are delimited by a sentence terminator and two spaces.

more precisely, that's the default regular expression for phrases. you can customize it. but it makes expressions such as i.e. or e.g. that and with a period unambiguous.

@jafingerhut My bad I had no idea this was a thing. I should have Googled it first.

@antoninbas Not bad on your part at all. Until a month ago I did not even realize that a few people are vehemently anti-double-space-between-sentences, e.g. whoever made the image early in this article: https://www.cultofpedagogy.com/two-spaces-after-period/

Lol. Given that the using a single space after a period is called "French spacing" (https://en.wikipedia.org/wiki/History_of_sentence_spacing#French_and_English_spacing), I think my ignorance can be excused :)

antoninbas · 2018-08-15T00:31:07Z

p4-16/psa/PSA.mdk

+
+In either case, an implementation might also support putting the `if`
+statement inside of the action `set_output_port`, but this is not
+required by PSA.


did you want to mention "fast-failover" as one possible reason for the group to become empty without action from the switch or controller software? That's a valid way to implement the P4Runtime "watch" attribute if the HW supports it. In this case, the behavior may be undefined until the software is notified that the group is empty and can take action.

This whole description is for hiding the fact that a data plane implementation cannot support empty action selector groups, from the controller software.

It does it by explicitly having the switch/P4Runtime agent software do the hiding, via explicit actions some part of that software.

Are you thinking of a data plane that might do this without the involvement of target-specific switch software at all, e.g. in an ASIC? I've not seen networking ASICs that declare ports down on their own, without software getting involved.

All that said, I am fine with adding fast failover and/or watch attribute implementation as motivating reasons for wanting to use this workaround.

Hashing this out in live conversation might be better, but I am also confused by this sentence of your comment: "In this case, the behavior may be undefined until the software is notified that the group is empty and can take action." The reason for this new appendix is to avoid undefined behavior, even if the data plane does not have predictable behavior for empty action selector groups. The combined "data plane + P4Runtime agent" system can still have predictable portable behavior for times when the "action selector group is empty, as far as the controller knows".

antoninbas · 2018-08-15T00:32:54Z

p4-16/psa/PSA.mdk

+P4Runtime server implementation (hereafter called the agent).  The
+basic idea is that first the controller specifies the action (with
+action parameter values) to be performed when the group is empty.  The
+details of the message are left for the P4Runtime API specification to


we can bring this up at the WG tomorrow

... in response to discussion at the last P4 architecture work group meeting. Recommend, but do not require, that PSA data plane support empty groups by executing the table's default action if an empty group is encountered during packet processing time, with no restrictions on the names of the default action vs. the actions used in the non-empty groups. Describe some details of a faithful empty group implementation in the new appendix, and why the proposed workaround does not quite live up to that standard.

jafingerhut · 2018-08-17T22:27:53Z

@konne-google @cc10512 @vgurevich @wmohsin @hanw @antoninbas

I have updated the text a bit based on discussion we had at the P4 API and architecture work group meetings held on 2018-Aug-15.

The basic changes are:

recommend (but do not require) that a PSA data plane table with action selector implementation, when matching a table entry that points at an empty group, executes the table's default action. For PSA v1.1, a PSA data plane is allowed to have unpredictable packet processing behavior in that situation, but we may make the recommendation a requirement in some future PSA version.
the appendix is still there, but it explains why the workaround there doesn't really implement the recommendation. I don't see a way to extend it so that it does, so I left it as an "almost does it" in case it helps implementers think of ways to achieve the goal for their implementation.

ghost · 2018-08-17T22:37:49Z

This looks good to me now.

antoninbas · 2018-08-20T17:05:03Z

@jafingerhut my understanding was that we were considering adding a new table property to indicate which action to execute when the selector group is empty. Any reason why this has to be the same as the default action for a match miss?
On a related note, the current P4Runtime spec states that:

In this P4Runtime release, we have decided to restrict the default entry for 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.

See p4lang/PI#306 (comment)

jafingerhut · 2018-08-20T17:16:19Z

@antoninbas No, no reason I can think of that the 'encountered an empty action selector group' action must be the same as the default action. I probably thought of making the same while writing up the idea and thought it would simplify things if the default action for both were identical.

So if we make a new table property, e.g. one with a name like psa_empty_group_action = <action_name>;, it would only be correct to have such a property defined for a table with an action selector implementation. Like a default_action = <action_name>;, it would specify the action initially assigned to the table for this purpose (i.e. execution if an empty action selector group is found during a match), and the P4Runtime API will provide a command by which a controller can change that action after loading the P4 program, like one can for non-const default_actions for tables?

This is like `default_action`, except that it specifies an action for a table with ActionSelector implementation to execute if an empty group is encountered.

jafingerhut · 2018-08-20T17:52:33Z

@antoninbas @konne-google I have updated the PR to add a new table property psa_empty_group_action, specifying the action to be executed if an action selector table hits an empty group.

…ction"

antoninbas

LGTM

antoninbas · 2018-08-20T18:11:23Z

p4-16/psa/PSA.mdk

+P4Runtime server implementation (hereafter called the agent).  The
+basic idea is that first the controller specifies the action (with
+action parameter values) to be performed when the group is empty.  The
+details of the message are left for the P4Runtime API specification to


It is likely that now that we have the new table property, we will not add this to P4Runtime 1.0

Understood. Do you think that this appendix is clear enough that it is not required behavior, but one possible idea that might help PSA implementers understand and/or create their own implementation of the empty group action behavior?

I think it's clear but that specific paragraph is maybe a bit out-of-sync with the rest. Up until that paragraph we talk about the "empty group action" (which can be set through the new table property). But in this paragraph we have:

since PSA allows different groups to use different action
types, it seems best to allow the controller to specify a different
empty group action per group.

I understand that it is because you are trying to take into account the constraint that some HW may be limited to using the same action in any given group, while making sure that the solution is generic enough to allow different groups to use different actions.

In my case the most likely implementation will be to create a new group and update the match rules pointing to that group, but it is not ideal if there is more than one match rule for any given group.

I have modified this paragraph to remove any mention of the controller specifying the empty action group, for now mentioning only the agent obtaining it from the compiled representation of the P4 program.

antoninbas · 2018-08-20T18:12:00Z

p4-16/psa/PSA.mdk

+The `psa_empty_group_action` property of a table is similar to the
+`default_action` property in the following ways:
+
+ They both have actions as their values.


For P4Runtime 1.0 - to avoid making the TableEntry message too complex - it is likely that we will require the "empty group action" to be const.

Understood. Is it reasonable to document that detail in the P4Runtime API spec? Or would it better be mentioned here as something like "See the P4Runtime API spec for details, but at least P4Runtime API v1.0 will not define any messages for modifying the empty group action."

Yes we definitely to mention this in the spec. I have opened an issue to keep track of this: p4lang/p4runtime#43

I have gone ahead and added a sentence similar to the one I mentioned above. Here is the proposed text:

"In the absence of a const modifier, the control software is
allowed to change the action. The P4 API working group has no plans
to define a message for modifying a table's empty group action in
version 1.0 of the P4Runtime API specification, but such a message
might be added in a later version."

antoninbas · 2018-08-20T20:40:28Z

p4-16/psa/PSA.mdk

+
+Below we describe one way that nearly achieves the goals above, except
+that it requires the empty group action to have the same name as the
+non-empty groups of the action selector.  This may be too onerous of a


maybe it would be clearer to repeat the word "action" here.

If you meant replacing "the same name as" with "the same action name as", then I have made that change.

If you meant something else, let me know.

I may be misunderstanding the sentence, but I meant:
"it requires the empty group action to be the same as the action for non-empty groups"

Not sure if this clarifies anything here, but somehow we need a clear way to describe the proposed restriction on the actions that are allowed within the same action selector group.

I've been trying to use "action name" to mean the name of the action, without any action parameter values specified.

If "action" means "action name plus action parameter values", then there are places in this description we definitely don't want to use the word "action" alone.

I think we're on the same page, you should feel free to merge
I think replacing "name" with "action name" like you did in your latest commit made the sentence clearer

…ly good

jafingerhut · 2018-08-29T23:32:51Z

@cc10512 I think Antonin and Konstantin are OK with these changes now. I have read through them one last time and done one hopefully final typo fix. If you could give it a read, I'll be happy to merge it once it is in a state where you approve, too.

Here is a summary of the changes:

Recommend (but do not require) that a PSA data plane table with action selector implementation, when matching a table entry that points at an empty group, executes the action assigned to a new table property called psa_empty_group_action, similar to the existing default_action table property. For PSA v1.1, a PSA data plane is allowed to have unpredictable packet processing behavior in that situation, but we may make the recommendation a requirement in some future PSA version.
A new appendix explains another way to achieve the desired empty group action effect, but only if the PSA implementation supports multiple different action names within the same action selector group at the same time (which is not required by PSA). I would guess that such a technique might be useful to some PSA implementers in supporting the recommended empty group behavior.

cc10512

Please tighten up the spec itself. It is a bit too intermingled with restrictions and future plans. Some of those are already discussed in the appendix, and we can do more in terms of examples. However, it should be more clear what is the PSA specification that we define for this version.

cc10512 · 2018-08-31T17:01:38Z

p4-16/psa/PSA.mdk

+does have this restriction, it must still allow different members of
+the same group to have different action parameter values.  PSA
+implementations must also allow the action names of different groups
+in a selector to be different.


I think this paragraph may benefit from a bit of reorganization for clarity.
In particular, it would be good to first list the requirements: must have at lease one action in each group, allow different actions in different groups. And introduce the psa_emtpy_group_action property (it simply occurs later) and its semantics: provide the must have action when there are no entries in the group. Then discuss the possible extensions and restrictions: may allow different parameter values, may restrict to one action.

Thanks for detailed review comments. Take a look at the latest version when you get a chance. I have attempted to address all of your comments, although I am only replying to this one.

cc10512 · 2018-08-31T17:05:54Z

p4-16/psa/PSA.mdk

+earlier `G` was non-empty.  We will consider requiring such support in
+future versions of PSA.
+
+Until then, implementations are allowed to have non-portable


Please specify what is supported today first, rather than base it on what will be required in the future. Then say this may be tightened in the future.

cc10512 · 2018-08-31T17:06:30Z

p4-16/psa/PSA.mdk

+as far as the P4Runtime client controller software can observe.  See
+Appendix [#appendix-empty-action-selector-groups].
+
+The `psa_empty_group_action` property of a table is similar to the


Please move higher in the section. Have the spec first, then the example.

cc10512 · 2018-08-31T17:07:32Z

p4-16/psa/PSA.mdk

+
+ They both have actions as their values.
+ The P4 source code specifies the initial value.
+ If the table property is not given in the P4 source code, its value


Is that the psa_empty_group_action property? Please be precise.

cc10512 · 2018-08-31T17:08:57Z

p4-16/psa/PSA.mdk

+ They may have a `const` modifier, indicating that control software
+  is not allowed to change this action.
+ In the absence of a `const` modifier, the control software is
+  allowed to change the action.  The P4 API working group has no plans


Please remove the plans for the P4 API working group. It is not relevant, and some implementations of P4 runtime may already chose to implement such a message.

cc10512 · 2018-08-31T21:01:33Z

Thank you for making the changes. Looks good to me!

jafingerhut · 2018-08-31T21:11:51Z

Great. I am going to hurry up and merge this now, before any more comments arise :)

PSA - Add notes about empty groups for the ActionSelector extern

f5328f8

jafingerhut requested review from cc10512 and antoninbas August 9, 2018 00:08

jafingerhut mentioned this pull request Aug 11, 2018

PSA Explicitly forbid empty ActionSelector groups to be used by a tab… #575

Closed

antoninbas reviewed Aug 15, 2018

View reviewed changes

jafingerhut added 2 commits August 14, 2018 19:35

PSA eliminate contradiction between statements

ec0ac0f

Add a new table property psa_empty_group_action

437cdc3

This is like `default_action`, except that it specifies an action for a table with ActionSelector implementation to execute if an empty group is encountered.

Update the appendix to use "empty group action" instead of "default a…

6c41d10

…ction"

antoninbas approved these changes Aug 20, 2018

View reviewed changes

antoninbas reviewed Aug 20, 2018

View reviewed changes

jafingerhut added 4 commits August 20, 2018 15:20

PSA - address comments about definition of psa_empty_group_action

5d70982

PSA - one more minor wording comment on psa_empty_group_action addressed

503f2c9

PSA - Explain a condition where the empty group workaround is perfect…

2fbd57f

…ly good

PSA - Remove a partial sentence that should not have been there.

beffab8

cc10512 suggested changes Aug 31, 2018

View reviewed changes

PSA Updates based on review comments.

b5ff154

cc10512 approved these changes Aug 31, 2018

View reviewed changes

PSA Correct some typos.

dbbb830

jafingerhut merged commit 1069653 into p4lang:master Aug 31, 2018

jafingerhut mentioned this pull request Aug 31, 2018

PSA What should be specified for an empty group in an ActionSelector? #573

Closed

PSA - Add notes about empty groups for the ActionSelector extern #662

PSA - Add notes about empty groups for the ActionSelector extern #662

Conversation

jafingerhut commented Aug 9, 2018

jafingerhut commented Aug 9, 2018 • edited

ghost commented Aug 9, 2018

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut Aug 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut Aug 15, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut commented Aug 17, 2018

ghost commented Aug 17, 2018

antoninbas commented Aug 20, 2018

jafingerhut commented Aug 20, 2018

jafingerhut commented Aug 20, 2018

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut Aug 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jafingerhut commented Aug 29, 2018

cc10512 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cc10512 commented Aug 31, 2018

jafingerhut commented Aug 31, 2018

jafingerhut commented Aug 9, 2018 •

edited

jafingerhut Aug 15, 2018 •

edited

jafingerhut Aug 15, 2018 •

edited

jafingerhut Aug 20, 2018 •

edited