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

PSA: Should there be a clone_port field for egress-to-egress clones? #492

Closed
jafingerhut opened this issue Nov 16, 2017 · 6 comments
Closed

Comments

@jafingerhut
Copy link
Collaborator

@cc10512 @vgurevich @hanw

As the PSA draft is right now, a normal packet sent from ingress to egress has an egress_port associated with it, chosen in ingress or the PRE, and egress cannot change it. It can choose to drop or recirculate the packet instead of sending it out to that port, but it cannot change the egress_port.

For egress-to-egress clone packets, it may just be an oversight, or it may be by design, that there is no clone_port in the type psa_egress_output_metadata_t.

Is it intended that during egress processing, if the P4 program decides to create a clone of the packet, that it also gets to pick the destination port of the cloned packet? And that destination port can be any port, i.e. it is not restricted to be the same as the egress_port of the egress packet we are cloning?

If the answer is "yes", then there is an easy fix -- add a clone_port field to struct psa_egress_output_metadata_t.

If the answer is "no", there are several other small edits needed in the PSA, I believe, but wanted to get feedback on the questions above first before changing anything.

@jafingerhut
Copy link
Collaborator Author

@hanw Any thoughts on this? It is a one-line addition to fix it if it is simply a missing metadata field.

@hanw
Copy link
Contributor

hanw commented Dec 6, 2017

The situation is slightly more involved than just adding a clone_port. We have two options: 1, clone_port (and clone_cos) is configured by the runtime as part of PRE API, and selected on data plane using an index (clone_session_id). 2: clone_port is configured by the data plane on a per-packet basis, and no runtime configuration is involved. Ideally, a portable solution should support both. I personally think the second is cleaner, but it may be difficult to support the first scenario, which is undesirable.

@jafingerhut
Copy link
Collaborator Author

What is the advantage of supporting the first scenario?

Easier to auto-translate programs from P4_14 egress-to-egress clone?

@jafingerhut
Copy link
Collaborator Author

Discussed during 2017-Dec-06 P4 Arch WG meeting. Proposal at that time was that in order to clone, P4 code must fill in a new metadata field that specifies a clone_session_id. In the PRE, each clone_session_id can be configured by the control plane to correspond to a pair of values clone_port and clone_class_of_service.

@jafingerhut
Copy link
Collaborator Author

This PR proposes changes to the PSA for using a clone_session_id to specify the destination of cloned packets: #515

@jafingerhut
Copy link
Collaborator Author

The PR adding clone sessions, used when creating clones in ingress or egress, was merged in. Closing this issue.

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

No branches or pull requests

2 participants