[Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured #1822
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… configured Root cause: - In handlePortTableUpdate, refreshPgsFromPort is called if admin status is updated even if the speed is not configured. This is reasonable because the port can be configured as headroom override and the profile should be applied in that case. - However, the port's state is set to PORT_READY in refreshPgsForPort regardless whether speed is configured, which is not correct. This is should be avoided and PORT_READY should be set by caller if necessary Fix: - Don't set port's state to READY in refreshPgsForPort - Explicitly set port's state to READY in handlePortState. This has been done by all other callers. Signed-off-by: Stephen Sun <[email protected]>
Signed-off-by: Stephen Sun <[email protected]>
stephenxs
changed the title
neethajohn
approved these changes
Jul 23, 2021
liat-grozovik
approved these changes
Jul 29, 2021
qiluo-msft
pushed a commit
that referenced
this pull request
Jul 29, 2021
…ofile for active ports without speed configured (#1820) **What I did** Bugfix: Don't create lossless buffer profiles for active ports without speed configured This is to backport PR #1822 to 202012. Root cause: - In `handlePortTableUpdate`, `refreshPgsFromPort` is called if admin status is updated even if the speed is not configured. This is reasonable because the port can be configured as headroom override and the profile should be applied in that case. - However, as a side-effect, the port's state is set to `PORT_READY` in `refreshPgsForPort` regardless of whether the speed is configured, which is not correct. This is should be avoided and `PORT_READY` should be set by the caller if necessary Fix: - Don't set the port's state to `PORT_READY` in `refreshPgsForPort` and check the port's state before calling it. Note: - The change is covered by the existing vs tests. - The scenario where the bug was originally found can not be covered by vs test because: - The speed is always configured in vs image by default - Removing speed is not handled in buffer manager since it's not a valid flow. - A regression test will be opened to cover this case.
5 tasks
vaibhavhd
added a commit
to sonic-net/sonic-buildimage
that referenced
this pull request
Aug 3, 2021
Update sonic-swss submodule head to include below fixes: Ignore ALREADY_EXIST error in FDB creation (sonic-net/sonic-swss#1815) Update MACsec SA PN counter to support SAI API 1.8 (sonic-net/sonic-swss#1818) [swss]: Allow portsyncd to run on system without ports (sonic-net/sonic-swss#1808) [debugcounterorch] check if counter type is supported before querying… (sonic-net/sonic-swss#1789) [configure.ac] Add the option of passing libnl path to configure script (sonic-net/sonic-swss#1824) refactor(fdbsyncd): Convert files with dos2unix (sonic-net/sonic-swss#1828) [VS] Fix for VS test failures (sonic-net/sonic-swss#1836) Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830) [gearbox] Set context for phys based on configs (sonic-net/sonic-swss#1826) [Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1822) Bridge mac setting, fix statedb time format (sonic-net/sonic-swss#1844) [cfgmgr]: Introduce common libs. (sonic-net/sonic-swss#1842)
carl-nokia
pushed a commit
to carl-nokia/sonic-buildimage
that referenced
this pull request
Aug 7, 2021
…t#8313) Update sonic-swss submodule head to include below fixes: Ignore ALREADY_EXIST error in FDB creation (sonic-net/sonic-swss#1815) Update MACsec SA PN counter to support SAI API 1.8 (sonic-net/sonic-swss#1818) [swss]: Allow portsyncd to run on system without ports (sonic-net/sonic-swss#1808) [debugcounterorch] check if counter type is supported before querying… (sonic-net/sonic-swss#1789) [configure.ac] Add the option of passing libnl path to configure script (sonic-net/sonic-swss#1824) refactor(fdbsyncd): Convert files with dos2unix (sonic-net/sonic-swss#1828) [VS] Fix for VS test failures (sonic-net/sonic-swss#1836) Td2: Reclaim buffer from unused ports (sonic-net/sonic-swss#1830) [gearbox] Set context for phys based on configs (sonic-net/sonic-swss#1826) [Dynamic Buffer Calc] Bug fix: Don't create lossless buffer profile for active ports without speed configured (sonic-net/sonic-swss#1822) Bridge mac setting, fix statedb time format (sonic-net/sonic-swss#1844) [cfgmgr]: Introduce common libs. (sonic-net/sonic-swss#1842)
|
@stephenxs, not able to cherry-pick this PR in 202106 cleanly. Is there other depedent PR's ? Or please please create a separate PR for 202106. |
I’ll check it. Thanks |
Hi @judyjoseph |
judyjoseph
pushed a commit
that referenced
this pull request
Aug 18, 2021
…or active ports without speed configured (#1822) **What I did** Bugfix: Don't create lossless buffer profiles for active ports without speed configured Root cause: - In `handlePortTableUpdate`, `refreshPgsFromPort` is called if admin status is updated even if the speed is not configured. This is reasonable because the port can be configured as headroom override and the profile should be applied in that case. - However, as a side-effect, the port's state is set to `PORT_READY` in `refreshPgsForPort` regardless of whether the speed is configured, which is not correct. This should be avoided and `PORT_READY` should be set by the caller if necessary Fix: - Don't set the port's state to `PORT_READY` in `refreshPgsForPort` and check the port's state before calling it. - Explicitly set port's state to `PORT_READY` in `handlePortStateTable`. This has been done by all other callers. Note: - The change is covered by the existing vs tests. - The scenario where the bug was originally found can not be covered by vs test because: - The speed is always configured in vs image by default - Removing speed is not handled in buffer manager since it's not a valid flow. - A regression test will be opened to cover this case.
raphaelt-nvidia
pushed a commit
to raphaelt-nvidia/sonic-swss
that referenced
this pull request
Oct 5, 2021
…or active ports without speed configured (sonic-net#1822) **What I did** Bugfix: Don't create lossless buffer profiles for active ports without speed configured Root cause: - In `handlePortTableUpdate`, `refreshPgsFromPort` is called if admin status is updated even if the speed is not configured. This is reasonable because the port can be configured as headroom override and the profile should be applied in that case. - However, as a side-effect, the port's state is set to `PORT_READY` in `refreshPgsForPort` regardless of whether the speed is configured, which is not correct. This should be avoided and `PORT_READY` should be set by the caller if necessary Fix: - Don't set the port's state to `PORT_READY` in `refreshPgsForPort` and check the port's state before calling it. - Explicitly set port's state to `PORT_READY` in `handlePortStateTable`. This has been done by all other callers. Note: - The change is covered by the existing vs tests. - The scenario where the bug was originally found can not be covered by vs test because: - The speed is always configured in vs image by default - Removing speed is not handled in buffer manager since it's not a valid flow. - A regression test will be opened to cover this case.
EdenGri
pushed a commit
to EdenGri/sonic-swss
that referenced
this pull request
Feb 28, 2022
…uffer for unused ports (sonic-net#1822) Signed-off-by: Stephen Sun [email protected] What I did Db migrator support reclaiming reserved buffer for unused ports How I did it For admin down ports, if the buffer objects configuration aligns with default configuration, set the buffer objects configuration as: Dynamic model: all normal buffer objects are configured on admin down ports. Buffer manager will apply zero profiles on admin down ports. Static model: zero buffer objects are configured on admin down ports. How to verify it Unit test. Manually test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What I did
Bugfix: Don't create lossless buffer profiles for active ports without speed configured
Root cause:
handlePortTableUpdate,refreshPgsFromPortis called if admin status is updated even if the speed is not configured.This is reasonable because the port can be configured as headroom override and the profile should be applied in that case.
PORT_READYinrefreshPgsForPortregardless of whether the speed is configured, which is not correct.This should be avoided and
PORT_READYshould be set by the caller if necessaryFix:
PORT_READYinrefreshPgsForPortand check the port's state before calling it.PORT_READYinhandlePortStateTable. This has been done by all other callers.Note:
Signed-off-by: Stephen Sun [email protected]
Why I did it
How I verified it
Regression test and vs test.
Details if related