[sai-gen] Deprecate the name annotations in favor of SaiTable #479

r12f · 2023-12-12T20:49:30Z

This PR has 2 updates:

Deprecate the @name annotations in favor of @SaiTable.
Add api_order attribute in @SaiTable annotation to help us enforce the order of the generated APIs.

The second change is a must have, otherwise for any API set that contains multiple type of entries (generated from multiple tables), the order could be changed between changes and break the ABI compatibility as the screenshot shows below:

The doc is also updated to capture the use of this new attribute.

For generated content updates:

No updates on SAI header files:

r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/SAI/experimental/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/SAI/experimental/
r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline

For libsai, table id will be updated as previous change.

r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/SAI/experimental/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/SAI/experimental/
r12f@r12f-dl380:~/data/code/sonic/DASH/dash-pipeline
$ diff SAI/lib/ ~/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/
diff SAI/lib/saidashacl.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashacl.cpp
31c31
<     tableId = 50200087;
---
>     tableId = 45323240;
diff SAI/lib/saidashdirectionlookup.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashdirectionlookup.cpp
19c19
<     pi_p4_id_t tableId = 45670434;
---
>     pi_p4_id_t tableId = 38960243;
110c110
<     pi_p4_id_t tableId = 45670434;
---
>     pi_p4_id_t tableId = 38960243;
diff SAI/lib/saidasheni.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidasheni.cpp
19c19
<     pi_p4_id_t tableId = 46804748;
---
>     pi_p4_id_t tableId = 38612462;
118c118
<     pi_p4_id_t tableId = 46804748;
---
>     pi_p4_id_t tableId = 38612462;
212c212
<     tableId = 45859274;
---
>     tableId = 47336097;
diff SAI/lib/saidashinboundrouting.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashinboundrouting.cpp
19c19
<     pi_p4_id_t tableId = 38920290;
---
>     pi_p4_id_t tableId = 42758350;
152c152
<     pi_p4_id_t tableId = 38920290;
---
>     pi_p4_id_t tableId = 42758350;
diff SAI/lib/saidashmeter.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashmeter.cpp
31c31
<     tableId = 38547152;
---
>     tableId = 49926129;
244c244
<     tableId = 39168708;
---
>     tableId = 48776568;
425c425
<     tableId = 34535910;
---
>     tableId = 47787652;
diff SAI/lib/saidashoutboundcatopa.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashoutboundcatopa.cpp
19c19
<     pi_p4_id_t tableId = 39175949;
---
>     pi_p4_id_t tableId = 48860231;
219c219
<     pi_p4_id_t tableId = 39175949;
---
>     pi_p4_id_t tableId = 48860231;
diff SAI/lib/saidashoutboundrouting.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashoutboundrouting.cpp
19c19
<     pi_p4_id_t tableId = 44067785;
---
>     pi_p4_id_t tableId = 42788937;
308c308
<     pi_p4_id_t tableId = 44067785;
---
>     pi_p4_id_t tableId = 42788937;
diff SAI/lib/saidashpavalidation.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashpavalidation.cpp
19c19
<     pi_p4_id_t tableId = 35526612;
---
>     pi_p4_id_t tableId = 48948181;
118c118
<     pi_p4_id_t tableId = 35526612;
---
>     pi_p4_id_t tableId = 48948181;
diff SAI/lib/saidashvip.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashvip.cpp
19c19
<     pi_p4_id_t tableId = 45245089;
---
>     pi_p4_id_t tableId = 38937816;
110c110
<     pi_p4_id_t tableId = 45245089;
---
>     pi_p4_id_t tableId = 38937816;
diff SAI/lib/saidashvnet.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/saidashvnet.cpp
31c31
<     tableId = 34815711;
---
>     tableId = 34579306;
diff SAI/lib/sairoute.cpp /home/r12f/data/code/sonic/DASH-exp/dash-pipeline/SAI/lib/sairoute.cpp
19c19
<     pi_p4_id_t tableId = 49279256;
---
>     pi_p4_id_t tableId = 36513740;
124c124
<     pi_p4_id_t tableId = 49279256;
---
>     pi_p4_id_t tableId = 36513740;

r12f · 2023-12-12T21:02:40Z

Ok, the PR is ready for review.

@chrispsommers , with this all tables are off the @name annotation.
@jafingerhut and @fruffy, this change should help solving the issue #347 . (@KrisNey-MSFT as FYI as well)

chrispsommers · 2023-12-12T21:03:02Z

dash-pipeline/bmv2/underlay.p4

-    @name("route|route")
-    // TODO: To add structural annotations (example: @SaiTable[skipHeaderGen=true])
+    @SaiTable[name = "route", api = "route"]
+    // TODO: To add structural annotations (example: @SaiTable[skipHeaderGen=true]]


I think this comment can be deleted. The idea of a new attribute skipHeaderGen was not followed, instead the api name omits the dash which is kind of a heuristic in the api_gen code, see here:

DASH/dash-pipeline/SAI/sai_api_gen.py

Line 910 in e6a8e98

if "dash" in sai_api.app_name:

. The purpose was to use existing underlay SAI headers, not try to generate new ones. It was convenient to look for dash in the table name but IMO it's still a hidden assumption.

Do you think it'd be more consistent to use the attribute skipHeaderGen or some other aptly-named one to explicitly direct the codegen not to emit header code for a table? It would default to false.

of course! will update the PR soon. (will be afk for a hour or so.)

thanks a lot for capturing this! i totally missed this one.

It's obscure, and in full disclosure, I oversaw the addition of underlay API so it is incumbent on me - thanks for offering to address it!

Since essentially, the dash prefix affects the generation of both the header files and implementation files, instead of using "skiopHeaderGen", I added a new attribute called "api_type" and set to "underlay" to more precisely reflect what we are doing.

The changes are commited now. Please let me know what you think! :D

and of course, no SAI header file change and only table id updates in lib files.

ok, test passed now. @chrispsommers , it should be good to take another look now.

KrisNey-MSFT · 2023-12-12T21:31:57Z

[like] Kristina Moore reacted to your message:

…

________________________________ From: Riff ***@***.***> Sent: Tuesday, December 12, 2023 9:02:52 PM To: sonic-net/DASH ***@***.***> Cc: Kristina Moore ***@***.***>; Mention ***@***.***> Subject: Re: [sonic-net/DASH] [sai-gen] Deprecate the name annotations in favor of SaiTable (PR #479) Ok, the PR is ready for review. @chrispsommers<https://github.com/chrispsommers> , with this all tables are off the @name annotation. @jafingerhut<https://github.com/jafingerhut> and @fruffy<https://github.com/fruffy>, this change should help solving the issue #347<#347> . ***@***.***<https://github.com/KrisNey-MSFT> as FYI as well) — Reply to this email directly, view it on GitHub<#479 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AFJSI6EIS2YHY3N4SVHRC4DYJDA7ZAVCNFSM6AAAAABASC2II6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNJSHAYDMOBYGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

r12f · 2023-12-13T17:19:26Z

Since #480 is approved, which contains all changes in this PR. For keeping the changes clean, I am going to merge this PR first, rebase #480 , then merge #480 afterwards.

Move tables to use SaiTable annotation.

4d15328

r12f requested review from kcudnik, marian-pritsak and chrispsommers December 12, 2023 20:49

r12f added 2 commits December 12, 2023 20:55

spellcheck.

b8e1490

Update table sorting.

5630bb1

chrispsommers reviewed Dec 12, 2023

View reviewed changes

r12f added 3 commits December 12, 2023 23:02

Adding api-type for handling underlay tables.

2e14fa3

Removing todo.

e543ea4

fully remove the table name annotation parsing logic.

1105d4a

r12f mentioned this pull request Dec 13, 2023

[sai-gen] Deprecate name attribute for all match keys and action parameters, remove type guessing heuristics on object parent names. #480

Merged

r12f merged commit 01ac82e into sonic-net:main Dec 13, 2023
5 checks passed

r12f deleted the user/r12f/table branch December 13, 2023 17:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sai-gen] Deprecate the name annotations in favor of SaiTable #479

[sai-gen] Deprecate the name annotations in favor of SaiTable #479

r12f commented Dec 12, 2023 •

edited

Loading

r12f commented Dec 12, 2023

chrispsommers Dec 12, 2023

r12f Dec 12, 2023

r12f Dec 12, 2023

chrispsommers Dec 12, 2023

r12f Dec 12, 2023

r12f Dec 12, 2023

r12f Dec 12, 2023 •

edited

Loading

KrisNey-MSFT commented Dec 12, 2023 via email

r12f commented Dec 13, 2023

[sai-gen] Deprecate the name annotations in favor of SaiTable #479

[sai-gen] Deprecate the name annotations in favor of SaiTable #479

Conversation

r12f commented Dec 12, 2023 • edited Loading

r12f commented Dec 12, 2023

chrispsommers Dec 12, 2023

Choose a reason for hiding this comment

r12f Dec 12, 2023

Choose a reason for hiding this comment

r12f Dec 12, 2023

Choose a reason for hiding this comment

chrispsommers Dec 12, 2023

Choose a reason for hiding this comment

r12f Dec 12, 2023

Choose a reason for hiding this comment

r12f Dec 12, 2023

Choose a reason for hiding this comment

r12f Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

KrisNey-MSFT commented Dec 12, 2023 via email

r12f commented Dec 13, 2023

r12f commented Dec 12, 2023 •

edited

Loading

r12f Dec 12, 2023 •

edited

Loading