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

Fixes QoS scheduler config on chips that do not support hierarchical scheduler #2674

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmukun
Copy link
Contributor

@dmukun dmukun commented Feb 21, 2023

What I did
This PR fixes QoS scheduler configuration failure on chips that do not support hierarchical scheduling

Why I did it
Scheduler configuration using scheduler_group SAI API fails on chips such as BCM56275 which does not support hierarchical scheduling. On these chips, scheduler_group SAI API returns error (SAI_STATUS_NOT_SUPPORTED) because it does not support L2 level scheduler configuration. On these chips, we need to use QUEUE SAI API instead to configure scheduling at the queue level.

How I verified it

  1. Verified scheduler configuration (config.txt) on switch based on BCM56275
  2. Verified deletion of scheduler configuration (config qos clear) on switch which has BCM56275
  3. Verified scheduler configuration (config.txt) on switch based on BCM56960
  4. Verified deletion of scheduler configuration (config qos clear) on switch which has BCM56960

Refer to UT-sairedis-log.txt

Details if related

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106
  • 202111
  • 202205
  • 202211

config.txt
UT-sairedis-log.txt

@dmukun
Copy link
Contributor Author

dmukun commented Mar 1, 2023

@prsunny, @bingwang-ms, @neethajohn, Can you help assign a reviewer to review this PR, please. This PR is useful to resolve scheduler configuration issues.

Copy link
Collaborator

@stephenxs stephenxs left a comment

Choose a reason for hiding this comment

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

Also please add unit test cases to cover the change.

@@ -1590,9 +1590,19 @@ bool QosOrch::applySchedulerToQueueSchedulerGroup(Port &port, size_t queue_ind,
attr.value.oid = scheduler_profile_id;

sai_status = sai_scheduler_group_api->set_scheduler_group_attribute(group_id, &attr);
if (SAI_STATUS_NOT_SUPPORTED != sai_status)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest using

        if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status)
                 || status ==  SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED)

Refer https://github.com/sonic-net/sonic-swss/blob/master/orchagent/switchorch.cpp#L678 as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for reviewing. This condition "SAI_STATUS_NOT_SUPPORTED != sai_status" was not right in the first place. Will fix this.

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.

2 participants