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.

Sign up for GitHub

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

Jump to bottom

Optimize counter initialization by reducing the number of bulk counter poll calls and communication between swss/sairedis #1527

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Optimize counter initialization by reducing the number of bulk counter poll calls and communication between swss/sairedis #1527

wants to merge 30 commits into from
+834 −196

Conversation

stephenxs
Copy link
Contributor

@stephenxs stephenxs commented Feb 9, 2025 •
edited
Loading

Optimize counter initialization by reducing the number of bulk counter poll calls and communication between swss/sairedis for the following counter groups

  • priority group watermark
  • priority group drop
  • queue watermark
  • queue stat
  • PFC watchdog

HLD sonic-net/SONiC#1862

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs stephenxs mentioned this pull request Feb 9, 2025
Merged
github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 9, 2025
View reviewed changes
syncd/FlexCounter.cpp Fixed Show resolved Hide resolved
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

github-advanced-security[bot]
github-advanced-security bot found potential problems Feb 9, 2025
View reviewed changes
syncd/FlexCounter.cpp
@@ -2618,6 +2819,171 @@
notifyPoll();
}

void FlexCounter::bulkAddCounter(

Check notice

Code scanning / CodeQL

Irregular enum initialization Note

In an enumerator list, the = construct should not be used to explicitly initialize members other than the first, unless all items are explicitly initialized.
@stephenxs
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs stephenxs mentioned this pull request Feb 9, 2025
Open
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs stephenxs requested review from kcudnik and keboliu February 10, 2025 08:53
@stephenxs stephenxs marked this pull request as ready for review February 13, 2025 11:59
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Copy link
Contributor Author

/azpw run

@mssonicbld
Copy link
Collaborator

/AzurePipelines run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Copy link
Contributor Author

the failure is not relevant to the commits

[ RUN      ] ClientServerSai.OUTBOUND_CA_TO_PA_ENTRY
Bad file descriptor (src/signaler.cpp:345)
/bin/bash: line 6: 25244 Aborted                 (core dumped) ${dir}$tst
FAIL: tests

kcudnik
kcudnik reviewed Feb 13, 2025
View reviewed changes
syncd/FlexCounterManager.cpp Outdated
SWSS_LOG_ENTER();

auto fc = getInstance(instanceId);
sai_object_type_t objectType = VidManager::objectTypeQuery(vids[0]); // VID and RID will have the same object type
Copy link
Collaborator

Choose a reason for hiding this comment

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

check if array is not empty and use .at()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to .at(0) but no need to check empty here.
it has already checked in the caller https://github.com/sonic-net/sonic-sairedis/pull/1527/files#diff-0f282bb4fdc70639e695cdafd322730eaa1c141b3b4e87ec0fda6e317f687017R2797

kcudnik
kcudnik reviewed Feb 13, 2025
View reviewed changes
syncd/Syncd.cpp
@@ -2791,49 +2791,95 @@ sai_status_t Syncd::processFlexCounterEvent(
}

auto groupName = key.substr(0, delimiter);
auto strVid = key.substr(delimiter + 1);
auto strVids = key.substr(delimiter + 1);
auto vidStringVector = swss::tokenize(strVids, ',');
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like this changes from single vid to multiple, will here be possible compatibility issue ? for example newer swss and older syncd that will expect 1 vid only and crash on deserialize when it will receive multiple ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but it doesn't look like a valid use case.
Swss depends on sairedis, which means sairedis can be newer but won't be older than swss.
What do you think?

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs
Fix compiling errors
4ea9f74 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Bulk counter init + bulk chunk size
c092ac2 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Support addBulkStatsContext using vector
4751d59 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Support test for bulk counter init
254eff2 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Optimize addBulkStatsContext
22737a9 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Insert entry with single OID as key into FLEX_COUNTER_DB
1f4796f 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Cover bulk counter initialization
df95629 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Remove non-used function queryObjectSupportedCounters
d6ac21d 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Remove unused code
2206451 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Minor changes
c124814 
1. add more comments
2. change function name
3. reduce log severity to DEBUG for counter polling to avoid rate limit

Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Simplify the logic
c7bc1b6 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix issue: fail to parse virtual ID vector which causes error message
6832104 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix alignment
e247996 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix issue: check bulk with empty OID list and add unit test case
9ca5b17 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Update log severity before and after bulk counter polling to info
9b76188 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Enhance unit test
8ddf7e5 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Handle corner case that partial counters are not supported to be poll…
ffb93e3 
…ed by bulk call

Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Enhance the unit test
7424742 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Add comments
66e2e61 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Move the syslog to avoid confusion
135200d 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Do not check bulk capability if it is configured for counter groups w…
6ba9c47 
…ithout plugin

Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix spelling check error and tail white spaces
139202d 
Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix issue and simplify code
9a4bb03 
add a map to simplify bulkAddCounter and addCounter
fix issue that bulkAddCounter isn't supported by attr counter

Signed-off-by: Stephen Sun <[email protected]>
@stephenxs
Fix review comments
fe9a9be 
Signed-off-by: Stephen Sun <[email protected]>
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@stephenxs stephenxs requested a review from kcudnik February 15, 2025 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keboliu keboliu keboliu approved these changes

@kcudnik kcudnik Awaiting requested review from kcudnik

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Cherry Pick Conflict_msft-202412 Request for msft-202412 Branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stephenxs @mssonicbld @kcudnik @keboliu @dgsudharsan @Junchao-Mellanox