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.

@@ -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
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

Signed-off-by: Marian Pritsak <[email protected]>
@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