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

Support multicast resource information. #472

Merged
merged 8 commits into from
Apr 24, 2024
Merged

Conversation

jonathan-dilorenzo
Copy link
Contributor

On real hardware, multicast resources are presumably finite. While we have a way of capturing that for other tables (with the size parameter) and for action profiles (with the max_group_size parameter), there is currently no way to capture this information for multicast (or for the clone session table for that matter).

This PR is a first stab at adding such a mechanism. In my mind, these fields would be populated by users in the P4 program through the @pkginfo annotation (much like the rest of pkginfo).

Obviously, this is currently a WIP and I'd want to add something to the spec too, but I wanted to see if the idea even made sense first.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

Hi John, I think the idea of getting platform capabilities is a good one but I'm not necessarily in favor of making it part of "PkgInfo," which to me provides program-specific metadata, not platform-specific. Perhaps we need a generic platform capabilities query API returning KV pairs?

@jonathan-dilorenzo
Copy link
Contributor Author

Yeah, PkgInfo feels a bit weird.

But I don't think we quite want to query the platform for it... Rather, we want to tell the platform our desired capabilities and then it is up to the platform to reject them if it can't fulfill our desires or accept them otherwise. Like with all other tables really. So I think we'd want to fit it into the P4Info somehow...

But good to discuss options in the meeting!

@jonathan-dilorenzo
Copy link
Contributor Author

Based on discussions in the P4 API WG with @chrispsommers, @smolkaj, and @jafingerhut, there seems to be tentative support for adding something like this (with a few changes I'm about to make) as a short-term solution to modeling multicast resources (and giving the switch a directive to reject a P4Info if it can't).

In the long-term, we want to look into treating multicast like any other P4 table, essentially specifying that a table is a multicast table by attaching something much like an ActionProfile except that it replicates packets to all actions instead of choosing one non-deterministically (or deterministically based on a hash function).

A first approach to the long-term solution might be to disallow applying multicast tables in the P4 program, and then they would instead be applied where ever the architecture uses a multicast table.

We also discussed that these would be useful to provide as capabilities, so I'm gonna try to add that in a follow-up PR. The idea will be that, if the numbers are given in the P4Info, then the capability numbers must be >= to those in the P4Info (or the P4Info should otherwise be rejected). If they are not given, then the switch can still be queried to see how many multicast entries and replicas it supports.

@chrispsommers
Copy link
Collaborator

This seems fine as a working idea, but if I were doing this, I'd probably make changes in p4c to vet the idea a bit further.

@jonathan-dilorenzo
Copy link
Contributor Author

@chrispsommers, to be clear, the "changes in p4c" you're referring to is the annotation interpretation?

@chrispsommers
Copy link
Collaborator

@chrispsommers, to be clear, the "changes in p4c" you're referring to is the annotation interpretation?

Correct, the front-end which parses annotations and renders P4Info. You might find a desire to tweak the schema after trying to implement the enhancement.

@jonathan-dilorenzo
Copy link
Contributor Author

Added support for this PR in p4lang/p4c#4611 (though this would need to go in first, so that the P4Info fields exist to be set).

@jonathan-dilorenzo
Copy link
Contributor Author

AI: Change multicast_table to multicast_group_table and then send out again for review and we'll try to get this in before next meeting.

@jonathan-dilorenzo
Copy link
Contributor Author

I think this is now ready for a final review as discussed @chrispsommers, @smolkaj, and @jafingerhut.

Copy link
Collaborator

@chrispsommers chrispsommers left a comment

Choose a reason for hiding this comment

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

LGTM

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.

A few nits

docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Outdated Show resolved Hide resolved
docs/v1/P4Runtime-Spec.mdk Show resolved Hide resolved
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.

One more comment. LGTM otherwise. Approving early.

proto/p4/config/v1/p4info.proto Outdated Show resolved Hide resolved
@smolkaj smolkaj merged commit fee499a into main Apr 24, 2024
6 checks passed
@smolkaj smolkaj deleted the capture_multicast_resources branch April 24, 2024 21:44
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.

3 participants