-
Notifications
You must be signed in to change notification settings - Fork 503
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
Use shared egress ACL table for PFCWD in BRCM DNX platform #3136
Conversation
Add @kenneth-arista @arlakshm @vmittal-msft for viz. |
Please add UT. |
Sai changes still now complete. These are sonic changes for this functionality change. PR to be merged after the SAI changes are integrated. |
19bb34b
to
936088c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks fine.
- Can you please share all the test run results to verify this functionality? Manual, sonic-mgmt, ixia tests etc
- Please share syslog message change due to this. current/new both.
- Please test with/without mac sec enabled ports.
- Also, please post results from pizzabox.
- Are these swss changes complete or expecting more after applying second patch from BRCM ?
- Please make sure to verify pfcwd drop counters to make sure they are still reporting drops.
Thanks
Vineet
Also, please add test cases to cover changes. |
Today PFCWD uses separate egress ACL tables, one table for each traffic class. This may result in ACL resource overflow when other features that also need egress ACL table is enabled at the same time, e.g., MACSEC. ( See sonic-net/sonic-buildimage#17839 for details). The fix is to use shared egress ACL table for PFCWD in BRCM DNX platform.
…o switch. Implement the requirement in aclorch.
603bd2d
to
f1535fc
Compare
The test added in this PR depends on sonic-net/sonic-buildimage#19108 |
We completed the testing with ixia, and shared egress acl worked fine without seeing any issue.
The change adds the following new syslog when binging eress ACL table to switch:
Yes, we tested PFCWD alone and also PFCWD + MACsec.
We will provide the result when the testing is done.
The swss change is complete.
Yes, the drop counters worked fine.
|
Hi @vmittal-msft, test was added. |
Hi @vmittal-msft , the test in this PR depends on sonic-net/sonic-buildimage#19108. |
@kperumalbfn , kindly review. @ysmanman , do you've plans to fix coverage? |
orchagent/aclorch.cpp
Outdated
if (platform == BRCM_PLATFORM_SUBSTRING && sub_platform == BRCM_DNX_PLATFORM_SUBSTRING && | ||
newTable.type.getName() == TABLE_TYPE_PFCWD) | ||
{ | ||
if(!bindEgrAclTableToSwitch(newTable)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of binding ACL table directly to switch, can we use ACL Table Group? If we use ACL Table, we could bind only one ACL to the switch bind point. ACL Table groups are used for port binding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of binding ACL table directly to switch, can we use ACL Table Group? If we use ACL Table, we could bind only one ACL to the switch bind point. ACL Table groups are used for port binding.
This was discussed in a previous meeting with both MSFT and BRCM. We understood the limitation, and decided to deferred this change later when we encounter the actual use case, i.e., we have multiple egress tables that need to bind to switch. @vmittal-msft
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. This is was discussed in BRCM meeting and decided to bind acl table instead of group since there no current use case for this. If need arise in future, we can enhance this and support.
orchagent/aclorch.cpp
Outdated
@@ -5112,3 +5168,48 @@ void AclOrch::removeAllAclRuleStatus() | |||
} | |||
} | |||
|
|||
// Bind egress ACL table (with bind type switch) to switch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SwitchOrch already has the code to bind ACLGroup to the switch.
sonic-swss/orchagent/switchorch.cpp
Line 300 in 78af15d
ReturnCode SwitchOrch::bindAclGroupToSwitch(const sai_acl_stage_t &group_stage, const referenced_object &acl_grp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
orchagent/aclorch.cpp
Outdated
} | ||
|
||
// Unbind egress ACL table from swtich | ||
bool AclOrch::unbindEgrAclTableFromSwitch(AclTable &table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as bind aclTable, please use ACL Group instead of ACLTable and switchorch already has the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@ysmanman please check comments and re-base. |
Hi @prsunny , I chatted with @vmittal-msft yesterday about the code coverage issue. The missing code coverage was due to existing code not providing any code coverage for |
ok, we can take that separately. waiting for reviewers to sign-off |
… handle both ingress and egress tables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…e in switchorch.cpp.
@ysmanman please re-base. |
…#3136) * Use shared egress ACL table for PFCWD in BRCM DNX platform. Today PFCWD uses separate egress ACL tables, one table for each traffic class. This may result in ACL resource overflow when other features that also need egress ACL table is enabled at the same time, e.g., MACSEC. ( See sonic-net/sonic-buildimage#17839 for details). The fix is to use shared egress ACL table for PFCWD in BRCM DNX platform.
Cherry-pick PR to 202405: #3250 |
* Use shared egress ACL table for PFCWD in BRCM DNX platform. Today PFCWD uses separate egress ACL tables, one table for each traffic class. This may result in ACL resource overflow when other features that also need egress ACL table is enabled at the same time, e.g., MACSEC. ( See sonic-net/sonic-buildimage#17839 for details). The fix is to use shared egress ACL table for PFCWD in BRCM DNX platform.
Today PFCWD uses separate egress ACL tables, one table for each traffic class. This may result in ACL resource overflow when other features that also need egress ACL table is enabled at the same time, e.g., MACSEC. ( See sonic-net/sonic-buildimage#17839 for details).
The fix is to use shared egress ACL table for PFCWD in BRCM DNX platform.
What I did
Why I did it
How I verified it
Details if related