[master] Smart Counter Poll to allow counters to work properly on Broadcom platforms. - #1774
Conversation
|
/azp run |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
WIP - adding unit test |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azpw retry |
|
Retrying failed(or canceled) jobs... |
|
Retrying failed(or canceled) stages in build 1156144: ✅Stage Build:
|
Aside from an unrelated regression, there are no notable regressions from running |
|
@justin-wong-ce - I re-reviewed the latest head. The earlier cleanup comments look mostly addressed and CI is green, but I still have two correctness questions before approval.
|
lolyu
left a comment
lolyu
left a comment
There was a problem hiding this comment.
Review: #1774 (re-review at 7627bd9)
Thanks for the thorough revision — I verified every change against the full source at this head, not just the diff. All nine findings across my earlier rounds are resolved or consciously deferred with good reason. Approving.
The two real bugs and the latent one — all fixed:
- Tombstone leak → free-list.
addGroup(1425) reusesm_freeGroupIndicesbefore appending;cleanupCounterGroupMappingreclaims the index on last-reference removal (1499). The vector no longer accumulates permanent empty-set tombstones across churn. 👍 m_failedPollsleak → fixed.removeObjectnowerase({rid, vid})before dropping the object (1514), so re-added ports get an honest DEBUG→ERROR ramp again.m_failedPollstier off-by-one → fixed. Nowuint32_t n = ++m_failedPolls[{rid, vid}]— a single lookup with cleann==1/n<=3/n==4tiers (the ERROR fires exactly once). The C++11++operator[]adaptation is functionally equivalent to thetry_emplaceI suggested.
Convention / duplication:
addGrouphelper now owns group creation at all five sites (index captured once, pre-push) — the four-spelling index inconsistency is now unwritable, and the sort-lambda is factored in.std::setcopies →const&(2008/2067/2080/2082); unqualifiedfind→std::find(2011).- The 4× erase-then-emplace block collapsed to a single
m_objectIdsMap[vid] = std::make_shared<...>at each site — cleaner than the helper I proposed. getSortedCGRefremoved entirely, which resolves the mutable-interior-pointer concern more completely than constifying it would have.
On the SWSS_LOG_ENTER build constraint: fair point, and well handled. Resolving the dedup/encapsulation items by inlining (removing the accessor, using operator[] directly) rather than adding helper functions is the right synthesis — it achieves the cleanup without paying the per-call logging overhead the CI check imposes. Noted and agreed.
One optional follow-up (non-blocking): the bulk-vs-single dispatch duplication you're leaving in place pending removal of the fallback path is a reasonable call — consider a // TODO: remove fallback path once addObjectWithCounterGroups is proven stable marker so that intent is visible in-tree to the next reader.
Nice work — the feature was sound to begin with, and the new code is now tight. LGTM.
|
Hi @justin-wong-ce for the cherrypick, could you test with 202605 and update in the description? |
I have ran the changes with 202511, 202605 sonic-mgmt is in progress - I expect more noise with that due to 202605 being newly branched. I have manually created a cherry-pick here: #1956 - I will post further updates directly there. |
|
202605 tests are in progress. Cherry pick will not be done until testing is complete with results. |
|
FYI @lolyu @vaibhavhd I have updated the PR description as well. |
|
Cherry-pick PR to 202605: #1995 |
…y on Broadcom platforms. (sonic-net#1774)" This reverts commit cda4337.
mssonicbld
added
Included in 202605 Branch
and removed
Created PR to 202605 Branch
labels
Approach What is the motivation for this PR? In #1774 , a new counter support discovery method is introduced for being able to read counter support for platforms where different ports can have different capabilities. The method worked well for some platforms, not so much for some others. This change adds an argument to syncd so there is the capability to toggle the per-port counter discovery. The argument will default to false so the legacy code path is used by default. Since there is now an option to toggle the code path on or off, there is no longer a need for a runtime fallback. Work item tracking Microsoft ADO (number only): How did you do it? Add an argument to syncd as well as the logic needed to read the argument in various files in syncd. Unit tests are also updated to account for the new argument, as well as mocking the use of this argument for unit tests that only test for per-port discovery logic. How did you verify/test it? Unit tests passes on both master and with the change casted to 202605. syncd is also able to boot on physical hardware and enter the expected code path based on whether the argument was provided for launch or not. Any platform specific information? Generic
…2001) Approach What is the motivation for this PR? In #1774 , a new counter support discovery method is introduced for being able to read counter support for platforms where different ports can have different capabilities. The method worked well for some platforms, not so much for some others. This change auto-uses the new syncd arg enablePerPortCounterDiscovery (introduced in #2000) when the broadcom ASIC type is detected in syncd_init_common.sh. Work item tracking Microsoft ADO (number only): How did you do it? Add logic in syncd_init_common.sh to auto apply the syncd arg when broadcom ASIC type is detected. How did you verify/test it? syncd unit tests pass on master and on 202605. On physical hardware, an image is built with the changes cherry-picked to 202605 and tests are ran in 2 scenarios: With the change as is syncd_init_common.sh hotpatched to filter on another asic type. In both scenarios, syncd enters the expected code path. Additionally, a subset of counters-focused sonic-mgmt tests are ran. There is no test result difference compared to tests done without this change. Tests were ran on Arista-7260CX3-D108C8 and Arista-7060X6-64PE-B-C512S2. The list of sonic-mgmt tests ran: test_pretest.py dhcp_relay/test_dhcp_counter_stress.py drop_packets/test_drop_counters.py drop_packets/test_configurable_drop_counters.py gnmi/test_gnmi_countersdb.py snmp/test_snmp_queue_counters.py test_posttest.py To ensure no warm and fast reboot issues from any potential time increase from syncd, some reboot-related tests are also ran. The following sonic-mgmt tests pass with this change casted, tested on Arista-7260CX3-D108C8 and Arista-7060CX-32S-C32: platform_tests/test_advanced_reboot.py::test_warm_reboot_sad* platform_tests/test_advanced_reboot.py::test_warm_reboot_mac_jump platform_tests/test_advanced_reboot.py::test_warm_reboot platform_tests/test_advanced_reboot.py::test_fast_reboot Any platform specific information? Broadcom
Approach What is the motivation for this PR? In #1774 , a new counter support discovery method is introduced for being able to read counter support for platforms where different ports can have different capabilities. The method worked well for some platforms, not so much for some others. This change applies the new syncd arg enablePerPortCounterDiscovery (introduced in #2000) when enable_per_port_counter_discovery is populated as true in SYNCD_VARS, which is retrieved from DEVICE_METADATA in CONFIG_DB. Work item tracking Microsoft ADO (number only): How did you do it? Add logic in syncd_init_common.sh to apply the syncd arg enablePerPortCounterDiscovery when it is defined as so in SYNCD_VARS. How did you verify/test it? syncd unit tests pass on master and on 202605. On physical hardware, an image is built with the changes cherry-picked to 202605 and tests are ran in 6 scenarios: 1): ASIC check: use arg on broadcom HWSKU config: not defined Result: per-port counter discovery code path used 2): ASIC check: use arg on broadcom HWSKU config: enable_per_port_counter_discovery set to true Result: per-port counter discovery code path used 3): ASIC check: use arg on broadcom HWSKU config: enable_per_port_counter_discovery set to false Result: legacy code path used 4): ASIC check: use arg on mellanox HWSKU config: not defined Result: legacy code path used 5): ASIC check: use arg on mellanox HWSKU config: enable_per_port_counter_discovery set to true Result: per-port counter discovery code path used 6): ASIC check: use arg on mellanox HWSKU config: enable_per_port_counter_discovery set to false Result: legacy code path used In all 6 scenarios, syncd enters the expected code path. Additionally, a subset of counters-focused sonic-mgmt tests are ran. There is no test result difference compared to tests done without this change. Tests were ran on Arista-7260CX3-D108C8. The list of sonic-mgmt tests ran: test_pretest.py dhcp_relay/test_dhcp_counter_stress.py drop_packets/test_drop_counters.py drop_packets/test_configurable_drop_counters.py gnmi/test_gnmi_countersdb.py snmp/test_snmp_queue_counters.py test_posttest.py Any platform specific information? Broadcom
5 participants
Summary
This is change is to address #1753 and the master implementation of the HLD added in sonic-net/SONiC#2190.
FlexCounter.cppassumes all ports support the same counter capabilities. This causes issues on most Broadcom platform switches as there are different types of ports on a switch that does not support the same set of counters.Fix by dynamically discovering what each interface is capable of during initialization of
syncd.For more detials please refer to the above issue and HLD.
Testing
Master testing is also done, but due to frequent changes in master tests fail on other issues depending which commit is used for testing. However, none of the issues seem related to this change.
More comprehensive testing is done on the 202511 stable branch:
Testing is done on both a
Arista-7060X6-16PE-384C-B-O128S2andArista-7260CX3-D108C8on 202511 with the tests:A full
sonic-mgmttest suite run has also been ran (202511). There are no notable fallout compared tosonic-mgmtruns without this change. This is ran on both XGS and DNX Broadcom platforms.EDIT: Jul 13, 2026
202605 Testing
Additional 202605 testing has been done.
Comparing full 202605
sonic-mgmttest results on TH5 (usingArista-7060X6-64PE) with and without these changes, there is no difference in the test results.Performance Impact:
All logic change is only done in the counter initialisation stage of FlexCounters.cpp - there is no polling logic change at all.
Therefore, any performance impact is limited to any new execution of
syncd- i.e. reboot /config reload/systemctl restart.Tested on several topologies:
This impact seems reasonable for what this offers.
The fastest operation that would cause a
syncdrestart is asystemctl restart swss, that is a operation that spans minutes. The worst real life scenario on a HwSKU with older CPU and many interfaces takes additional ~17 seconds. New, high interface-count HwSKUs only takes an extra ~2 seconds.As for memory usage - the impact is on the order of kilobytes, the impact is negligible as the system has GBs of RAM.