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

Add support for VLAN Stacking/Translation API #1400

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for VLAN Stacking/Translation API #1400

wants to merge 1 commit into from

Conversation

kuanyu99
Copy link

@kuanyu99 kuanyu99 mentioned this pull request Jan 21, 2022
Open
@itaibaz
Copy link
Collaborator

itaibaz commented Jan 21, 2022

Should we also extend saiacl.h to allow pop / swap ?
We have SAI_ACL_ACTION_TYPE_ADD_VLAN_ID (push) and set outer/inner vlan ID (swap use case)
But I don't see remove_vlan (pop)

@pttch pttch mentioned this pull request Jan 26, 2022
Open
@rlhui
Copy link
Collaborator

rlhui commented Feb 24, 2022

Need to have a SAI specific doc for this. And also describe how this fits to SAI behavioral model.

@kuanyu99 kuanyu99 force-pushed the add_vlan_stack branch 2 times, most recently from a0fecbc to 40b5c0c Compare March 2, 2022 02:29
@kuanyu99
Copy link
Author

kuanyu99 commented Mar 2, 2022

Need to have a SAI specific doc for this. And also describe how this fits to SAI behavioral model.

I add a document about this new PR and also add a vlan stack stage into the pipeline.
I have some problem editing the vsdx file. So I use a PDF to represent the pipeline model about the vlan stacking.

@kcudnik
Copy link
Collaborator

kcudnik commented Mar 2, 2022

please fix errors:

WARNING: not expected number of spaces after * (1,4,8) saivlan.h 561:     *   outer vlan tag, the original one is an inner vlan tag.
WARNING: Long brief > 200 on SAI_VLAN_STACK_ATTR_MATCH_TYPE:
 - Match type The original Vlan ID should match to inner ta...

prvattem
prvattem reviewed Apr 28, 2022
View reviewed changes
inc/saivlan.h Outdated
SAI_VLAN_STACK_MATCH_TYPE_INNER,

SAI_VLAN_STACK_MATCH_TYPE_OUTER,

Copy link

Choose a reason for hiding this comment

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

Please enhance this to match multi-tag also. To start with a double-tag matching.
ex: SAI_VLAN_STACK_MATCH_TYPE_MULTI with stack depth 2?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, I don't fully understand the operation you mentioned.
Can you check if the current design is enough for your desire? Or can you give me some examples about your mentioned operation.

@kuanyu99 kuanyu99 force-pushed the add_vlan_stack branch 4 times, most recently from 35c95b5 to a6dbc76 Compare May 9, 2022 09:20
kcudnik
kcudnik reviewed May 13, 2022
View reviewed changes
kcudnik
kcudnik reviewed May 13, 2022
View reviewed changes
kcudnik
kcudnik reviewed May 13, 2022
View reviewed changes
kcudnik
kcudnik reviewed May 13, 2022
View reviewed changes
kcudnik
kcudnik reviewed May 13, 2022
View reviewed changes
kcudnik
kcudnik requested changes May 13, 2022
View reviewed changes
Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

please address comments

@chaoskao chaoskao mentioned this pull request Aug 10, 2022
Open
@kuanyu99
Copy link
Author

Hi @rlhui @ashutosh-agrawal @itaibaz @kcudnik ,
Can we merge this PR? Or what should I do to proceed it?

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 11, 2022

please wait for others to apprive

prvattem
prvattem reviewed Aug 12, 2022
View reviewed changes
srj102
srj102 suggested changes Aug 22, 2022
View reviewed changes
inc/saitypes.h Outdated
@@ -1335,6 +1350,9 @@ typedef union _sai_attribute_value_t

/** @validonly meta->attrvaluetype == SAI_ATTR_VALUE_TYPE_LATCH_STATUS */
sai_latch_status_t latchstatus;

/** @validonly meta->attrvaluetype == SAI_ATTR_VALUE_TYPE_VLAN_STACKING_VID */
sai_vlan_stacking_vid_t vlanstackingvid;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a new type ?
Can we not add more attributes instead for the inner and outer vlan. ?
Not adding a new type to the union helps avoid all the meta changes in SAI and also simplifies meta changes in sonic.

Copy link
Author

@kuanyu99 kuanyu99 Aug 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If considering this situation, I think change to use individual attributes can achieve the same thing.
Will change to use your listed example in next commit.

inc/saivlan.h Outdated
* @type sai_vlan_stacking_vid_t
* @flags MANDATORY_ON_CREATE | CREATE_ONLY
*/
SAI_VLAN_STACK_ATTR_ORIGINAL_VLAN_ID,
Copy link
Contributor

Choose a reason for hiding this comment

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

To avoid meta changes can we use the foll:

SAI_VLAN_STACK_ATTR_ORIGINAL_VLAN_ID_OUTER
SAI_VLAN_STACK_ATTR_ORIGINAL_VLAN_ID_INNER

Copy link
Author

Choose a reason for hiding this comment

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

Done. Please check.

inc/saivlan.h Outdated
@@ -657,7 +821,10 @@ typedef struct _sai_vlan_api_t
sai_get_vlan_stats_fn get_vlan_stats;
sai_get_vlan_stats_ext_fn get_vlan_stats_ext;
sai_clear_vlan_stats_fn clear_vlan_stats;

sai_create_vlan_stack_fn create_vlan_stack;
Copy link
Contributor

Choose a reason for hiding this comment

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

vlan stack/translation is a different stage in the pipeline.
It involves tag manipulations only.

Should we be modifying the sai_vlan_api_t ?

Instead Is it better to add a new API sai_vlan_stack_api_t and add
the vlan_stack related functions there ?

Copy link
Author

Choose a reason for hiding this comment

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

It is also an option. But if I do so, the sai_vlan_stack_api_t should be put in another header file, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes that would be another header file. However since we this is another stage in the pipeline it should be fine.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @srj102 ,
I have split the vlan stack into another header file. Please check if it helps.

@kuanyu99 kuanyu99 force-pushed the add_vlan_stack branch 3 times, most recently from 5e32dd9 to a70db3f Compare August 26, 2022 08:32
@wait2035 wait2035 mentioned this pull request Jun 19, 2023
Open
@kuanyu99
Add support for VLAN Stacking API
b3a84b9 
- Define SAI layer VLAN Stacking API in saivlanstack.h
  Able to support vlan translation and QinQ function
- Add document to descript the function of the vlan stack
- Add pipeline model for vlan stacking

Signed-off-by: kuanyu_chen <kuanyu_chen@edge-core.com>
@kuanyu99
Copy link
Author

Hi @ashutosh-agrawal @srj102 @prvattem
I have made some updates based on the comments recently.
Please check if anything needs to be corrected or can we let the PR be merged.

AnandhiDhanabalan
AnandhiDhanabalan reviewed Jul 27, 2023
View reviewed changes
inc/saivlanstack.h
* @flags CREATE_AND_SET
* @default 0xFF
*/
SAI_VLAN_STACK_ATTR_VLAN_APPLIED_PRI,
Copy link

Choose a reason for hiding this comment

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

Similar to _PRI, the CFI bit in vlan tag should be handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AnandhiDhanabalan AnandhiDhanabalan AnandhiDhanabalan left review comments

@kcudnik kcudnik kcudnik approved these changes

@ashutosh-agrawal ashutosh-agrawal Awaiting requested review from ashutosh-agrawal

@srj102 srj102 Awaiting requested review from srj102

@prvattem prvattem Awaiting requested review from prvattem

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

Successfully merging this pull request may close these issues.
8 participants
@kuanyu99 @itaibaz @rlhui @kcudnik @ashutosh-agrawal @srj102 @prvattem @AnandhiDhanabalan