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 a section with notes on PRE multicast replication behavior #513

Merged
merged 2 commits into from Dec 15, 2017

Conversation

jafingerhut
Copy link
Collaborator

Also restrictions on what kinds of items can be added to multicast
groups.

Also restrictions on what kinds of items can be added to multicast
groups.
@jafingerhut
Copy link
Collaborator Author

@samar-abdi I don't see how in Github to add you as a reviewer, but please take a look at this and add any comments you have.

Copy link
Contributor

@cc10512 cc10512 left a comment

Choose a reason for hiding this comment

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

LGTM.

(egress_port[N-1], instance[N-1])

When a packet is sent to that group, `N` copies of the packet are
made. Copy number `i` that is sent to egress processing will have its
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very strong requirement. I do not think that the (port, instance) pairs should be assumed to be in any order and I would not require the replication to follow ay order either.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't read the statement as imposing an ordering, just the pairing. Each copy will be uniquely associated with a pair.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree that this text could be misread to imply that copies are generated in a particular order, and I can try to come up with a different way to describe it that doesn't imply that.

In Section 6.2 of the current PSA draft, there is a requirement that PSA implementations process packets in the egress control block in the same relative order that they were processed in the ingress control block, for multicast packet copies with the same (ingress_port, multicast_group, class_of_service, egress_port) values (OK, it doesn't say it exactly that way, but perhaps it should). That has implications on how packet replication can be implemented. It doesn't imply that the packet replication set is a list with replication order determined by the control plane, but I expect many implementations would choose to do something like that.

If you think that shouldn't be a requirement, then we should discuss it, because otherwise you are probably going to make some multicast users pretty unhappy with your implementation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've updated the PR with another commit intended to answer @vgurevich's comment.

Section 6.2 actually doesn't describe the FIFO properties as requirements, but 'expectations'. That may be wishy-washy in language, and we may want to change it to a requirement, or just a recommendation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@vgurevich Do these modified changes look better to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

@jafingerhut ,

Sorry for the late reply. Yes, the changes look good. Thank you!

Also hopefully clarify the expectation of packet ordering in a PSA
system for multicast packets, that is earlier in Section 6.2.
@jafingerhut jafingerhut merged commit a6a93c7 into p4lang:master Dec 15, 2017
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

3 participants