Expose the capability to config wred drop probability #571
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
(green, yellow, and red) Signed-off-by: Wenda <wenni@microsoft.com>
wendani
changed the title
wendani
changed the title
wendani
changed the title
yxieca
reviewed
Aug 10, 2018
orchagent/qosorch.cpp
Outdated
| attr.id = SAI_WRED_ATTR_YELLOW_DROP_PROBABILITY; | ||
| attr.value.s32 = 100; | ||
| attrs.push_back(attr); | ||
|
|
There was a problem hiding this comment.
This chunk of code looks like providing some default values for these settings. By removing them, you also removed the 'default' value.
I don't see other enforcement to make sure we pass down the configuration all the time. Maybe that belongs to a different PR?
If 'default' value is desirable, then you can create 2 attr objects for them, set the default value before going through the if - else lists. But don't add them until the whole if-else block is done. like following:
type attr_green_drop;
attr_green_drop.s32 = 100;
... ...
if ...
else if ...
...
attrs.push(attr_green_drops);
@lguohan do we need to do this?
There was a problem hiding this comment.
I see your other review. But we don't apply qos.json unless we have buffer configuration. So maybe default value is still needed. If we have default value, then the other PR is no longer needed. right?
There was a problem hiding this comment.
Hardware has a default value if you do not set. This is the case with RED_DROP_PROBABILITY, which is interestingly not set default here.
There was a problem hiding this comment.
I agree with @yxieca , the pr can be modified as follows. if the user does not provide drop probability, we should set the default value in swss to be 100. In this case, your other pr is not needed.
specified in qos.json Signed-off-by: Wenda <wenni@microsoft.com>
|
the vs check is broken. It used to succeed before retest. |
|
retest this please |
lguohan
approved these changes
Aug 16, 2018
|
retest this please |
1 similar comment
|
retest this please |
wendani
added a commit
to wendani/sonic-swss
that referenced
this pull request
Aug 18, 2018
* Add the capability to set wred drop probability of all packet colors (green, yellow, and red) Signed-off-by: Wenda <wenni@microsoft.com> * Ensure drop probability takes default value 100% if not explicitly specified in qos.json Signed-off-by: Wenda <wenni@microsoft.com>
lguohan
pushed a commit
that referenced
this pull request
Aug 18, 2018
…a queue (#584) * Expose the capability to config wred drop probability (#571) * Add the capability to set wred drop probability of all packet colors (green, yellow, and red) Signed-off-by: Wenda <wenni@microsoft.com> * Ensure drop probability takes default value 100% if not explicitly specified in qos.json Signed-off-by: Wenda <wenni@microsoft.com> * Use "[WRED_PROFILE|]" as a CLI signal to unbind wred profile from a queue or a list of queues; Add the parsing and processing logic in qosorch to support such an operation Signed-off-by: Wenda <wenni@microsoft.com> * Add print out for debugging purpose modified: orchagent/qosorch.cpp * Adjust log level Signed-off-by: Wenda <wenni@microsoft.com> * Change to use "[]" as a signal to unbind wred profile from a queue or a list of queues Signed-off-by: Wenda <wenni@microsoft.com> * Remove logs for debugging * Adjust code format * Address comments Signed-off-by: Wenda <wenni@microsoft.com>
EdenGri
pushed a commit
to EdenGri/sonic-swss
that referenced
this pull request
Feb 28, 2022
neethajohn
pushed a commit
that referenced
this pull request
Sep 14, 2022
…#2422) What I did Do not enforce drop probability for a color whose WRED is disabled. Signed-off-by: Stephen Sun stephens@nvidia.com Why I did it Currently, there is a logic to enforce the drop probability if it is not explicitly designated for a color. However, the drop probability is not a mandatory attribute. It can incur vendor SAI complaints to set it when the color is disabled. The logic was introduced from the very beginning (by PR #571) because no drop probability was defined in the QoS template at the time, which is no longer true. So we will enforce drop probability only if it is not configured and the color is enabled. How I verified it Unit test and manual test
stephenxs
added a commit
to stephenxs/sonic-swss
that referenced
this pull request
Sep 19, 2022
…sonic-net#2422) What I did Do not enforce drop probability for a color whose WRED is disabled. Signed-off-by: Stephen Sun stephens@nvidia.com Why I did it Currently, there is a logic to enforce the drop probability if it is not explicitly designated for a color. However, the drop probability is not a mandatory attribute. It can incur vendor SAI complaints to set it when the color is disabled. The logic was introduced from the very beginning (by PR sonic-net#571) because no drop probability was defined in the QoS template at the time, which is no longer true. So we will enforce drop probability only if it is not configured and the color is enabled. How I verified it Unit test and manual test
yxieca
pushed a commit
that referenced
this pull request
Sep 21, 2022
…#2422) What I did Do not enforce drop probability for a color whose WRED is disabled. Signed-off-by: Stephen Sun stephens@nvidia.com Why I did it Currently, there is a logic to enforce the drop probability if it is not explicitly designated for a color. However, the drop probability is not a mandatory attribute. It can incur vendor SAI complaints to set it when the color is disabled. The logic was introduced from the very beginning (by PR #571) because no drop probability was defined in the QoS template at the time, which is no longer true. So we will enforce drop probability only if it is not configured and the color is enabled. How I verified it Unit test and manual test
oleksandrivantsiv
pushed a commit
to oleksandrivantsiv/sonic-swss
that referenced
this pull request
Mar 1, 2023
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.
Expose the capability to config wred drop probability of all packet colors (green, yellow, and red)
Signed-off-by: Wenda wenni@microsoft.com
What I did
Why I did it
How I verified it
Tested on dut 3 cases:
Details if related