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

[sonic-sairedis] Support bulk counter #1094

Merged
merged 5 commits into from
Aug 8, 2022

Conversation

Junchao-Mellanox
Copy link
Contributor

@Junchao-Mellanox Junchao-Mellanox commented Jul 28, 2022

Note: the build may fail due to SAI header dependency. Vendor SAI implementation shall include this PR: opencomputeproject/SAI#1352

HLD: https://github.com/sonic-net/SONiC/blob/master/doc/bulk_counter/bulk_counter.md

Why I did this?

PR https://github.com/opencomputeproject/SAI/pull/1352/files introduced new SAI APIs that supports bulk stats:

sai_bulk_object_get_stats
sai_bulk_object_clear_stats
SONiC flex counter infrastructure shall utilize bulk stats API to gain better performance. This document discusses how to integrate these two new APIs to SONiC.

What I did?

  1. Support using bulk stats APIs based on object type. E.g. for a counter group that queries queue and pg stats, queue stats support bulk while pg stats does not, in that case queue stats shall use bulk API, pg stats shall use non bulk API
  2. Automatically fall back to old way if bulk stats APIs are not supported

How I test this

Almost full unit test coverage
Manual test

@Junchao-Mellanox
Copy link
Contributor Author

Hi @kcudnik , could you please review this?

stephenxs
stephenxs previously approved these changes Jul 28, 2022
@Junchao-Mellanox
Copy link
Contributor Author

Hi @liat-grozovik , could you please help add kcudnik as reviewer?

@keboliu keboliu requested a review from kcudnik July 28, 2022 08:59
@Junchao-Mellanox
Copy link
Contributor Author

The coverage report does not really reflect the coverage:

Source File Diff Coverage (%) Missing Lines
lib/ClientSai.cpp 0.0% 986,997-999,1001,1003,1006,1016-1018,1020,1022
lib/ClientServerSai.cpp 0.0% 300,311-313,315,318,328-330,332
lib/RedisRemoteSaiInterface.cpp 0.0% 1214,1225,1227,1229,1232,1242,1244,1246
lib/Sai.cpp 0.0% 404,415,417,420,430,432
lib/sai_redis_interfacequery.cpp 0.0% 252,263,265,268,278,280
lib/ServerSai.cpp 0.0% 320,331,333,336,346,348
meta/Meta.cpp 0.0% 1853,1864,1866,1869,1879,1881
syncd/FlexCounter.cpp 95.6% 507,694,702-703
syncd/VendorSai.cpp 0.0% 649,660-662,664,673,676,686-688,690,698
vslib/Sai.cpp 0.0% 522,533-535,537,540,550-552,554
vslib/sai_vs_interfacequery.cpp 0.0% 205,216,218,221,231,233
vslib/VirtualSwitchSaiInterface.cpp 0.0% 964,975,977,980,990,992


My major change is on FlexCounter.cpp whose coverage is 95%. Changes to other files are only to have a default implementation for new SAI APIs to make compiler happy. I am not sure that it is necessary to cover them as it does not contains real logic.

@kcudnik
Copy link
Collaborator

kcudnik commented Jul 29, 2022

please address code coverage

kcudnik
kcudnik previously approved these changes Jul 29, 2022
@Junchao-Mellanox Junchao-Mellanox dismissed stale reviews from kcudnik and stephenxs via 36267a0 August 1, 2022 02:49
@liushilongbuaa
Copy link
Contributor

/azpw run

@liushilongbuaa
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kcudnik
Copy link
Collaborator

kcudnik commented Aug 1, 2022

please address code coverage

@Junchao-Mellanox
Copy link
Contributor Author

Hi @kcudnik , could you please review and sign-off?

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Junchao-Mellanox
Copy link
Contributor Author

/azpw run Azure.sonic-sairedis

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-sairedis

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liat-grozovik
Copy link
Collaborator

@kcudnik could you please help to merge?

@qiluo-msft qiluo-msft merged commit bfd37e3 into sonic-net:master Aug 8, 2022
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 9, 2022
Update sonic-sairedis submodule pointer to include the following:
* [sonic-sairedis] Support bulk counter ([sonic-net#1094](sonic-net/sonic-sairedis#1094))
* Transfer organization from Azure to sonic-net ([sonic-net#1095](sonic-net/sonic-sairedis#1095))
* [BFN] Provide unified approach to select P4 profile based on chip family ([sonic-net#1089](sonic-net/sonic-sairedis#1089))
* Add support of mdio IPC server class using sai switch api and unix socket ([sonic-net#1080](sonic-net/sonic-sairedis#1080))
* [FlexCounter] Refactor FlexCounter class  ([sonic-net#1073](sonic-net/sonic-sairedis#1073))

Signed-off-by: dprital <drorp@nvidia.com>
dprital added a commit to dprital/sonic-buildimage that referenced this pull request Aug 29, 2022
Update sonic-sairedis submodule pointer to include the following:
* Run 20 vs tests at a time. ([sonic-net#1111](sonic-net/sonic-sairedis#1111))
* [asan] suppress the static variable leaks ([sonic-net#1085](sonic-net/sonic-sairedis#1085))
* [sonic-sairedis] Support bulk counter ([sonic-net#1094](sonic-net/sonic-sairedis#1094))
* Transfer organization from Azure to sonic-net ([sonic-net#1095](sonic-net/sonic-sairedis#1095))
* [BFN] Provide unified approach to select P4 profile based on chip family ([sonic-net#1089](sonic-net/sonic-sairedis#1089))

Signed-off-by: dprital <drorp@nvidia.com>
mlorrillere added a commit to mlorrillere/sonic-buildimage that referenced this pull request Sep 8, 2022
* 660a920 [Chassis] Create fabric ports for switch_type fabric. (sonic-net/sonic-sairedis#1114)
* 8140c22 Fix issue: bulk counter feature cannot compile on platforms having no sai_bulk_object_get_stats/sai_bulk_object_clear_stats (sonic-net/sonic-sairedis#1105)
* 0aa60f5 [lgtm] Add uuid library (sonic-net/sonic-sairedis#1119)
* e8a01a8 Add retry on zmq functions if fail with EINTR. (sonic-net/sonic-sairedis#1109)
* 594b242 Add SAI_PORT_ATTR_OPER_SPEED support (sonic-net/sonic-sairedis#1107)
* 4c9e048 Add Xsight specific syncd start options (sonic-net/sonic-sairedis#1112)
* da26ace Run 20 vs tests at a time. (sonic-net/sonic-sairedis#1111)
* ffc4109 [asan] suppress the static variable leaks (sonic-net/sonic-sairedis#1085)
* bfd37e3 [sonic-sairedis] Support bulk counter (sonic-net/sonic-sairedis#1094)
* 90ba09a Transfer organization from Azure to sonic-net (sonic-net/sonic-sairedis#1095)
* 4853881 [BFN] Provide unified approach to select P4 profile based on chip family (sonic-net/sonic-sairedis#1089)
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
Note: the build may fail due to SAI header dependency. Vendor SAI implementation shall include this PR: opencomputeproject/SAI#1352

HLD: https://github.com/sonic-net/SONiC/blob/master/doc/bulk_counter/bulk_counter.md

**Why I did this?**

PR https://github.com/opencomputeproject/SAI/pull/1352/files introduced new SAI APIs that supports bulk stats:

sai_bulk_object_get_stats
sai_bulk_object_clear_stats
SONiC flex counter infrastructure shall utilize bulk stats API to gain better performance. This document discusses how to integrate these two new APIs to SONiC.

**What I did?**

1. Support using bulk stats APIs based on object type. E.g. for a counter group that queries queue and pg stats, queue stats support bulk while pg stats does not, in that case queue stats shall use bulk API, pg stats shall use non bulk API
2. Automatically fall back to old way if bulk stats APIs are not supported

**How I test this**

Almost full unit test coverage
Manual test
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.

7 participants