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

Define the term data plane volatile and how it relates to read-write symmetry #412

Merged
merged 5 commits into from
Feb 17, 2023

Conversation

jafingerhut
Copy link
Contributor

No description provided.

@jafingerhut
Copy link
Contributor Author

Intended to resolve #408

@jafingerhut
Copy link
Contributor Author

TODO: Does the MessageDifferencer function mentioned in the spec ignore all of the data plane volatile fields when comparing read values against write values, at least for things that are data plane volatile in PSA?

I would guess it would need to be updated for the new things that can be data plane volatile in PNA.

@jafingerhut
Copy link
Contributor Author

@smolkaj @jonathan-dilorenzo @verios-google FYI please read it over to see if you have any corrections/comments/questions.

@smolkaj
Copy link
Member

smolkaj commented Jan 17, 2023

@verios-google for visibility

Copy link
Contributor

@verios-google verios-google left a comment

Choose a reason for hiding this comment

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

Great change, good job including the PSA and PNA specific items.
I have a question, would the field time_since_last_hit (Protobuf with elapsed_ns) still be considered data plane volatile in non-PSA/non-PNA implementations? Or would that depend on the implementation?

@jafingerhut
Copy link
Contributor Author

Great change, good job including the PSA and PNA specific items. I have a question, would the field time_since_last_hit (Protobuf with elapsed_ns) still be considered data plane volatile in non-PSA/non-PNA implementations? Or would that depend on the implementation?

It would depend upon the behavior of that architecture, but if you are asking about the v1model architecture, then yes it is data plane volatile.

I have a difficult time imagining an implementation that claims to support idle timeout in a way compatible with the existing defined P4Runtime API messages, including their documentation, where those fields were not data plane volatile. But it seems safest to qualify the documentation with the architectures we know, rather than ones that we do not.

Those were copy-paste mistakes.
Copy link
Member

@smolkaj smolkaj left a comment

Choose a reason for hiding this comment

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

This looks great from my PoV. Thanks Andy!

@@ -2185,6 +2185,114 @@ C++ API supports comparing messages while treating repeated fields as sets, so
that different orderings of the same elements are considered equal. This method
of comparing Protobuf messages may come at a cost in performance.

### Data plane volatile objects

An exception to read-write symmetry are objects whose contents or
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Should we briefly mention this exception and add a forward reference to this section at the beginning of the previous section, so people don't miss it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

sorry for the delay, LGTM

I intend to merge this by end of day

Copy link
Contributor

@jonathan-dilorenzo jonathan-dilorenzo left a comment

Choose a reason for hiding this comment

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

Super nice! Thanks Andy!


For a PSA [@PSA] table with property `psa_idle_timeout` equal to
`PSA_IdleTimeout_t.NOTIFY_CONTROL`, the data plane can modify the
`elapsed_ns` field of a `TabeEntry` when _no_ packets match the entry
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: TabeEntry -> TableEntry

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@antoninbas antoninbas merged commit 10f2c03 into p4lang:main Feb 17, 2023
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