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 routing groups #546

Merged
merged 23 commits into from
Jun 4, 2024
Merged

Add routing groups #546

merged 23 commits into from
Jun 4, 2024

Conversation

marian-pritsak
Copy link
Collaborator

Make an intermediate stage between ENI and routing to be able to atomically bind/unbind an entire
LPM table to/from ENI.

@KrisNey-MSFT KrisNey-MSFT requested a review from r12f April 3, 2024 16:49
@marian-pritsak marian-pritsak force-pushed the rg branch 14 times, most recently from 9f586cb to 10fe8cf Compare April 18, 2024 17:31
@@ -6,12 +6,29 @@
control outbound_routing_stage(inout headers_t hdr,
inout metadata_t meta)
{

action set_routing_group_version(bit<1> admin_state) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

since the parameter is not a version, do you mind to change this to set_routing_group_attr?

Copy link
Collaborator

Choose a reason for hiding this comment

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

also do you mind to change admin_state to disabled?

there are 2 reasons:

  • disabled is more intuitive than admin_state.
  • admin_state should not be frequently used (otherwise, why programming it), hence it will be better to make use of the default value.

Making this field disabled with default value set to false seems to be a good way to achieve these 2 things.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -33,8 +50,13 @@ control outbound_routing_stage(inout headers_t hdr,
return;
}

routing_group.apply();
if (meta.eni_data.routing_group_admin_state) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it is better to create a dedicated structure for routing group data, then we can use meta.routing_group_data.admin_state to access the related states.

As metadata becomes more and more complicated, it will be more future-proof.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

routing.apply();
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: drop and return first might be better then if else : D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

dash-pipeline/bmv2/stages/outbound_routing.p4 Show resolved Hide resolved
@marian-pritsak marian-pritsak force-pushed the rg branch 8 times, most recently from 6bc69eb to 69d3d05 Compare April 19, 2024 22:46
Make an intermediate stage between ENI and routing
to be able to atomically bind/unbind an entire
LPM table to/from ENI.

Signed-off-by: Marian Pritsak <[email protected]>
@KrisNey-MSFT
Copy link
Collaborator

PTF tests ok, SAI Challenger tests will be run locally on Marian's machine

@marian-pritsak marian-pritsak force-pushed the rg branch 2 times, most recently from 1ade41e to a0d995a Compare May 28, 2024 15:47
Signed-off-by: Marian Pritsak <[email protected]>
@marian-pritsak marian-pritsak force-pushed the rg branch 2 times, most recently from a97b5e0 to b90351a Compare May 29, 2024 16:27
@KrisNey-MSFT
Copy link
Collaborator

DPUGen to generate config; has hard-coded entries; ENI_ID is no longer a field; @mgheorghe will take a look at fixing the test, fix the outbound routing entry to use routing_group ID and struct instead of ENI.

@r12f r12f merged commit b3d7a6a into sonic-net:main Jun 4, 2024
11 checks passed
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.

4 participants