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

[Counters] Improve performance by polling only configured ports buffer queue/pg counters #2143

Merged
merged 18 commits into from
May 31, 2022
Merged

[Counters] Improve performance by polling only configured ports buffer queue/pg counters #2143

merged 18 commits into from
May 31, 2022

Conversation

shlomibitton
Copy link
Contributor

What I did

Currently in SONiC all ports queue and pg counters are created by default with the max possible amount of counters.
This feature change this behavior to poll only configured counters provided by the config DB BUFFER_PG and BUFFER_QUEUE tables.
If no tables are present in the DB, no counters will be created for ports.
Filter the unwanted queues/pgs returned by SAI API calls and skip the creation of these queue/pg counters.
Also allow creating/removing counters on runtime if buffer PG/Queue is configured or removed.

Why I did it

Improve performance by filtering unconfigured queue/pg counters on init.

How I verified it

Check after enabling the counters, if configured counters created in Counters DB according to the configurations.
Add/Remove buffer PG/Queue configurations and observe the corresponding counters created/removed accordingly.
New UT added to verify this flow.

Details if related

@shlomibitton shlomibitton requested a review from prsunny as a code owner February 9, 2022 13:21
@azure-pipelines
Copy link

Supported commands
  • help:
    • Get descriptions, examples and documentation about supported commands
    • Example: help "command_name"
  • list:
    • List all pipelines for this repository using a comment.
    • Example: "list"
  • run:
    • Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run.
    • Example: "run" or "run pipeline_name, pipeline_name, pipeline_name"
  • where:
    • Report back the Azure DevOps orgs that are related to this repository and org
    • Example: "where"

See additional documentation.

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2143 in repo Azure/sonic-swss

1 similar comment
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2143 in repo Azure/sonic-swss

@shlomibitton shlomibitton reopened this Feb 9, 2022
… init.

If no buffer configurations available, no counters will be created.
Allow creating/removing counters on runtime if buffer PG/Queue is created or removed.
New UT added to verify new flow.

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…omi_buffer_queues_pgs_counters_configurations_master
@mssonicbld
Copy link
Collaborator

/AzurePipelines run LGTM analysis: C/C++

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/AzurePipelines run LGTM

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss (Test vstest)

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@shlomibitton
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@stepanblyschak could you please review this?

@liat-grozovik
Copy link
Collaborator

/azp run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
@shlomibitton
Copy link
Contributor Author

/azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prsunny prsunny requested a review from neethajohn February 22, 2022 17:54
@shlomibitton shlomibitton marked this pull request as draft February 27, 2022 11:29
@shlomibitton shlomibitton marked this pull request as ready for review March 10, 2022 13:34
yxieca pushed a commit that referenced this pull request Jun 22, 2022
…r queue/pg counters (#2143)

- What I did
Currently in SONiC all ports queue and pg counters are created by default with the max possible amount of counters.
This feature change this behavior to poll only configured counters provided by the config DB BUFFER_PG and BUFFER_QUEUE tables.
If no tables are present in the DB, no counters will be created for ports.
Filter the unwanted queues/pgs returned by SAI API calls and skip the creation of these queue/pg counters.
Also allow creating/removing counters on runtime if buffer PG/Queue is configured or removed.

- Why I did it
Improve performance by filtering unconfigured queue/pg counters on init.

- How I verified it
Check after enabling the counters, if configured counters created in Counters DB according to the configurations.
Add/Remove buffer PG/Queue configurations and observe the corresponding counters created/removed accordingly.
New UT added to verify this flow.

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
yxieca added a commit that referenced this pull request Jun 23, 2022
…ts buffer queue/pg counters (#2143)"

This reverts commit 41614b8.
@yxieca
Copy link
Contributor

yxieca commented Jun 23, 2022

@shlomibitton I tried again, still having PR test failures with:

        "stdin": null, 
        "stdin_add_newline": true, 
        "strip_empty_ends": true, 
        "warn": true
    }
}, 
"msg": "non-zero return code", 
"rc": 1, 
"start": "2022-06-23 10:17:24.697107", 
"stderr": "Traceback (most recent call last):\n  File \"/usr/local/bin/watermarkstat\", line 315, in <module>\n    main()\n  File \"/usr/local/bin/watermarkstat\", line 310, in main\n    watermarkstat.print_all_stat(table_prefix, args.type)\n  File \"/usr/local/bin/watermarkstat\", line 261, in print_all_stat\n    data = self.get_counters(table_prefix,\n  File \"/usr/local/bin/watermarkstat\", line 237, in get_counters\n    elif fields[pos] != STATUS_NA:\nIndexError: list index out of range", 
"stderr_lines": [
    "Traceback (most recent call last):", 
    "  File \"/usr/local/bin/watermarkstat\", line 315, in <module>", 
    "    main()", 
    "  File \"/usr/local/bin/watermarkstat\", line 310, in main", 
    "    watermarkstat.print_all_stat(table_prefix, args.type)", 
    "  File \"/usr/local/bin/watermarkstat\", line 261, in print_all_stat", 
    "    data = self.get_counters(table_prefix,", 
    "  File \"/usr/local/bin/watermarkstat\", line 237, in get_counters", 
    "    elif fields[pos] != STATUS_NA:", 
    "IndexError: list index out of range"
], 
"stdout": "", 
"stdout_lines": []

}

@shlomibitton
Copy link
Contributor Author

shlomibitton commented Jun 23, 2022

@yxieca did you try it with the fix I provided for sonic-utilities?
sonic-net/sonic-utilities#2220

@yxieca
Copy link
Contributor

yxieca commented Jun 23, 2022

@yxieca did you try it with the fix I provided for sonic-utilities? Azure/sonic-utilities#2220

Ah, I see, I jumped the gun. The utility PR is still open.

neethajohn pushed a commit to sonic-net/sonic-utilities that referenced this pull request Jul 28, 2022
Signed-off-by: Nazarii Hnydyn nazariig@nvidia.com

Propagating #2220 with resolved review comments

What I did
Since PG counters are created only if they are configured in the switch, it is not enough to relay only on the first entry in the DB when building the output table of watermarkstat script.
We need to go over all configured counters, check what is the max configured, and build the table accordingly.

How I did it
Iterate all configured PG buffers for all ports and find the max index.
Build the output table according to the max index.

How to verify it
Run test "iface_namingmode/test_iface_namingmode.py" including this PR: sonic-net/sonic-swss#2143 and observe it passes.
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
…r queue/pg counters (sonic-net#2143)

- What I did
Currently in SONiC all ports queue and pg counters are created by default with the max possible amount of counters.
This feature change this behavior to poll only configured counters provided by the config DB BUFFER_PG and BUFFER_QUEUE tables.
If no tables are present in the DB, no counters will be created for ports.
Filter the unwanted queues/pgs returned by SAI API calls and skip the creation of these queue/pg counters.
Also allow creating/removing counters on runtime if buffer PG/Queue is configured or removed.

- Why I did it
Improve performance by filtering unconfigured queue/pg counters on init.

- How I verified it
Check after enabling the counters, if configured counters created in Counters DB according to the configurations.
Add/Remove buffer PG/Queue configurations and observe the corresponding counters created/removed accordingly.
New UT added to verify this flow.

Signed-off-by: Shlomi Bitton <shlomibi@nvidia.com>
preetham-singh pushed a commit to preetham-singh/sonic-swss that referenced this pull request Aug 6, 2022
yxieca pushed a commit to sonic-net/sonic-utilities that referenced this pull request Aug 8, 2022
Signed-off-by: Nazarii Hnydyn nazariig@nvidia.com

Propagating #2220 with resolved review comments

What I did
Since PG counters are created only if they are configured in the switch, it is not enough to relay only on the first entry in the DB when building the output table of watermarkstat script.
We need to go over all configured counters, check what is the max configured, and build the table accordingly.

How I did it
Iterate all configured PG buffers for all ports and find the max index.
Build the output table according to the max index.

How to verify it
Run test "iface_namingmode/test_iface_namingmode.py" including this PR: sonic-net/sonic-swss#2143 and observe it passes.
liat-grozovik pushed a commit that referenced this pull request Aug 25, 2022
…r queue/pg counters (#2360)

Propagating #2143 with resolved merge conflicts
Depends on: sonic-net/sonic-utilities#2239

- What I did
Currently in SONiC all ports queue and pg counters are created by default with the max possible amount of counters.
This feature change this behavior to poll only configured counters provided by the config DB BUFFER_PG and BUFFER_QUEUE tables.
If no tables are present in the DB, no counters will be created for ports.
Filter the unwanted queues/pgs returned by SAI API calls and skip the creation of these queue/pg counters.
Also allow creating/removing counters on runtime if buffer PG/Queue is configured or removed.

- Why I did it
Improve performance by filtering unconfigured queue/pg counters on init.

- How I verified it
Check after enabling the counters, if configured counters created in Counters DB according to the configurations.
Add/Remove buffer PG/Queue configurations and observe the corresponding counters created/removed accordingly.
New UT added to verify this flow.

Signed-off-by: Nazarii Hnydyn <nazariig@nvidia.com>
preetham-singh pushed a commit to preetham-singh/sonic-utilities that referenced this pull request Nov 21, 2022
…t#2239)

Signed-off-by: Nazarii Hnydyn nazariig@nvidia.com

Propagating sonic-net#2220 with resolved review comments

What I did
Since PG counters are created only if they are configured in the switch, it is not enough to relay only on the first entry in the DB when building the output table of watermarkstat script.
We need to go over all configured counters, check what is the max configured, and build the table accordingly.

How I did it
Iterate all configured PG buffers for all ports and find the max index.
Build the output table according to the max index.

How to verify it
Run test "iface_namingmode/test_iface_namingmode.py" including this PR: sonic-net/sonic-swss#2143 and observe it passes.
prabhataravind added a commit to prabhataravind/sonic-mgmt that referenced this pull request Aug 2, 2023
 * Check all queues for the test instead of just q0
 * Remove check for q7 counters until regression due to
   sonic-net/sonic-swss#2143 is fixed
 * Perform interface queue counter checks only once when there are v4 and v6
   neighbors over the same interface

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
What I did
Skip counters that are not enabled.

How to verify it
With change sonic-net/sonic-swss#2143, following commands will cause exception:

admin@vlab-01:~$ show priority-group persistent-watermark headroom
Traceback (most recent call last):
File "/usr/local/bin/watermarkstat", line 315, in
main()
File "/usr/local/bin/watermarkstat", line 310, in main
watermarkstat.print_all_stat(table_prefix, args.type)
File "/usr/local/bin/watermarkstat", line 261, in print_all_stat
data = self.get_counters(table_prefix,
File "/usr/local/bin/watermarkstat", line 237, in get_counters
elif fields[pos] != STATUS_NA:
IndexError: list index out of range

With the change:

admin@vlab-01:~$ show priority-group persistent-watermark headroom
Ingress headroom per PG:
Port
Ethernet0
Ethernet4
Ethernet8
Ethernet12
Ethernet16
... ...

Signed-off-by: Ying Xie ying.xie@microsoft.com
malletvapid23 added a commit to malletvapid23/Sonic-Utility that referenced this pull request Aug 3, 2023
Signed-off-by: Nazarii Hnydyn nazariig@nvidia.com

Propagating #2220 with resolved review comments

What I did
Since PG counters are created only if they are configured in the switch, it is not enough to relay only on the first entry in the DB when building the output table of watermarkstat script.
We need to go over all configured counters, check what is the max configured, and build the table accordingly.

How I did it
Iterate all configured PG buffers for all ports and find the max index.
Build the output table according to the max index.

How to verify it
Run test "iface_namingmode/test_iface_namingmode.py" including this PR: sonic-net/sonic-swss#2143 and observe it passes.
prabhataravind added a commit to sonic-net/sonic-mgmt that referenced this pull request Aug 7, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Sep 15, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Sep 16, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to mssonicbld/sonic-mgmt that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Nov 6, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
mssonicbld pushed a commit to sonic-net/sonic-mgmt that referenced this pull request Nov 10, 2023
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
AharonMalkin pushed a commit to AharonMalkin/sonic-mgmt that referenced this pull request Jan 25, 2024
* All control packets including BGP are expected to use Q7, separate
  from data packet queues
* Check for q7 counters is not being done until regression due to
  sonic-net/sonic-swss#2143 is fixed

Signed-off-by: Prabhat Aravind <paravind@microsoft.com>
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.

8 participants