[libsai] Implement APIs set/get attribute #651

jimmyzhai · 2024-12-16T16:10:14Z

Following #648, continue to refactor libsai implementation for easier read and maintenance:

Avoid using compilated j2 template as best
Use c++ code directly to express the logic of API implementation, based on p4 table metadata info (p4meta.[h, cpp])
Implement APIs set/get attribute by using common DashSai::set and DashSai::get, which fix the issue dash libsai does not support APIs set/get attribute of SAI object/entry #650
Update libsai test (vnet_out.cpp) to cover sanity test of APIs set/get attribute

Note: To refactor APIs create/remove with current approach in next PR.

The generated sample code of APIs set/get attribute is as below:

SAI object objectID

p4meta outbound_routing_group_meta_table definition
SAI object entry

p4meta outbound_routing_entry_meta_table definition

2. Implement set/get attribute

2. Correctly set/get mask for ternary key

mssonicbld · 2024-12-16T16:10:18Z

/azp run

azure-pipelines · 2024-12-16T16:10:25Z

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

r12f · 2024-12-18T02:31:17Z

Hi Junhua, do you mind to update the PR description on how the generated code looks like? It will help people to understand this PR better.

r12f · 2024-12-18T03:07:31Z

dash-pipeline/SAI/src/p4meta.h

+
+    template<typename T>
+    static inline
+    void get_value(


better rename the functions. the names are too vague.

fixed. Update functions to have a meaningful name.

r12f · 2024-12-18T03:08:05Z

dash-pipeline/SAI/src/p4meta.h

+            _In_ const p4::v1::FieldMatch_LPM *t,
+            _Out_ sai_attribute_value_t &value)
+    {
+        assert (field == "ipPrefix");


move non-template to p4meta.cpp

2. Move non-template function implementations to p4meta.cpp

mssonicbld · 2024-12-19T08:03:54Z

/azp run

azure-pipelines · 2024-12-19T08:04:01Z

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

jimmyzhai · 2024-12-19T08:26:33Z

Hi Junhua, do you mind to update the PR description on how the generated code looks like? It will help people to understand this PR better.

Added generated sample code in PR description

r12f · 2024-12-19T19:24:17Z

Hi Junhua, do you mind to update the PR description on how the generated code looks like? It will help people to understand this PR better.

Added generated sample code in PR description

Thanks for sharing the details out! This makes the review much easier!

dash-pipeline/tests/libsai/vnet_out/vnet_out.cpp

r12f

looks good to me! commends can be solved in the next PR, all minor and non-blocking.

r12f · 2024-12-23T19:34:21Z

dash-pipeline/SAI/src/dashsai.cpp

+{
+    DASH_LOG_ENTER();
+
+    if (!m_apiInitialized)


maybe add another macro here, say DASH_CHECK_API_INITIALIZED_RET(retcode)?

fix it at #653

r12f · 2024-12-23T20:18:28Z

dash-pipeline/SAI/src/dashsai.cpp

+
+    MUTEX;
+
+    auto range = m_tableEntryMap.equal_range(id);


emmmm, do we really need a multi-map here? each object id should be unique with the object id encoding, am I right? @kcudnik

@r12f , map is enough. fix it at #653

r12f · 2024-12-23T20:21:05Z

dash-pipeline/SAI/src/p4meta.cpp

+        return pair_key;
+    }
+
+    std::pair<p4::v1::Action_Param*, p4::v1::Action_Param*> get_action_param_pair_from_p4_table_entry(


get_action_param_with_is_v6_flag_from_p4_table_entry

also use out parameter will be more explicit, the pair.second is very unintuitive to read.

fix it at #653

r12f · 2024-12-23T20:24:39Z

dash-pipeline/SAI/src/p4meta.cpp

+        return pair_param;
+    }
+
+    bool string_has_suffix(const std::string &str, const std::string &suffix)


string_ends_with

fix it at #653

@r12f , string::ends_with is since c++20. Anyway, let's name it our string_ends_with.

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 #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 #654 The generated sample code of APIs create/remove attribute is as below: - SAI object objectID ![image](https://github.com/user-attachments/assets/1f300b7a-7750-43df-89e2-71570801cd06) - SAI object entry ![image](https://github.com/user-attachments/assets/d16a3bcc-0a41-4fd1-a23a-6f6663761a67) 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](https://github.com/user-attachments/assets/4dbf0725-1ca5-488f-bf08-62b032c2f66a)

jimmyzhai added 3 commits December 13, 2024 17:01

1. Add p4meta.h, p4meta.cpp for meta info of p4 table

1a19107

2. Implement set/get attribute

1. Remove p4meta.cpp and move set/get_value to p4meta.h

46ba5c0

2. Correctly set/get mask for ternary key

Add test of set/get_attribute in vnet_out.cpp

f024213

jimmyzhai requested a review from r12f December 16, 2024 16:17

r12f reviewed Dec 18, 2024

View reviewed changes

1. Change function names to be more readable

5149bf6

2. Move non-template function implementations to p4meta.cpp

jimmyzhai requested a review from r12f December 19, 2024 08:36

jimmyzhai mentioned this pull request Dec 19, 2024

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

Merged

r12f reviewed Dec 19, 2024

View reviewed changes

dash-pipeline/tests/libsai/vnet_out/vnet_out.cpp Show resolved Hide resolved

r12f approved these changes Dec 23, 2024

View reviewed changes

r12f merged commit 65b42a8 into sonic-net:main Dec 23, 2024
4 checks passed

jimmyzhai deleted the libsai_set_get branch January 13, 2025 12:36

jimmyzhai mentioned this pull request Jan 21, 2025

dash libsai does not support APIs set/get attribute of SAI object/entry #650

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libsai] Implement APIs set/get attribute #651

[libsai] Implement APIs set/get attribute #651

jimmyzhai commented Dec 16, 2024 •

edited

Loading

mssonicbld commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024

r12f commented Dec 18, 2024

r12f Dec 18, 2024

jimmyzhai Dec 19, 2024

r12f Dec 18, 2024

jimmyzhai Dec 19, 2024

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

jimmyzhai commented Dec 19, 2024

r12f commented Dec 19, 2024

r12f left a comment

r12f Dec 23, 2024

jimmyzhai Dec 24, 2024

r12f Dec 23, 2024

jimmyzhai Dec 24, 2024

r12f Dec 23, 2024

r12f Dec 23, 2024

jimmyzhai Dec 24, 2024

r12f Dec 23, 2024

jimmyzhai Dec 24, 2024

jimmyzhai Dec 24, 2024

[libsai] Implement APIs set/get attribute #651

[libsai] Implement APIs set/get attribute #651

Conversation

jimmyzhai commented Dec 16, 2024 • edited Loading

mssonicbld commented Dec 16, 2024

azure-pipelines bot commented Dec 16, 2024

r12f commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mssonicbld commented Dec 19, 2024

azure-pipelines bot commented Dec 19, 2024

jimmyzhai commented Dec 19, 2024

r12f commented Dec 19, 2024

r12f left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmyzhai commented Dec 16, 2024 •

edited

Loading