Skip to content

Conversation

angelazhang8
Copy link
Contributor

@angelazhang8 angelazhang8 commented Aug 7, 2025

Motivation

Our switches currently support per-group hash modes and resource usage modes. We are currently not leveraging this capability. This P4 API change allows for more granular control of the WCMP implementation in GPINS.

Summary of changes

Add BucketSelectionMode (with modes DEFAULT_MODE_DETERMINED_BY_ACTION_SELECTOR, HASH, and RANDOM) and SizeSemantics (with modes DEFAULT_SIZE_DETERMINED_BY_ACTION_SELECTOR, SUM_OF_WEIGHTS, and SUM_OF_MEMBERS) to ActionProfileActionSet in p4runtime.proto.

@chrispsommers
Copy link
Collaborator

@angelazhang8 Thanks for your submission. Could you please add a description, including summary of changes and a motivation for this? Will you be able to present this to us in the WG meeting on Friday 8/8/25?

@matthewtlam
Copy link
Contributor

matthewtlam commented Aug 7, 2025

Please update the docs/v1/P4Runtime-Spec.adoc accordingly with the restrictions, expected behavior, etc. You can run the lint tool using ./tools/asciidoclint.py to format the spec.

Also please use the script ./codegen/update.sh to update the GoLang, Python and Rust versions as specified https://github.com/p4lang/p4runtime?tab=readme-ov-file#detailed-processes

@angelazhang8
Copy link
Contributor Author

@angelazhang8 Thanks for your submission. Could you please add a description, including summary of changes and a motivation for this? Will you be able to present this to us in the WG meeting on Friday 8/8/25?

Done. Yes, I am planning to present this in the WG meeting tomorrow.

@angelazhang8
Copy link
Contributor Author

@jafingerhut Can you also PTAL? Thanks!

@jonathan-dilorenzo
Copy link
Contributor

Will defer to @jafingerhut and @chrispsommers on if we wwant to add for 1.5 or 1.4.2 (or whatever). This should be a non-breaking change as designed.

@jonathan-dilorenzo
Copy link
Contributor

@jafingerhut and @chrispsommers, we internally don't currently have a use-case, but we would be totally amenable to adding these same knobs to ActionProfileGroups for feature parity (since otherwise the selector seems to become more expressive which is a bit odd imo).

@jonathan-dilorenzo
Copy link
Contributor

In SAI, there's a per-group hash algorithm attribute, which one can set to SAI_HASH_ALGORITHM_RANDOM to get the random behavior specified here. I'm not aware of switches that allow you to otherwise set arbitrary hash algorithms per group (though they may exist), but we use NONE to pick the default per-switch hash algorithm.

Still looking at the SUM_OF_WEIGHTS vs SUM_OF_MEMBERS, but I suspect it comes from these SAI group types. Perhaps the as-of-yet-undefined WCMP type is the one we use for SUM_OF_MEMBERS and have not yet upstreamed.

@angelazhang8 angelazhang8 changed the title Add BucketSelectionMode and SizeSemantics to ActionProfileActionSet in P4Runtime. Add ActionSelectionMode and SizeSemantics to ActionProfileActionSet in P4Runtime. Aug 11, 2025
@jonathan-dilorenzo
Copy link
Contributor

jonathan-dilorenzo commented Aug 12, 2025

@jafingerhut, @verios-google, @matthewtlam, and @chrispsommers, can we lock in the current proto (possibly with some additional comments)? Or any concerns with the naming?

Edit: So, I just realized that we need the proto submitted here before we can move forward internally too (since we have external repositories depending on this). We can try to make quick progress on the prose if you'd like? Or maybe we can even do something dumb and submit the proto without prose (or with imperfect prose) saying that the prose is coming by next meeting? If so, we would commit to making all the necessary improvements.

@jafingerhut
Copy link
Contributor

@jafingerhut, @verios-google, @matthewtlam, and @chrispsommers, can we lock in the current proto (possibly with some additional comments)? Or any concerns with the naming?

Edit: So, I just realized that we need the proto submitted here before we can move forward internally too (since we have external repositories depending on this). We can try to make quick progress on the prose if you'd like? Or maybe we can even do something dumb and submit the proto without prose (or with imperfect prose) saying that the prose is coming by next meeting? If so, we would commit to making all the necessary improvements.

On the .proto file changes alone, I have no objections to the current names. The most important thing is that they are explained clearly in some section of the specification before the next spec release, preferably with exactly what names they correspond to in the SAI APIs, with links to relevant SAI documentation. I think it is fine for those details NOT to be in the .proto file, but only in the P4Runtime specification document.

Copy link
Contributor

@matthewtlam matthewtlam left a comment

Choose a reason for hiding this comment

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

Besides my small NITs, everything LGTM. Approving early, but should fix the issue with one of the BUILDs failing though

@jonathan-dilorenzo
Copy link
Contributor

@jafingerhut, for the SUM_OF_MEMBERS that corresponds to an as-of-yet not upstreamed (but in the process of being upstreamed) SAI attribute (SAI_NEXT_HOP_GROUP_ATTR_NATIVE_WCMP). So I'm not sure we can say anything useful there yet.

@jonathan-dilorenzo
Copy link
Contributor

@jafingerhut and @angelazhang8, wdyt about separating out the proto PR from the spec PR so that we can keep leaving comments on the spec part without blocking on it?

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.

I am OK with approving the .proto now and even leaving the spec a little bit WIP, but let's create an issue to update the spec in a timely manner. We can bump it all to 1.5.1 then? I believe in progress.

@jafingerhut
Copy link
Contributor

jafingerhut commented Aug 12, 2025

SInce some of the comments here are about the spec, not the .proto file, perhaps leave this issue as it is now, and create a new one for the .proto file change by itself that can proceed faster?

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.

This is great! Thanks Angela! Mostly small comments, so marking approved.

@matthewtlam
Copy link
Contributor

matthewtlam commented Sep 11, 2025

@angelazhang8 Please add a comment about the new fields added to the ActionProfile message.

image

EDIT: my bad, it seems the website is not updated with the latest changes from the spec as I see you've already updated it

@chrispsommers
Copy link
Collaborator

chrispsommers commented Sep 12, 2025

@angelazhang8 It seems like you need to fix the signoff problem, there is a procedure to amend the commits, follow the DCO link

Signed-off-by: Angela Zhang <[email protected]>
Signed-off-by: Angela Zhang <[email protected]>
Signed-off-by: Angela Zhang <[email protected]>
Signed-off-by: Angela Zhang <[email protected]>
Signed-off-by: Angela Zhang <[email protected]>
…fileActionSet in P4Runtime. (p4lang#561)

* Create a separate PR for just the P4Runtime proto changes.

Signed-off-by: Angela Zhang <[email protected]>

* address comments

---------

Signed-off-by: Angela Zhang <[email protected]>
Signed-off-by: Angela Zhang <[email protected]>
@jonathan-dilorenzo
Copy link
Contributor

LGTM and ready to merge from my perspective. @chrispsommers and @jafingerhut.

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.

6 participants