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.

Sign up for GitHub

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

Jump to bottom

[libsai] Re-implement APIs create/remove #653

Merged
merged 7 commits into from
Jan 8, 2025
Merged

[libsai] Re-implement APIs create/remove #653

merged 7 commits into from
Jan 8, 2025

Conversation

jimmyzhai
Copy link
Collaborator

@jimmyzhai jimmyzhai commented Dec 19, 2024 •
edited
Loading

Following #651, refactor APIs create/remove for easier read and maintenance:

  1. Use common code DashSai::create and DashSai::remove to implement them for all SAI objects/entries
  2. Fix issue Why dash_sai_create_dash_acl_rule create multiple SAI_OBJECT_TYPE_DASH_ACL_RULE objects? #436, now only 1 SAI object is created for multiple bmv2 objects (each bmv2 object as a p4 table entry in p4 table for each stage).
  3. Fix issue Per-action parameter support in dash libsai #654

The generated sample code of APIs create/remove attribute is as below:

  • SAI object objectID
    image

  • SAI object entry
    image

For the case of acl rule object, one acl rule is inserted into multiple p4 tables in different stages. These p4 stage tables are same and the only difference is table id and table action id in table entry, captured in struct P4MetaSiblingTable:

    struct P4MetaSiblingTable
    {
        uint32_t id;
        // action enum id -> p4 action id
        std::map<uint32_t, uint32_t> actions;
    };

The generated code of sibling tables for acl rule is as below:
image

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai jimmyzhai force-pushed the libsai_create_remove branch from d7f18a9 to 28dabbe Compare December 19, 2024 13:53
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai jimmyzhai force-pushed the libsai_create_remove branch from 5c2bc0b to 6b8b884 Compare December 20, 2024 13:48
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai jimmyzhai force-pushed the libsai_create_remove branch from 6b8b884 to 8b334c3 Compare December 20, 2024 14:45
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai jimmyzhai marked this pull request as ready for review December 20, 2024 15:42
@jimmyzhai jimmyzhai requested review from kcudnik and r12f December 20, 2024 15:42
kcudnik
kcudnik reviewed Dec 20, 2024
View reviewed changes
dash-pipeline/SAI/src/p4meta.cpp Outdated
Comment on lines 27 to 37
{
for (auto i=0u; i<keys.size(); i++) {
if (key_name == keys[i].name) {
return &keys[i];
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

over entire PR incosistent placemtn { once in end of function once in new line, over all project has { in new line

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@jimmyzhai jimmyzhai mentioned this pull request Dec 24, 2024
Merged
@jimmyzhai jimmyzhai force-pushed the libsai_create_remove branch from 8b334c3 to 93e8cca Compare December 24, 2024 16:23
@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai jimmyzhai requested a review from kcudnik January 6, 2025 13:37
r12f
r12f approved these changes Jan 8, 2025
View reviewed changes
Copy link
Collaborator

@r12f r12f left a comment

Choose a reason for hiding this comment

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

this change looks good to me! do you mind to add an example of the generated sibling table related metadata into the PR description? It will be great for people to understand the change.

r12f
r12f reviewed Jan 8, 2025
View reviewed changes
dash-pipeline/SAI/src/dashsai.cpp Outdated

for (uint32_t i = 0; i < attr_count; i++)
{
if (auto meta_param = meta_table.get_meta_action_param(action_id, attr_list[i].id))
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to move the assignment out of the if. this code pattern is easy to go wrong in general.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

dash-pipeline/SAI/src/dashsai.cpp
}

std::shared_ptr<p4::v1::TableEntry> entry = std::make_shared<p4::v1::TableEntry>();
entry->CopyFrom(*matchActionEntry);
Copy link
Collaborator

Choose a reason for hiding this comment

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

will be better to add a comment here to indicate all key and attribute index/id are the same.

Copy link
Collaborator

Choose a reason for hiding this comment

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

only the action is different.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added comments

@mssonicbld
Copy link

/azp run

@azure-pipelines Azure Pipelines
Copy link

Commenter does not have sufficient privileges for PR 653 in repo sonic-net/DASH

@jimmyzhai
Copy link
Collaborator Author

this change looks good to me! do you mind to add an example of the generated sibling table related metadata into the PR description? It will be great for people to understand the change.

Updated PR description to have the generated sibling tables for acl rule.

@r12f r12f merged commit 88a4771 into sonic-net:main Jan 8, 2025
4 checks passed
This was referenced Jan 21, 2025
Closed
Closed
@jimmyzhai jimmyzhai deleted the libsai_create_remove branch January 21, 2025 03:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@r12f r12f r12f approved these changes

@kcudnik kcudnik kcudnik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jimmyzhai @mssonicbld @r12f @kcudnik