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 attributes to disable L3 rewrites #1924

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

vivekmoorthy
Copy link
Contributor

@vivekmoorthy vivekmoorthy commented Nov 8, 2023

Knobs for disabling rewrites to following fields

  • Src MAC disable
  • Dst MAC disable
  • Vlan rewrite disable

While we do L3 IP based routing, we have scenarios where we need knobs for disabling L2 field rewrites.

Case 1: For some scenarios, the switch serves as an L3 passthrough. Creating nexthop/RIF for every source mac, dst-mac, vlan will not scale, and we would like to simply passthrough the L2 fields without any rewrites
Case 2: For some scenarios, we set VLAN in pre-ingress stage based on certain classification, and would like to preserve this VLAN in the packet.

- Src MAC disable
- Dst MAC disable
- Vlan rewrite disable

Signed-off-by: Vivek Ramamoorthy <vivekmoorthy@google.com>
@kcudnik
Copy link
Collaborator

kcudnik commented Nov 9, 2023

please fix build errors

Signed-off-by: Vivek Ramamoorthy <vivekmoorthy@google.com>
@vivekmoorthy vivekmoorthy changed the title Add atrributes to disable L3 rewrites Add attributes to disable L3 rewrites Nov 9, 2023
@vivekmoorthy
Copy link
Contributor Author

please fix build errors

Done.

@vivekmoorthy vivekmoorthy reopened this Nov 9, 2023
@vivekmoorthy
Copy link
Contributor Author

please fix build errors

done

@vivekmoorthy
Copy link
Contributor Author

@kcudnik @mikeberesford could you review this PR

@mikeberesford
Copy link
Contributor

@vivekmoorthy we will need to bring this to the SAI community meeting next week. I can help coordinate

@rlhui
Copy link
Collaborator

rlhui commented Nov 16, 2023

Today in SAI pipeline for L3/routed packets, dst mac/src mac/vlan id are replaced in neighbor encap/egress rif block. We discussed in SAI Meeting if these knobs should be in neighbor or rif object instead. However, if disable L3 rewrite are only for certain IP flows and not for entire egress IP interface/neighbor, it'd still be better to stay at next hop object.

Copy link
Contributor

@srikrishnagopu srikrishnagopu left a comment

Choose a reason for hiding this comment

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

Moving it to request based on today's feedback.

@vivekmoorthy
Copy link
Contributor Author

Moving it to request based on today's feedback.

Updated link to document with requirements, proposal and example in description.

@vivekmoorthy
Copy link
Contributor Author

@JaiOCP @rlhui @srikrishnagopu @rck-innovium Pls review. I have added link document in description with example usage.

Copy link
Contributor

@JaiOCP JaiOCP left a comment

Choose a reason for hiding this comment

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

Looks good to me

Copy link
Contributor

@ashutosh-agrawal ashutosh-agrawal left a comment

Choose a reason for hiding this comment

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

LGTM

@rck-innovium
Copy link
Contributor

@vivekmoorthy  Please check in the document as MD file.

inc/sainexthop.h Show resolved Hide resolved
Copy link
Contributor

@srikrishnagopu srikrishnagopu left a comment

Choose a reason for hiding this comment

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

LGTM.

Signed-off-by: Vivek Ramamoorthy <vivekmoorthy@google.com>
@vivekmoorthy
Copy link
Contributor Author

@vivekmoorthy  Please check in the document as MD file.

Done.

@vivekmoorthy vivekmoorthy reopened this Dec 7, 2023
@vivekmoorthy
Copy link
Contributor Author

@rck-innovium @marian-pritsak could you pls review

@vivekmoorthy
Copy link
Contributor Author

@rlhui Based on discussion last week and with the approvals and comments addressed, could we merge this PR?

@rlhui
Copy link
Collaborator

rlhui commented Dec 19, 2023

@marian-pritsak , please help review/approve? Thanks.

@vivekmoorthy
Copy link
Contributor Author

@marian-pritsak , please help review/approve? Thanks.

@marian-pritsak could you please help with this?
Thanks.

@rlhui rlhui merged commit 0edf5f8 into opencomputeproject:master Jan 10, 2024
4 checks passed
saiarcot895 added a commit to saiarcot895/sonic-sairedis that referenced this pull request Jan 26, 2024
The primary purpose of this is to bring in support for compiling on
Debian Bookworm.

This brings in the following changes:
* Update the Doxyfile for doxygen in Debian Bookworm (opencomputeproject/SAI#1946)
* Enable sai_uint16_t in ProcessStructValueType Struct Member (opencomputeproject/SAI#1949)
* [meta] Add support for port stat extensions (opencomputeproject/SAI#1947)
* [meta] Add custom range start end values check (opencomputeproject/SAI#1945)
* Cable diagnostics attribute added (opencomputeproject/SAI#1894)
* Add attributes to disable L3 rewrites (opencomputeproject/SAI#1924)
* Add MAC remote loopback to the port loopback enums. (opencomputeproject/SAI#1934)
* [TAM] Granular counter subscription (opencomputeproject/SAI#1670)

Signed-off-by: Saikrishna Arcot <sarcot@microsoft.com>
JaiOCP pushed a commit to JaiOCP/SAI that referenced this pull request Mar 7, 2024
* Add atrributes to disable L3 rewrites

- Src MAC disable
- Dst MAC disable
- Vlan rewrite disable

Signed-off-by: Vivek Ramamoorthy <vivekmoorthy@google.com>
Knobs for disabling rewrites to following fields

Src MAC disable
Dst MAC disable
Vlan rewrite disable
While we do L3 IP based routing, we have scenarios where we need knobs for disabling L2 field rewrites.

Case 1: For some scenarios, the switch serves as an L3 passthrough. Creating nexthop/RIF for every source mac, dst-mac, vlan will not scale, and we would like to simply passthrough the L2 fields without any rewrites
Case 2: For some scenarios, we set VLAN in pre-ingress stage based on certain classification, and would like to preserve this VLAN in the packet.
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.

9 participants