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

Add description of new value AUTO_DELETE for table property pna_idle_timeout #92

Conversation

jfingerh
Copy link
Contributor

Also clarify some wording in the description of add-on-miss, and remove the old signature in the document, in favor of using Madoko INCLUDE mechanism to include the corresponding section of the pna.p4 include file, so that the document and include file will remain in sync with each other.

Also clarify some wording in the description of add-on-miss, and
remove the old signature in the document, in favor of using Madoko
INCLUDE mechanism to include the corresponding section of the pna.p4
include file, so that the document and include file will remain in
sync with each other.
up and resulted in a miss, and the action name and action parameters
specified by the parameters of the call to the `add_entry` extern
function. Thus, future packets that invoke `t.apply()` with the same
lookup key will get a match and invoke the specified action (until and
Copy link

Choose a reason for hiding this comment

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

Does this not assume in-order processing of packets across multiple flows? Should we not state this explicitly?

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 agree that there is a potentially subtle interaction here between pipelines that can have packets "pass each other up" in the middle of the pipeline, versus those that do not, and it would be good to address this issue somehow in the PNA spec. I will create a separate issue to track this, since I doubt I will quickly find an acceptable description, and that property of a packet processing pipeline probably affects other features besides this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created this issue: #96 Feel free to add comments on the topic there, but I hope it is reasonable to allow this PR to be merged before that issue is fully resolved.

Copy link

Choose a reason for hiding this comment

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

Agree, we should merge this.

@thomascalvert-xlnx
Copy link
Member

LGTM.

(It did leave me wondering whether the pna_idle_timeout and idle_timeout_with_auto_delete properties could be unified into a single one... (Maybe make AUTO_DELETE an additional member of the PNA_IdleTimeout_t enum?) But that's not the topic of this commit. :) )

@jfingerh
Copy link
Contributor Author

@thomascalvert-xlnx I like the idea of making AUTO_DELETE a new member of the enum PNA_IdleTimeout_t. I suppose some targets might even be able to implement a combination of notifying the control plane AND auto-deleting the entry when an entry expires, but no need to add that unless there is a request to do so.

I will go ahead and change this PR with this idea, and see what others reviewers think.

@jfingerh jfingerh changed the title Add description of idle_timeout_with_auto_delete Add description of new value AUTO_DELETE for table property pna_idle_timeout Nov 25, 2022
Copy link
Member

@thomascalvert-xlnx thomascalvert-xlnx left a comment

Choose a reason for hiding this comment

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

Yes I like the AUTO_DELETE option, it looks cleaner IMHO.

Suggest that the text talks about how entries can be explicitly deleted (not via timeout but on seeing e.g. a RST packet), and briefly describe the semantics of ExpireTimeProfileId_t. But that could easily be added in a separate change to this one.

@thantry
Copy link

thantry commented Nov 28, 2022

LGTM

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.

None yet

5 participants