-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fix buffer configurations for 7170 #2972
Conversation
"pool":"[BUFFER_POOL|ingress_lossless_pool]", | ||
"size":"0", | ||
"static_th":"11075584" | ||
"size":"4096", |
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.
why do we change it so small?
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.
ingress lossless profile should have no limit.
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.
When threshold mode dynamic is configured, the above size value is ignored by the barefoot SDK. Buffers are carved out dynamically for the ingress buffers. Same comment applies for egress. So this size value should be ok.
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.
the size here is the reserved buffer for the pg, for lossless traffic, it should be zero.
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.
@zzhiyuan can you change size to 0 and give it a shot? I believe that should work with the SDE. You can reach out to me if you face any issues changing this value to 0
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.
I've changed all the lossless buffer sizes to 0. Thanks
need high level description of why such change. |
The previous buffer configuration was copied from a Broadcom platform. This configuration is for Barefoot, just like in: https://github.com/Azure/sonic-buildimage/blob/master/device/barefoot/x86_64-accton_wedge100bf_65x-r0/mavericks/buffers_defaults_t0.j2 |
the egress_lossless_profile does not seem to be correct. |
@lguohan for t1 qos config is missed in the current image. What actually wrong with egress_lossless_profile? |
@@ -10,37 +14,70 @@ | |||
{%- macro generate_buffer_pool_and_profiles() %} | |||
"BUFFER_POOL": { | |||
"ingress_lossless_pool": { | |||
"size": "33329088", | |||
"size": "{{ ingress_lossless_pool_size }}", |
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.
the ingress lossless pool size is changed from 32M -> 4M, why such change. I do not understand the rationale behind this change.
"size":"0", | ||
"static_th":"10587408" | ||
"size":"4096", | ||
"dynamic_th":"7" |
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.
we should avoid any egress packet drop for lossless traffic. previously configuration was to avoid any packet drop at egress. I do not think current configuration can guarantee no packet drop at egress for lossless traffic.
@NStetskovych , check my comments. |
@lguohan is it ok to merge now? |
Are there any additional problems with this PR? Thanks |
"pool":"[BUFFER_POOL|ingress_lossless_pool]", | ||
"size":"0", | ||
"static_th":"11075584" | ||
"dynamic_th":"0" |
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.
the dynamic threshold definating is alpha = 2^dynamic_th. If the dynamic_th is zero, then alpha is 1. it does not guarantee lossless behavior by SAI definition.
device/arista/x86_64-arista_7170_64c/Arista-7170-Q59S20/buffers_defaults_t0.j2
Outdated
Show resolved
Hide resolved
device/arista/x86_64-arista_7170_64c/Arista-7170-Q59S20/buffers_defaults_t1.j2
Outdated
Show resolved
Hide resolved
e790331
to
8466bfb
Compare
retest vs please |
retest vsimage please |
Hi Sunny, I don't have access to msft jenkins to retrigger the build. Can someone from msft do it please? Thanks |
retest vsimage please |
* Update sonic-utilities to master branch version sonic-utilities was (intentionally) pointing to a commit on a fork, since merging sonic-utilities's changes for Bookworm first onto the master branch would result in PR checker failures. Now that sonic-buildimage is on master branch and the Bookworm changes in sonic-utilities have been merged into master, sonic-utilties can now point to master. 17e77fe2 Revert "Run yang validation in unit test (#3025)" (#3055) 96dd5559 [dhcp_relay] Fix dhcp_relay counter display issue (#3054) 6dfeee69 [sflow][db_migrator] Egress Sflow support (#3020) 02a588b7 Don't collect /proc/sched_debug d7ec3251 Fix error about having a mutable default for field headers in dataclass 0ab3ab91 Fix test execution on Bookworm (#3041) ef8f6f83 Specify test dependencies under extra_requires 61c44e80 Update python packages 1e813105 [wol] Implement wol command line utility (#3048) 8ebc56a0 [sonic_installer]: Improve exception handling: introduce notes. (#3029) 3610ce93 [sonic-package-manager] Fix YANG validation failure on upgrade when feature has constraints in YANG model on FEATURE table (#2933) cfd2dd39 Add container rsyslog.conf to the sys dump (#3039) c4b07828 Support new platform in generic configuration update (#3038) a8d236c8 [fast-reboot-filter-routes.py] Remove click and improve error reporting (#3030) 75199c0f [sonic-package-manager] insert newline in /etc/sonic/generated_services.conf (#3040) cd855698 [VOQ][saidump] Modify generate_dump: replace save_saidump with save_saidump_by_route_size (#2972) f1e24ae5 GCU support for Cisco-8000 features (#3010) 67e1c3dc Update GCU rsyslog validator (#3012) 253b7975 [sonic-package-manager] do not modify config_db.json (#3032) 177dd8e8 [sonic-package-manager] add generated service to /etc/sonic/generated_services.conf (#3037) 62fcd77a Configure NTP according to extended configuration (#2835) ced09404 [dualtor_neighbor_check] Adjust zero-mac check condition (#3034) a4eeb698 [config] config reload should generate sysinfo if missing (#3031) e01fc891 Run yang validation in unit test (#3025) Signed-off-by: Saikrishna Arcot <[email protected]>
…atically (#17457) src/sonic-utilities * 1b1402f5 - (HEAD -> 202311, origin/202311) [hash]: Add ECMP/LAG hash algorithm CLI (#3036) (9 days ago) [Nazarii Hnydyn] * 71514ea3 - Revert "Run yang validation in unit test (#3025)" (#3055) (9 days ago) [Ying Xie] * b5daf5d4 - [dhcp_relay] Fix dhcp_relay counter display issue (#3054) (9 days ago) [Yaqiang Zhu] * b3172505 - [sflow][db_migrator] Egress Sflow support (#3020) (9 days ago) [Rajkumar-Marvell] * 1e813105 - [wol] Implement wol command line utility (#3048) (3 weeks ago) [Zhijian Li] * 8ebc56a0 - [sonic_installer]: Improve exception handling: introduce notes. (#3029) (3 weeks ago) [Nazarii Hnydyn] * 3610ce93 - [sonic-package-manager] Fix YANG validation failure on upgrade when feature has constraints in YANG model on FEATURE table (#2933) (3 weeks ago) [Stepan Blyshchak] * cfd2dd39 - Add container rsyslog.conf to the sys dump (#3039) (4 weeks ago) [Vivek] * c4b07828 - Support new platform in generic configuration update (#3038) (4 weeks ago) [Stephen Sun] * a8d236c8 - [fast-reboot-filter-routes.py] Remove click and improve error reporting (#3030) (4 weeks ago) [Stepan Blyshchak] * 75199c0f - [sonic-package-manager] insert newline in /etc/sonic/generated_services.conf (#3040) (4 weeks ago) [Stepan Blyshchak] * cd855698 - [VOQ][saidump] Modify generate_dump: replace save_saidump with save_saidump_by_route_size (#2972) (4 weeks ago) [JunhongMao] * f1e24ae5 - GCU support for Cisco-8000 features (#3010) (4 weeks ago) [rbpittman] * 67e1c3dc - Update GCU rsyslog validator (#3012) (4 weeks ago) [jingwenxie] * 253b7975 - [sonic-package-manager] do not modify config_db.json (#3032) (5 weeks ago) [Stepan Blyshchak] * 177dd8e8 - [sonic-package-manager] add generated service to /etc/sonic/generated_services.conf (#3037) (5 weeks ago) [Stepan Blyshchak] * 62fcd77a - Configure NTP according to extended configuration (#2835) (5 weeks ago) [Yevhen Fastiuk] * ced09404 - [dualtor_neighbor_check] Adjust zero-mac check condition (#3034) (5 weeks ago) [Longxiang Lyu] * a4eeb698 - [config] config reload should generate sysinfo if missing (#3031) (6 weeks ago) [jingwenxie] * e01fc891 - Run yang validation in unit test (#3025) (6 weeks ago) [ganglv]
…lly (#17501) #### Why I did it src/sonic-swss ``` * ff524e6d - (HEAD -> master, origin/master, origin/HEAD) [dash] add a retry for an ACL rule creation if a tag is not created yet (#2972) (7 hours ago) [Yakiv Huryk] * 620db3da - [ci] Allow partially success build artifact in PR checker pipeline. #2986 (3 days ago) [Liu Shilong] * d357e6f1 - [copporch] Add safeguard during policer attribute update (#2977) (4 days ago) [Vivek] * cb460394 - [fpmsyncd][WR] Relax the static schema constraint for ROUTE_TABLE (#2981) (5 days ago) [Vivek] * a1ce21f6 - Change base directory referenced in coverage.xml (#2976) (6 days ago) [Lawrence Lee] * 920959cf - [Dash] [UT] Add ZMQ test case for dash (#2967) (6 days ago) [Hua Liu] ``` #### How I did it #### How to verify it #### Description for the changelog
- What I did
Request from Barefoot to change the buffer configuration for 7170.
- How I did it
- How to verify it
Load 7170 with new image with this change. Interfaces are up.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)