From 6c6d403aa5c7858dc4859b649cd17dc1148393e4 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 19 Feb 2026 00:41:42 +0000 Subject: [PATCH 01/22] Add counter enhancement See HLD for details: https://github.com/sonic-net/SONiC/pull/2190 Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 557 ++++++++++++++++++++++++++++++++++++++++-- syncd/FlexCounter.h | 12 + 2 files changed, 554 insertions(+), 15 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 006b8d23d4..12f4526916 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -502,6 +502,41 @@ class CounterContext : public BaseCounterContext protected: sai_object_id_t m_switchId = SAI_NULL_OBJECT_ID; + struct CounterGroupRef + { + size_t idx; + size_t size; + }; + CounterGroupRef makeCounterGroupRef(uint64_t idx, uint64_t size) + { + SWSS_LOG_ENTER(); + CounterGroupRef ctx; + ctx.idx = idx; + ctx.size = size; + return ctx; + } + std::set* getSortedCGRef(CounterGroupRef const& cgr) + { + SWSS_LOG_ENTER(); + return &m_supportedCounterGroups[cgr.idx]; + } + static bool counterGroupRefDscSorter(CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + SWSS_LOG_ENTER(); + // Must adhere to strict weak ordering + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + } public: typedef CounterIds CounterIdsType; @@ -518,6 +553,125 @@ class CounterContext : public BaseCounterContext SWSS_LOG_ENTER(); } + virtual void addObjectWithCounterGroups( + _In_ sai_object_id_t vid, + _In_ sai_object_id_t rid, + _In_ const std::vector &idStrings, + _In_ const std::string &per_object_stats_mode) override + { + SWSS_LOG_ENTER(); + sai_stats_mode_t instance_stats_mode = SAI_STATS_MODE_READ_AND_CLEAR; + sai_stats_mode_t effective_stats_mode; + // TODO: use if const expression when c++17 is supported + if (HasStatsMode::value) + { + if (per_object_stats_mode == STATS_MODE_READ_AND_CLEAR) + { + instance_stats_mode = SAI_STATS_MODE_READ_AND_CLEAR; + } + else if (per_object_stats_mode == STATS_MODE_READ) + { + instance_stats_mode = SAI_STATS_MODE_READ; + } + else + { + SWSS_LOG_WARN("Stats mode %s not supported for flex counter. Using STATS_MODE_READ_AND_CLEAR", per_object_stats_mode.c_str()); + } + + effective_stats_mode = (m_groupStatsMode == SAI_STATS_MODE_READ_AND_CLEAR || + instance_stats_mode == SAI_STATS_MODE_READ_AND_CLEAR) ? SAI_STATS_MODE_READ_AND_CLEAR : SAI_STATS_MODE_READ; + } + else + { + effective_stats_mode = m_groupStatsMode; + } + + std::vector counter_ids; + for (const auto &str : idStrings) + { + StatType stat; + deserializeStat(str.c_str(), &stat); + counter_ids.push_back(stat); + } + + // Create base counter group if not yet instantiated + if (m_supportedCounterGroups.empty()) + { + m_supportedCounterGroups.push_back(std::set(counter_ids.begin(), counter_ids.end())); + m_counterGroupsSorted.push_back(makeCounterGroupRef(0, counter_ids.size())); + } + + updateSupportedCounterGroups(rid, vid, counter_ids, effective_stats_mode); + + size_t groupIndex = m_objectSupportedCountersGroupMap[vid]; + std::set& supportedIdsSet = m_supportedCounterGroups[groupIndex]; + if (supportedIdsSet.empty()) + { + return; + } + std::vector supportedIds(supportedIdsSet.begin(), supportedIdsSet.end()); + + if (double_confirm_supported_counters) + { + std::vector stats(supportedIds.size()); + + if (!collectData(rid, supportedIds, effective_stats_mode, false, stats)) + { + SWSS_LOG_ERROR("%s RID %s VID %s can't provide the statistic", m_name.c_str(), + sai_serialize_object_id(rid).c_str(), sai_serialize_object_id(vid).c_str()); + throw std::runtime_error("Test counter poll failed on populating m_objectIdsMap"); + } + } + + // Perform a remove and re-add to simplify the logic here + removeObject(vid, false); + + bool supportBulk; + // TODO: use if const expression when cpp17 is supported + if (HasStatsMode::value) + { + supportBulk = false; + } + else + { + supportBulk = no_double_check_bulk_capability || checkBulkCapability(vid, rid, supportedIds); + } + + if (!supportBulk) + { + auto counter_data = std::make_shared>(rid, supportedIds); + // TODO: use if const expression when cpp17 is supported + if (HasStatsMode::value) + { + counter_data->setStatsMode(instance_stats_mode); + } + m_objectIdsMap.emplace(vid, counter_data); + } + else if (m_counterChunkSizeMapFromPrefix.empty()) + { + std::sort(supportedIds.begin(), supportedIds.end()); + auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); + addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); + } + else + { + std::map> counter_prefix_map; + std::vector default_partition; + createCounterBulkChunkSizePerPrefixPartition(supportedIds, counter_prefix_map, default_partition); + + for (auto &counterPrefix : counter_prefix_map) + { + std::sort(counterPrefix.second.begin(), counterPrefix.second.end()); + auto bulkContext = getBulkStatsContext(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); + addBulkStatsContext(vid, rid, counterPrefix.second, *bulkContext.get()); + } + + std::sort(default_partition.begin(), default_partition.end()); + auto bulkContext = getBulkStatsContext(default_partition, "default", default_bulk_chunk_size); + addBulkStatsContext(vid, rid, default_partition, *bulkContext.get()); + } + } + // For those object type who support per object stats mode, e.g. buffer pool. virtual void addObject( _In_ sai_object_id_t vid, @@ -868,8 +1022,227 @@ class CounterContext : public BaseCounterContext } } + virtual void bulkAddObjectWithCounterGroups( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) + { + SWSS_LOG_ENTER(); + sai_stats_mode_t effective_stats_mode; + // TODO: use if const expression when c++17 is supported + if (HasStatsMode::value) + { + // Bulk operation is not supported by the counter group. + SWSS_LOG_INFO("Counter group %s %s does not support bulk. Fallback to single call", m_name.c_str(), m_instanceId.c_str()); + + // Fall back to old way + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + addObjectWithCounterGroups(vid, rid, idStrings, per_object_stats_mode); + } + + return; + } + else + { + effective_stats_mode = m_groupStatsMode; + } + + std::vector allCounterIds; + for (const auto &str : idStrings) + { + StatType stat; + deserializeStat(str.c_str(), &stat); + allCounterIds.push_back(stat); + } + + + // Create base counter group if not yet instantiated + if (m_supportedCounterGroups.empty()) + { + std::set fullCounterSet(allCounterIds.begin(), allCounterIds.end()); + m_supportedCounterGroups.push_back(fullCounterSet); + m_counterGroupsSorted.push_back(makeCounterGroupRef(0, allCounterIds.size())); + } + + for (size_t i = 0; i < vids.size(); i++) + { + updateSupportedCounterGroups(rids[i], vids[i], allCounterIds, effective_stats_mode); + } + + // Check if any counter group exists + if (m_objectSupportedCountersGroupMap.empty()) + { + SWSS_LOG_NOTICE("%s %s does not have supported counters", m_name.c_str(), m_instanceId.c_str()); + return; + } + + std::map, std::tuple> bulkUnsupportedCounters; + auto statsMode = m_groupStatsMode == SAI_STATS_MODE_READ ? SAI_STATS_MODE_BULK_READ : SAI_STATS_MODE_BULK_READ_AND_CLEAR; + auto checkAndUpdateBulkCapability = [&](const std::vector &counter_ids, const std::string &prefix, uint32_t bulk_chunk_size) + { + BulkContextType ctx; + // Check bulk capabilities again + std::vector supportedBulkIds; + sai_status_t status = SAI_STATUS_SUCCESS; + if (m_supportedBulkCounters.empty()) + { + status = querySupportedCounters(rids[0], statsMode, m_supportedBulkCounters); + } + if (status == SAI_STATUS_SUCCESS && !m_supportedBulkCounters.empty()) + { + for (auto stat : counter_ids) + { + if (m_supportedBulkCounters.count(stat) != 0) + { + supportedBulkIds.push_back(stat); + } + } + } + if (supportedBulkIds.size() < counter_ids.size()) + { + // Bulk polling is unsupported for the whole group but single polling is supported + // Add all objects to m_objectIdsMap so that they will be polled using single API + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + + size_t groupIndex = m_objectSupportedCountersGroupMap[vid]; + std::vector intf_counter_ids(m_supportedCounterGroups[groupIndex].begin(), m_supportedCounterGroups[groupIndex].end()); + if (intf_counter_ids.empty()) + { + continue; + } + std::vector stats(intf_counter_ids.size()); + if (!collectData(rid, intf_counter_ids, effective_stats_mode, false, stats)) + { + SWSS_LOG_ERROR("%s RID %s VID %s can't provide the statistic", m_name.c_str(), + sai_serialize_object_id(rid).c_str(), sai_serialize_object_id(vid).c_str()); + throw std::runtime_error("Test counter poll failed on populating m_objectIdsMap"); + } + auto it_vid = m_objectIdsMap.find(vid); + if (it_vid != m_objectIdsMap.end()) + { + // Remove and re-add if vid already exists + m_objectIdsMap.erase(it_vid); + } + + auto counter_data = std::make_shared>(rid, intf_counter_ids); + m_objectIdsMap.emplace(vid, counter_data); + SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); + } + return; + } + + ctx.counter_ids = counter_ids; + addBulkStatsContext(vids, rids, counter_ids, ctx); + status = m_vendorSai->bulkGetStats( + SAI_NULL_OBJECT_ID, + m_objectType, + static_cast(ctx.object_keys.size()), + ctx.object_keys.data(), + static_cast(ctx.counter_ids.size()), + reinterpret_cast(ctx.counter_ids.data()), + statsMode, + ctx.object_statuses.data(), + ctx.counters.data()); + if (status == SAI_STATUS_SUCCESS) + { + auto bulkContext = getBulkStatsContext(counter_ids, prefix, bulk_chunk_size); + addBulkStatsContext(vids, rids, counter_ids, *bulkContext.get()); + } + else + { + // Bulk is not supported for this counter prefix + // Append it to bulkUnsupportedCounters + std::tuple value(prefix, bulk_chunk_size); + bulkUnsupportedCounters.emplace(counter_ids, value); + SWSS_LOG_INFO("Counters starting with %s do not support bulk. Fallback to single call for these counters", prefix.c_str()); + } + }; + + // Use counter group with the most counters + std::vector supportedIds(getSortedCGRef(m_counterGroupsSorted[0])->begin(), getSortedCGRef(m_counterGroupsSorted[0])->end()); + + if (m_counterChunkSizeMapFromPrefix.empty()) + { + std::sort(supportedIds.begin(), supportedIds.end()); + checkAndUpdateBulkCapability(supportedIds, "default", default_bulk_chunk_size); + } + else + { + std::map> counter_prefix_map; + std::vector default_partition; + createCounterBulkChunkSizePerPrefixPartition(supportedIds, counter_prefix_map, default_partition); + + for (auto &counterPrefix : counter_prefix_map) + { + std::sort(counterPrefix.second.begin(), counterPrefix.second.end()); + } + + std::sort(default_partition.begin(), default_partition.end()); + + for (auto &counterPrefix : counter_prefix_map) + { + checkAndUpdateBulkCapability(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); + } + + checkAndUpdateBulkCapability(default_partition, "default", default_bulk_chunk_size); + } + + if (!bulkUnsupportedCounters.empty()) + { + SWSS_LOG_NOTICE("Partial counters do not support bulk. Re-check bulk capability for each object"); + + for (auto &it : bulkUnsupportedCounters) + { + std::vector bulkSupportedRIDs; + std::vector bulkSupportedVIDs; + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + std::vector stats(it.first.size()); + if (checkBulkCapability(vid, rid, it.first)) + { + bulkSupportedVIDs.push_back(vid); + bulkSupportedRIDs.push_back(rid); + } + else if (!double_confirm_supported_counters || collectData(rid, it.first, effective_stats_mode, false, stats)) + { + SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); + + auto it_vid = m_objectIdsMap.find(vid); + if (it_vid != m_objectIdsMap.end()) + { + // Remove and re-add if vid already exists + m_objectIdsMap.erase(it_vid); + } + + auto counter_data = std::make_shared>(rid, supportedIds); + m_objectIdsMap.emplace(vid, counter_data); + } + else + { + SWSS_LOG_WARN("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); + } + } + + if (!bulkSupportedVIDs.empty() && !bulkSupportedRIDs.empty()) + { + auto bulkContext = getBulkStatsContext(it.first, get<0>(it.second), get<1>(it.second)); + addBulkStatsContext(bulkSupportedVIDs, bulkSupportedRIDs, it.first, *bulkContext.get()); + } + } + } + } + virtual void bulkAddObject( - _In_ const std::vector& vids, + _In_ const std::vector& vids, _In_ const std::vector& rids, _In_ const std::vector& idStrings, _In_ const std::string &per_object_stats_mode) @@ -902,9 +1275,7 @@ class CounterContext : public BaseCounterContext { StatType stat; deserializeStat(str.c_str(), &stat); - { - allCounterIds.push_back(stat); - } + allCounterIds.push_back(stat); } updateSupportedCounters(rids[0]/*it is not really used*/, allCounterIds, effective_stats_mode); @@ -1105,6 +1476,7 @@ class CounterContext : public BaseCounterContext } } + virtual void collectData( _In_ swss::Table &countersTable) override { @@ -1123,9 +1495,10 @@ class CounterContext : public BaseCounterContext kv.second->getStatsMode() == SAI_STATS_MODE_READ_AND_CLEAR) ? SAI_STATS_MODE_READ_AND_CLEAR : SAI_STATS_MODE_READ; } - std::vector stats(statIds.size()); + std::vector stats(statIds.size(), 0); if (!collectData(rid, statIds, effective_stats_mode, true, stats)) { + SWSS_LOG_ERROR("counter read failed on RID 0x%x on intf 0x%x", rid, vid); continue; } @@ -1503,6 +1876,89 @@ class CounterContext : public BaseCounterContext return status == SAI_STATUS_SUCCESS; } + void updateSupportedCounterGroups( + _In_ sai_object_id_t rid, + _In_ sai_object_id_t vid, + _In_ const std::vector& counter_ids, + _In_ sai_stats_mode_t stats_mode) + { + SWSS_LOG_ENTER(); + if (m_objectSupportedCountersGroupMap.find(vid) == m_objectSupportedCountersGroupMap.end()) + { + // Check if a matching counter group already exists + for (size_t i = 0; i < m_counterGroupsSorted.size(); i++) + { + // Try match + std::set* counterSet = getSortedCGRef(m_counterGroupsSorted[i]); + std::vector values(counterSet->size(), 0); + std::vector countersToPoll(counterSet->begin(), counterSet->end()); + if (collectData(rid, countersToPoll, stats_mode, false, values)) + { + // Success - Check support for counters not in counter group (extra counters) + std::vector extraCounters; + std::set newCounters; + + // Counter groups is a subset of counter_ids in any situation + std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), + std::inserter(extraCounters, extraCounters.begin())); + for (const StatType &counter : extraCounters) + { + std::vector singleCounter {counter}; + std::vector singleValue(1); + if (collectData(rid, singleCounter, stats_mode, false, singleValue)) + { + newCounters.insert(counter); + } + else + { + SWSS_LOG_DEBUG("Counter %s not supported with rid %s, vid %s", + serializeStat(counter).c_str(), sai_serialize_object_id(rid).c_str(), + sai_serialize_object_id(vid).c_str()); + } + } + if (!newCounters.empty()) + { + // New counters discovered, create new counter group + newCounters.insert(counterSet->begin(), counterSet->end()); + m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + m_supportedCounterGroups.push_back(newCounters); + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + } + else + { + // Use existing counter group + m_objectSupportedCountersGroupMap.emplace(vid, m_counterGroupsSorted[i].idx); + } + return; + } + } + + // No counter groups matched, check counter support individually + std::set supportedIds; + std::vector values(1); + for (const auto &counter : counter_ids) + { + std::vector tmp_counter_ids {counter}; + if (collectData(rid, tmp_counter_ids, stats_mode, false, values)) + { + supportedIds.insert(counter); + } + } + + if (supportedIds.empty()) + { + return; + } + + // Make new counter group and assign the index if not assigned + m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + m_supportedCounterGroups.push_back(supportedIds); + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + } + } + void updateSupportedCounters( _In_ sai_object_id_t rid, _In_ const std::vector& counter_ids, @@ -1613,6 +2069,11 @@ class CounterContext : public BaseCounterContext sai_stats_mode_t& m_groupStatsMode; std::set m_supportedCounters; std::set m_supportedBulkCounters; + + std::map m_objectSupportedCountersGroupMap; + std::vector> m_supportedCounterGroups; + std::vector m_counterGroupsSorted; + std::map> m_objectIdsMap; std::map, std::shared_ptr> m_bulkContexts; }; @@ -1634,6 +2095,16 @@ class AttrContext : public CounterContext SWSS_LOG_ENTER(); } + // Wrapper to addObject + void addObjectWithCounterGroups( + _In_ sai_object_id_t vid, + _In_ sai_object_id_t rid, + _In_ const std::vector &idStrings, + _In_ const std::string &per_object_stats_mode) override + { + addObject(vid, rid, idStrings, per_object_stats_mode); + } + void addObject( _In_ sai_object_id_t vid, _In_ sai_object_id_t rid, @@ -1662,6 +2133,16 @@ class AttrContext : public CounterContext Base::m_objectIdsMap.emplace(vid, attr_ids); } + // Wrapper to bulkAddObject + void bulkAddObjectWithCounterGroups( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) override + { + bulkAddObject(vids, rids, idStrings, per_object_stats_mode); + } + void bulkAddObject( _In_ const std::vector& vids, _In_ const std::vector& rids, @@ -1735,6 +2216,15 @@ class DashMeterCounterContext : public BaseCounterContext SWSS_LOG_ENTER(); } + // Wrapper to addObject + void addObjectWithCounterGroups( + _In_ sai_object_id_t vid, + _In_ sai_object_id_t rid, + _In_ const std::vector &idStrings, + _In_ const std::string &per_object_stats_mode) override + { + addObject(vid, rid, idStrings, per_object_stats_mode); + } void addObject( _In_ sai_object_id_t vid, _In_ sai_object_id_t rid, @@ -1854,6 +2344,16 @@ class DashMeterCounterContext : public BaseCounterContext } } + // Wrapper to bulkAddObject + void bulkAddObjectWithCounterGroups( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) override + { + bulkAddObject(vids, rids, idStrings, per_object_stats_mode); + } + void bulkAddObject( _In_ const std::vector& vids, _In_ const std::vector& rids, @@ -2903,11 +3403,24 @@ void FlexCounter::bulkAddCounter( const auto &counterGroupRef = m_objectTypeField2CounterType.find({objectType, field}); if (counterGroupRef != m_objectTypeField2CounterType.end()) { - getCounterContext(counterGroupRef->second)->bulkAddObject( - vids, - rids, - idStrings, - ""); + try + { + getCounterContext(counterGroupRef->second)->bulkAddObjectWithCounterGroups( + vids, + rids, + idStrings, + ""); + } + catch (...) + { + SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + getCounterContext(counterGroupRef->second)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + } else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == BUFFER_POOL_COUNTER_ID_LIST) { @@ -2929,11 +3442,25 @@ void FlexCounter::bulkAddCounter( if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && counterIds.size()) { - getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( - vids, - rids, - counterIds, - statsMode); + try + { + getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObjectWithCounterGroups( + vids, + rids, + counterIds, + statsMode); + + } + catch (...) + { + SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( + vids, + rids, + counterIds, + statsMode); + + } } // notify thread to start polling diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index ab0f779547..24e750cedf 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -37,12 +37,24 @@ namespace syncd void removePlugins() {m_plugins.clear();} + virtual void addObjectWithCounterGroups( + _In_ sai_object_id_t vid, + _In_ sai_object_id_t rid, + _In_ const std::vector &idStrings, + _In_ const std::string &per_object_stats_mode) = 0; + virtual void addObject( _In_ sai_object_id_t vid, _In_ sai_object_id_t rid, _In_ const std::vector &idStrings, _In_ const std::string &per_object_stats_mode) = 0; + virtual void bulkAddObjectWithCounterGroups( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) = 0; + virtual void bulkAddObject( _In_ const std::vector& vids, _In_ const std::vector& rids, From 1a8fb2c0361d813a66635312d55f42356bf0e594 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 24 Feb 2026 21:49:11 +0000 Subject: [PATCH 02/22] Fix unittest failures Fixed code logic that does not account for a change in supported counters on an already initialized FlexCounter object. On an actual device, counter support does not change on an initialized FlexCounter object as it is hardware / SAI bound. Fixed mock function not returning a SAI_STATUS_FAILURE when a counter poll is supposed to fail. Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 130 +++++++++++++++-------------- unittest/syncd/TestFlexCounter.cpp | 7 +- 2 files changed, 75 insertions(+), 62 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 12f4526916..d0a50c07bf 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -600,6 +600,12 @@ class CounterContext : public BaseCounterContext m_supportedCounterGroups.push_back(std::set(counter_ids.begin(), counter_ids.end())); m_counterGroupsSorted.push_back(makeCounterGroupRef(0, counter_ids.size())); } + // Replace base counter group if full counter set differs + else if (m_supportedCounterGroups[0] != std::set(counter_ids.begin(), counter_ids.end())) + { + m_supportedCounterGroups[0] = std::set(counter_ids.begin(), counter_ids.end()); + m_counterGroupsSorted[0].size = counter_ids.size(); + } updateSupportedCounterGroups(rid, vid, counter_ids, effective_stats_mode); @@ -1063,10 +1069,15 @@ class CounterContext : public BaseCounterContext // Create base counter group if not yet instantiated if (m_supportedCounterGroups.empty()) { - std::set fullCounterSet(allCounterIds.begin(), allCounterIds.end()); - m_supportedCounterGroups.push_back(fullCounterSet); + m_supportedCounterGroups.push_back(std::set(allCounterIds.begin(), allCounterIds.end())); m_counterGroupsSorted.push_back(makeCounterGroupRef(0, allCounterIds.size())); } + // Ensure existing group is the same as new maximum group + else if (m_supportedCounterGroups[0] != std::set(allCounterIds.begin(), allCounterIds.end())) + { + m_supportedCounterGroups[0] = std::set(allCounterIds.begin(), allCounterIds.end()); + m_counterGroupsSorted[0].size = allCounterIds.size(); + } for (size_t i = 0; i < vids.size(); i++) { @@ -1883,80 +1894,77 @@ class CounterContext : public BaseCounterContext _In_ sai_stats_mode_t stats_mode) { SWSS_LOG_ENTER(); - if (m_objectSupportedCountersGroupMap.find(vid) == m_objectSupportedCountersGroupMap.end()) - { - // Check if a matching counter group already exists - for (size_t i = 0; i < m_counterGroupsSorted.size(); i++) - { - // Try match - std::set* counterSet = getSortedCGRef(m_counterGroupsSorted[i]); - std::vector values(counterSet->size(), 0); - std::vector countersToPoll(counterSet->begin(), counterSet->end()); - if (collectData(rid, countersToPoll, stats_mode, false, values)) + // Check if a matching counter group already exists + for (size_t i = 0; i < m_counterGroupsSorted.size(); i++) + { + // Try match + std::set* counterSet = getSortedCGRef(m_counterGroupsSorted[i]); + std::vector values(counterSet->size(), 0); + std::vector countersToPoll(counterSet->begin(), counterSet->end()); + if (collectData(rid, countersToPoll, stats_mode, false, values)) + { + // Success - Check support for counters not in counter group (extra counters) + std::vector extraCounters; + std::set newCounters; + + // Counter groups is a subset of counter_ids in any situation + std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), + std::inserter(extraCounters, extraCounters.begin())); + for (const StatType &counter : extraCounters) { - // Success - Check support for counters not in counter group (extra counters) - std::vector extraCounters; - std::set newCounters; - - // Counter groups is a subset of counter_ids in any situation - std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), - std::inserter(extraCounters, extraCounters.begin())); - for (const StatType &counter : extraCounters) + std::vector singleCounter {counter}; + std::vector singleValue(1); + if (collectData(rid, singleCounter, stats_mode, false, singleValue)) { - std::vector singleCounter {counter}; - std::vector singleValue(1); - if (collectData(rid, singleCounter, stats_mode, false, singleValue)) - { - newCounters.insert(counter); - } - else - { - SWSS_LOG_DEBUG("Counter %s not supported with rid %s, vid %s", - serializeStat(counter).c_str(), sai_serialize_object_id(rid).c_str(), - sai_serialize_object_id(vid).c_str()); - } - } - if (!newCounters.empty()) - { - // New counters discovered, create new counter group - newCounters.insert(counterSet->begin(), counterSet->end()); - m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); - m_supportedCounterGroups.push_back(newCounters); - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + newCounters.insert(counter); } else { - // Use existing counter group - m_objectSupportedCountersGroupMap.emplace(vid, m_counterGroupsSorted[i].idx); + SWSS_LOG_DEBUG("Counter %s not supported with rid %s, vid %s", + serializeStat(counter).c_str(), sai_serialize_object_id(rid).c_str(), + sai_serialize_object_id(vid).c_str()); } - return; } - } - - // No counter groups matched, check counter support individually - std::set supportedIds; - std::vector values(1); - for (const auto &counter : counter_ids) - { - std::vector tmp_counter_ids {counter}; - if (collectData(rid, tmp_counter_ids, stats_mode, false, values)) + if (!newCounters.empty()) { - supportedIds.insert(counter); + // New counters discovered, create new counter group + newCounters.insert(counterSet->begin(), counterSet->end()); + m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + m_supportedCounterGroups.push_back(newCounters); + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); } + else + { + // Use existing counter group + m_objectSupportedCountersGroupMap.emplace(vid, m_counterGroupsSorted[i].idx); + } + return; } + } - if (supportedIds.empty()) + // No counter groups matched, check counter support individually + std::set supportedIds; + std::vector values(1); + for (const auto &counter : counter_ids) + { + std::vector tmp_counter_ids {counter}; + if (collectData(rid, tmp_counter_ids, stats_mode, false, values)) { - return; + supportedIds.insert(counter); } + } - // Make new counter group and assign the index if not assigned - m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); - m_supportedCounterGroups.push_back(supportedIds); - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + if (supportedIds.empty()) + { + return; } + + // Make new counter group and assign the index if not assigned + m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + m_supportedCounterGroups.push_back(supportedIds); + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); } void updateSupportedCounters( diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 16baf94a69..aeec10b86e 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -751,9 +751,14 @@ TEST(FlexCounter, queryCounterCapability) } }; - sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { + sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, uint64_t *counters) { for (uint32_t i = 0; i < number_of_counters; i++) { + if (counter_ids[i] == SAI_PORT_STAT_IF_IN_UCAST_PKTS) + { + // Mocking failing poll of the SAI_PORT_STAT_IF_IN_UCAST_PKTS counter + return SAI_STATUS_FAILURE; + } counters[i] = 1000; } return SAI_STATUS_SUCCESS; From afbe56dd7c30b0090eec3307288fbef6b47010aa Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 24 Feb 2026 22:10:25 +0000 Subject: [PATCH 03/22] Fix tab indent Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index aeec10b86e..e7cdb9a2a3 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -756,7 +756,7 @@ TEST(FlexCounter, queryCounterCapability) { if (counter_ids[i] == SAI_PORT_STAT_IF_IN_UCAST_PKTS) { - // Mocking failing poll of the SAI_PORT_STAT_IF_IN_UCAST_PKTS counter + // Mocking failing poll of the SAI_PORT_STAT_IF_IN_UCAST_PKTS counter return SAI_STATUS_FAILURE; } counters[i] = 1000; From 87019b431752a92365d0a0b259d6e16ef2fae577 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 26 Feb 2026 02:06:47 +0000 Subject: [PATCH 04/22] Fix unit test failures, added more handling of counter groups - reverted previous test mock function change with getStats as it was intended - changed some ordering of test value checking as ordering of counters have changed - added logic to handle removal of counters from counter groups - added logic to cleanup stale counter groups - added logic to account for behavior when different global flags are set, i.e. (use_sai_stats_capa_query, dont_clear_support_counter, always_check_supported_counters) - moved large duplicated code blocks into helper functions Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 183 +++++++++++++++++++++++------ unittest/syncd/TestFlexCounter.cpp | 9 +- 2 files changed, 150 insertions(+), 42 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index d0a50c07bf..bef9aa7301 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -594,21 +594,14 @@ class CounterContext : public BaseCounterContext counter_ids.push_back(stat); } - // Create base counter group if not yet instantiated - if (m_supportedCounterGroups.empty()) - { - m_supportedCounterGroups.push_back(std::set(counter_ids.begin(), counter_ids.end())); - m_counterGroupsSorted.push_back(makeCounterGroupRef(0, counter_ids.size())); - } - // Replace base counter group if full counter set differs - else if (m_supportedCounterGroups[0] != std::set(counter_ids.begin(), counter_ids.end())) - { - m_supportedCounterGroups[0] = std::set(counter_ids.begin(), counter_ids.end()); - m_counterGroupsSorted[0].size = counter_ids.size(); - } - + std::set counter_ids_set = setupBaseCounterGroup(rid, counter_ids, effective_stats_mode); + counter_ids = std::vector(counter_ids_set.begin(), counter_ids_set.end()); updateSupportedCounterGroups(rid, vid, counter_ids, effective_stats_mode); + if (m_objectSupportedCountersGroupMap.count(vid) == 0) + { + return; // This vid has no supported counters + } size_t groupIndex = m_objectSupportedCountersGroupMap[vid]; std::set& supportedIdsSet = m_supportedCounterGroups[groupIndex]; if (supportedIdsSet.empty()) @@ -1065,20 +1058,8 @@ class CounterContext : public BaseCounterContext allCounterIds.push_back(stat); } - - // Create base counter group if not yet instantiated - if (m_supportedCounterGroups.empty()) - { - m_supportedCounterGroups.push_back(std::set(allCounterIds.begin(), allCounterIds.end())); - m_counterGroupsSorted.push_back(makeCounterGroupRef(0, allCounterIds.size())); - } - // Ensure existing group is the same as new maximum group - else if (m_supportedCounterGroups[0] != std::set(allCounterIds.begin(), allCounterIds.end())) - { - m_supportedCounterGroups[0] = std::set(allCounterIds.begin(), allCounterIds.end()); - m_counterGroupsSorted[0].size = allCounterIds.size(); - } - + std::set counter_ids_set = setupBaseCounterGroup(rids[0], allCounterIds, effective_stats_mode); + allCounterIds = std::vector(counter_ids_set.begin(), counter_ids_set.end()); for (size_t i = 0; i < vids.size(); i++) { updateSupportedCounterGroups(rids[i], vids[i], allCounterIds, effective_stats_mode); @@ -1465,6 +1446,45 @@ class CounterContext : public BaseCounterContext removeObject(vid, true); } + void cleanupCounterGroupMapping( + _In_ sai_object_id_t vid) + { + SWSS_LOG_ENTER(); + + auto groupIter = m_objectSupportedCountersGroupMap.find(vid); + if (groupIter == m_objectSupportedCountersGroupMap.end()) + { + return; + } + + size_t removedGroupIdx = groupIter->second; + m_objectSupportedCountersGroupMap.erase(groupIter); + + // Check if any other vid still references this group + bool referenced = false; + for (const auto& pair: m_objectSupportedCountersGroupMap) + { + if (pair.second == removedGroupIdx) + { + referenced = true; + break; + } + } + + if (!referenced) + { + // Remove from sorted list so updateSupportedCounterGroups won't match + m_counterGroupsSorted.erase( + std::remove_if(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [removedGroupIdx](const CounterGroupRef& ref) { return ref.idx == removedGroupIdx; }), + m_counterGroupsSorted.end()); + + // Clear the group data (can't erase from vector without invalidating other indices) + m_supportedCounterGroups[removedGroupIdx].clear(); + } + } + + void removeObject( _In_ sai_object_id_t vid, _In_ bool log) @@ -1485,6 +1505,7 @@ class CounterContext : public BaseCounterContext sai_serialize_object_type(m_objectType).c_str(), sai_serialize_object_id(vid).c_str()); } + cleanupCounterGroupMapping(vid); } @@ -1569,6 +1590,59 @@ class CounterContext : public BaseCounterContext } private: + std::set setupBaseCounterGroup( + _In_ sai_object_id_t rid, + _In_ const std::vector counter_ids, + _In_ sai_stats_mode_t &stats_mode) + { + SWSS_LOG_ENTER(); + // Query supported counters if flag set + std::set counter_ids_set; + if (!use_sai_stats_capa_query || querySupportedCounters(rid, stats_mode, counter_ids_set) != SAI_STATUS_SUCCESS) + { + // Query disabled/failed, use supplied set + counter_ids_set = std::set(counter_ids.begin(), counter_ids.end()); + } + else + { + // Query suceceeded, intersect with supplied set to no query uninteneded counters + std::set originalSet(counter_ids.begin(), counter_ids.end()); + std::set intersected; + std::set_intersection(counter_ids_set.begin(), counter_ids_set.end(), + originalSet.begin(), originalSet.end(), + std::inserter(intersected, intersected.begin())); + counter_ids_set = intersected; + } + + // Create base counter group if not yet instantiated + if (m_supportedCounterGroups.empty()) + { + m_supportedCounterGroups.push_back(counter_ids_set); + m_counterGroupsSorted.push_back(makeCounterGroupRef(0, counter_ids_set.size())); + } + // Replace base counter group if full counter set differs + else + { + bool groupExists = false; + for (size_t i = 0; i < m_supportedCounterGroups.size(); i++) + { + if (m_supportedCounterGroups[i] == counter_ids_set) + { + groupExists = true; + break; + } + } + if (!groupExists) + { + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size(), counter_ids_set.size())); + m_supportedCounterGroups.push_back(counter_ids_set); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + } + } + return counter_ids_set; + } + + bool isCounterSupported( _In_ StatType counter) const { @@ -1894,6 +1968,12 @@ class CounterContext : public BaseCounterContext _In_ sai_stats_mode_t stats_mode) { SWSS_LOG_ENTER(); + if (m_objectSupportedCountersGroupMap.count(vid) && !always_check_supported_counters) + { + SWSS_LOG_NOTICE("Ignore checking of supported counters"); + return; + } + // Check if a matching counter group already exists for (size_t i = 0; i < m_counterGroupsSorted.size(); i++) { @@ -1929,7 +2009,15 @@ class CounterContext : public BaseCounterContext { // New counters discovered, create new counter group newCounters.insert(counterSet->begin(), counterSet->end()); - m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + + // If vid already has assigned counter group, merge the two groups if dont_clear flag is set + if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter) + { + std::set oldGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + newCounters.insert(oldGroup.begin(), oldGroup.end()); + } + + m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(newCounters); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); @@ -1937,7 +2025,21 @@ class CounterContext : public BaseCounterContext else { // Use existing counter group - m_objectSupportedCountersGroupMap.emplace(vid, m_counterGroupsSorted[i].idx); + // If vid already has assigned counter group, merge the two groups if dont_clear flag is set + if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter && + m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]] != m_supportedCounterGroups[m_counterGroupsSorted[i].idx]) + { + std::set oldGroup = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; + newCounters.insert(oldGroup.begin(), oldGroup.end()); + m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); + m_supportedCounterGroups.push_back(newCounters); + m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + } + else + { + m_objectSupportedCountersGroupMap[vid] = m_counterGroupsSorted[i].idx; + } } return; } @@ -1961,7 +2063,7 @@ class CounterContext : public BaseCounterContext } // Make new counter group and assign the index if not assigned - m_objectSupportedCountersGroupMap.emplace(vid, m_supportedCounterGroups.size()); + m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(supportedIds); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); @@ -3350,11 +3452,22 @@ void FlexCounter::addCounter( const auto &counterGroupRef = m_objectTypeField2CounterType.find({objectType, field}); if (counterGroupRef != m_objectTypeField2CounterType.end()) { - getCounterContext(counterGroupRef->second)->addObject( - vid, - rid, - idStrings, - ""); + try { + getCounterContext(counterGroupRef->second)->addObjectWithCounterGroups( + vid, + rid, + idStrings, + ""); + + } + catch (...) { + SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + getCounterContext(counterGroupRef->second)->addObject( + vid, + rid, + idStrings, + ""); + } } else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == BUFFER_POOL_COUNTER_ID_LIST) { diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index e7cdb9a2a3..b54bc7c399 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -754,11 +754,6 @@ TEST(FlexCounter, queryCounterCapability) sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, uint64_t *counters) { for (uint32_t i = 0; i < number_of_counters; i++) { - if (counter_ids[i] == SAI_PORT_STAT_IF_IN_UCAST_PKTS) - { - // Mocking failing poll of the SAI_PORT_STAT_IF_IN_UCAST_PKTS counter - return SAI_STATUS_FAILURE; - } counters[i] = 1000; } return SAI_STATUS_SUCCESS; @@ -1748,13 +1743,13 @@ TEST(FlexCounter, counterIdChange) // support bulk to not support bulk values.clear(); - values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_NON_UCAST_PKTS,SAI_PORT_STAT_IF_IN_UCAST_PKTS"); + values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_UCAST_PKTS,SAI_PORT_STAT_IF_IN_NON_UCAST_PKTS"); fc.addCounter(oid, oid, values); usleep(1000*1050); counterVerifyFunc(countersTable, expectedKey, - {"SAI_PORT_STAT_IF_IN_NON_UCAST_PKTS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS"}, + {"SAI_PORT_STAT_IF_IN_UCAST_PKTS","SAI_PORT_STAT_IF_IN_NON_UCAST_PKTS"}, {"10", "20"}); // not support bulk but counter id changes From d7ed3e60b9086269f9e8e3bc509e562310f5e66e Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 26 Feb 2026 02:09:46 +0000 Subject: [PATCH 05/22] Fix typo Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index bef9aa7301..0e38ed3efd 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1605,7 +1605,7 @@ class CounterContext : public BaseCounterContext } else { - // Query suceceeded, intersect with supplied set to no query uninteneded counters + // Query succeeded, intersect with supplied set to no query uninteneded counters std::set originalSet(counter_ids.begin(), counter_ids.end()); std::set intersected; std::set_intersection(counter_ids_set.begin(), counter_ids_set.end(), From 5e00e57350a385ec4d7430e56592d94a2ef32d7b Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 26 Feb 2026 03:34:55 +0000 Subject: [PATCH 06/22] Fix more typos Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 0e38ed3efd..488c15dd06 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1605,7 +1605,7 @@ class CounterContext : public BaseCounterContext } else { - // Query succeeded, intersect with supplied set to no query uninteneded counters + // Query succeeded, intersect with supplied set to no query unintended counters std::set originalSet(counter_ids.begin(), counter_ids.end()); std::set intersected; std::set_intersection(counter_ids_set.begin(), counter_ids_set.end(), @@ -3461,7 +3461,7 @@ void FlexCounter::addCounter( } catch (...) { - SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); getCounterContext(counterGroupRef->second)->addObject( vid, rid, @@ -3534,7 +3534,7 @@ void FlexCounter::bulkAddCounter( } catch (...) { - SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); getCounterContext(counterGroupRef->second)->bulkAddObject( vids, rids, @@ -3574,7 +3574,7 @@ void FlexCounter::bulkAddCounter( } catch (...) { - SWSS_LOG_WARN("Error occured initlizating SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( vids, rids, From 6d71a1d8619c663872d56080c29e2abdd0401a2a Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 26 Feb 2026 03:39:22 +0000 Subject: [PATCH 07/22] Fix comment wording Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 488c15dd06..48786785a9 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1605,7 +1605,7 @@ class CounterContext : public BaseCounterContext } else { - // Query succeeded, intersect with supplied set to no query unintended counters + // Query succeeded, intersect with supplied set to not query unintended counters std::set originalSet(counter_ids.begin(), counter_ids.end()); std::set intersected; std::set_intersection(counter_ids_set.begin(), counter_ids_set.end(), From 1e1950ec5891346c416c3b6f0b5c87c8989a20b8 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 26 Feb 2026 19:45:11 +0000 Subject: [PATCH 08/22] Fix type conversion issues with armhf builds Change makeCounterGroupRef args to use size_t instead of uint64_t Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 48786785a9..ee98fedaff 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -507,7 +507,7 @@ class CounterContext : public BaseCounterContext size_t idx; size_t size; }; - CounterGroupRef makeCounterGroupRef(uint64_t idx, uint64_t size) + CounterGroupRef makeCounterGroupRef(size_t idx, size_t size) { SWSS_LOG_ENTER(); CounterGroupRef ctx; From a27f2e1e6c68a83348fa9ad8080165a593cbb86f Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Mon, 20 Apr 2026 17:35:12 +0000 Subject: [PATCH 09/22] Add additional logic checks and sorting for robustness Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 67 ++++++++++++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 16 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index ee98fedaff..6d76d1ecc7 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -510,10 +510,7 @@ class CounterContext : public BaseCounterContext CounterGroupRef makeCounterGroupRef(size_t idx, size_t size) { SWSS_LOG_ENTER(); - CounterGroupRef ctx; - ctx.idx = idx; - ctx.size = size; - return ctx; + return {idx, size}; } std::set* getSortedCGRef(CounterGroupRef const& cgr) { @@ -596,6 +593,7 @@ class CounterContext : public BaseCounterContext std::set counter_ids_set = setupBaseCounterGroup(rid, counter_ids, effective_stats_mode); counter_ids = std::vector(counter_ids_set.begin(), counter_ids_set.end()); + std::sort(counter_ids.begin(), counter_ids.end()); updateSupportedCounterGroups(rid, vid, counter_ids, effective_stats_mode); if (m_objectSupportedCountersGroupMap.count(vid) == 0) @@ -622,7 +620,7 @@ class CounterContext : public BaseCounterContext } } - // Perform a remove and re-add to simplify the logic here + // Perform a remove and re-add to simplify the logic here removeObject(vid, false); bool supportBulk; @@ -1025,7 +1023,7 @@ class CounterContext : public BaseCounterContext _In_ const std::vector& vids, _In_ const std::vector& rids, _In_ const std::vector& idStrings, - _In_ const std::string &per_object_stats_mode) + _In_ const std::string &per_object_stats_mode) override { SWSS_LOG_ENTER(); sai_stats_mode_t effective_stats_mode; @@ -1060,6 +1058,7 @@ class CounterContext : public BaseCounterContext std::set counter_ids_set = setupBaseCounterGroup(rids[0], allCounterIds, effective_stats_mode); allCounterIds = std::vector(counter_ids_set.begin(), counter_ids_set.end()); + std::sort(allCounterIds.begin(), allCounterIds.end()); for (size_t i = 0; i < vids.size(); i++) { updateSupportedCounterGroups(rids[i], vids[i], allCounterIds, effective_stats_mode); @@ -1215,7 +1214,10 @@ class CounterContext : public BaseCounterContext m_objectIdsMap.erase(it_vid); } - auto counter_data = std::make_shared>(rid, supportedIds); + size_t groupIndex = m_objectSupportedCountersGroupMap[vid]; + std::vector objCounterIds(m_supportedCounterGroups[groupIndex].begin(), + m_supportedCounterGroups[groupIndex].end()); + auto counter_data = std::make_shared>(rid, objCounterIds); m_objectIdsMap.emplace(vid, counter_data); } else @@ -1530,7 +1532,7 @@ class CounterContext : public BaseCounterContext std::vector stats(statIds.size(), 0); if (!collectData(rid, statIds, effective_stats_mode, true, stats)) { - SWSS_LOG_ERROR("counter read failed on RID 0x%x on intf 0x%x", rid, vid); + SWSS_LOG_ERROR("counter read failed on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); continue; } @@ -1592,7 +1594,7 @@ class CounterContext : public BaseCounterContext private: std::set setupBaseCounterGroup( _In_ sai_object_id_t rid, - _In_ const std::vector counter_ids, + _In_ const std::vector& counter_ids, _In_ sai_stats_mode_t &stats_mode) { SWSS_LOG_ENTER(); @@ -1987,7 +1989,9 @@ class CounterContext : public BaseCounterContext std::vector extraCounters; std::set newCounters; - // Counter groups is a subset of counter_ids in any situation + // Counter groups are a subset of counter_ids in any situation + // Vectors need to be sorted for set_difference for defined behavior - counter_ids are already sorted + std::sort(countersToPoll.begin(), countersToPoll.end()); std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), std::inserter(extraCounters, extraCounters.begin())); for (const StatType &counter : extraCounters) @@ -2029,8 +2033,10 @@ class CounterContext : public BaseCounterContext if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter && m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]] != m_supportedCounterGroups[m_counterGroupsSorted[i].idx]) { - std::set oldGroup = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; - newCounters.insert(oldGroup.begin(), oldGroup.end()); + std::set prevGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + newCounters.insert(prevGroup.begin(), prevGroup.end()); + std::set currGroup = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; + newCounters.insert(currGroup.begin(), currGroup.end()); m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(newCounters); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); @@ -2335,6 +2341,7 @@ class DashMeterCounterContext : public BaseCounterContext { addObject(vid, rid, idStrings, per_object_stats_mode); } + void addObject( _In_ sai_object_id_t vid, _In_ sai_object_id_t rid, @@ -2461,7 +2468,7 @@ class DashMeterCounterContext : public BaseCounterContext _In_ const std::vector& idStrings, _In_ const std::string &per_object_stats_mode) override { - bulkAddObject(vids, rids, idStrings, per_object_stats_mode); + bulkAddObject(vids, rids, idStrings, per_object_stats_mode); } void bulkAddObject( @@ -3460,8 +3467,18 @@ void FlexCounter::addCounter( ""); } + catch (const std::exception& e) + { + SWSS_LOG_WARN("Error initializing SAI objects with counter groups: %s, falling back", e.what()); + getCounterContext(counterGroupRef->second)->addObject( + vid, + rid, + idStrings, + ""); + } catch (...) { - SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Unknown error initializing SAI objects with counter groups, falling back"); + getCounterContext(counterGroupRef->second)->addObject( vid, rid, @@ -3532,9 +3549,18 @@ void FlexCounter::bulkAddCounter( idStrings, ""); } + catch (const std::exception& e) + { + SWSS_LOG_WARN("Error initializing SAI objects with counter groups: %s, falling back", e.what()); + getCounterContext(counterGroupRef->second)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } catch (...) { - SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Unknown error initializing SAI objects with counter groups, falling back"); getCounterContext(counterGroupRef->second)->bulkAddObject( vids, rids, @@ -3572,9 +3598,18 @@ void FlexCounter::bulkAddCounter( statsMode); } + catch (const std::exception& e) + { + SWSS_LOG_WARN("Error initializing SAI objects with counter groups: %s, falling back", e.what()); + getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( + vids, + rids, + counterIds, + statsMode); + } catch (...) { - SWSS_LOG_WARN("Error occurred initializing SAI objects with counter groups, falling back to global counter list implementation"); + SWSS_LOG_WARN("Unknown error initializing SAI objects with counter groups, falling back"); getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( vids, rids, From e44c106157787fa52f646858515caf8ae400dfc8 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 21 Apr 2026 18:02:16 +0000 Subject: [PATCH 10/22] Add unittest for dynamic counter groups Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 279 +++++++++++++++++++++++++++++ 1 file changed, 279 insertions(+) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index b54bc7c399..4db4c75d73 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2021,3 +2021,282 @@ TEST(FlexCounter, noEniDashMeterCounter) counterVerifyFunc, false); } + +class FlexCounterTcpFallback : public ::testing::Test +{ +protected: + static constexpr const char *configPath = "/tmp/test_tcp_fallback_db_config.json"; + + void SetUp() override + { + const std::string configContent = R"({ + "INSTANCES": { + "redis": { + "hostname": "127.0.0.1", + "port": 6379, + "unix_socket_path": "" + } + }, + "DATABASES": { + "COUNTERS_DB": { + "id": 2, + "separator": ":", + "instance": "redis" + } + }, + "VERSION": "1.0" + })"; + + std::ofstream ofs(configPath); + ofs << configContent; + ofs.close(); + + swss::SonicDBConfig::reset(); + swss::SonicDBConfig::initialize(configPath); + } + + void TearDown() override + { + std::remove(configPath); + swss::SonicDBConfig::reset(); + swss::SonicDBConfig::initialize(); + } +}; + +TEST_F(FlexCounterTcpFallback, tcpFallbackWhenNoUnixSocket) +{ + EXPECT_TRUE(swss::SonicDBConfig::getDbSock("COUNTERS_DB").empty()); + + sai->mock_getStatsExt = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, sai_stats_mode_t, uint64_t *counters) { + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, sai_stat_capability_list_t *) { + return SAI_STATUS_FAILURE; + }; + sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, const sai_object_key_t *, uint32_t, const sai_stat_id_t *, sai_stats_mode_t, sai_status_t *, uint64_t *) { + return SAI_STATUS_FAILURE; + }; + + // FlexCounter should detect empty socket path and use TCP + FlexCounter fc("test_tcp", sai, "COUNTERS_DB"); + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + + std::vector values; + values.emplace_back(POLL_INTERVAL_FIELD, "1000"); + values.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + values.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + fc.addCounterPlugin(values); + + values.clear(); + values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_UCAST_PKTS"); + + auto object_ids = generateOids(1, SAI_OBJECT_TYPE_PORT); + fc.addCounter(object_ids[0], object_ids[0], values); + EXPECT_FALSE(fc.isEmpty()); + + // Use TCP to connect and verify counters were written + swss::DBConnector db("COUNTERS_DB", 0, true); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + waitForCounterKeys(countersTable, 1); + + std::string key = toOid(object_ids[0]); + waitForCounterValues(countersTable, key, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS"}, + {"100", "200"}); + + fc.removeCounter(object_ids[0]); + EXPECT_TRUE(fc.isEmpty()); + + countersTable.del(key); +} + +TEST(FlexCounter, dynamicCounterGroups) +{ + // This test tests counter group functionality. It ensures ecah interface only polls the counters they support. + + // All 6 counters are requested for every port, but getStats fails for + // unsupported ones, so each port's counter group only contains its subset. + // Port 0: IN_OCTETS, OUT_OCTETS, IN_ERRORS (3 of 6) + // Port 1: IN_UCAST_PKTS, OUT_UCAST_PKTS, OUT_ERRORS (3 of 6) + // Port 2: IN_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (4 of 6, overlaps both) + // + // Unsupported counters must not appear in Redis for any port. + + std::vector allCounterNames = { + "SAI_PORT_STAT_IF_IN_OCTETS", + "SAI_PORT_STAT_IF_OUT_OCTETS", + "SAI_PORT_STAT_IF_IN_UCAST_PKTS", + "SAI_PORT_STAT_IF_OUT_UCAST_PKTS", + "SAI_PORT_STAT_IF_IN_ERRORS", + "SAI_PORT_STAT_IF_OUT_ERRORS" + }; + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + auto oids = generateOids(3, SAI_OBJECT_TYPE_PORT); + ASSERT_EQ(oids.size(), 3u); + + // Per-RID supported counter sets (keyed by object_id since RID == VID in tests) + std::map> supportedMap; + supportedMap[oids[0]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_OUT_OCTETS, SAI_PORT_STAT_IF_IN_ERRORS}; + supportedMap[oids[1]] = {SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_ERRORS}; + supportedMap[oids[2]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_IN_ERRORS, SAI_PORT_STAT_IF_OUT_ERRORS}; + + // Deterministic counter values: value = (port_index + 1) * 1000 + stat_enum + auto computeValue = [&](sai_object_id_t rid, sai_port_stat_t stat) -> uint64_t { + int portIdx = 0; + for (size_t i = 0; i < oids.size(); i++) + { + if (oids[i] == rid) { portIdx = static_cast(i); break; } + } + return static_cast((portIdx + 1) * 1000 + static_cast(stat)); + }; + + auto isAllSupported = [&](sai_object_id_t rid, uint32_t count, const sai_stat_id_t *ids) -> bool { + auto it = supportedMap.find(rid); + if (it == supportedMap.end()) return false; + for (uint32_t i = 0; i < count; i++) + { + if (it->second.count(static_cast(ids[i])) == 0) + return false; + } + return true; + }; + + // Succeed only when all requested counters are in the RID's supported set. + sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t rid, + uint32_t count, const sai_stat_id_t *ids, + uint64_t *counters) -> sai_status_t + { + if (!isAllSupported(rid, count, ids)) + return SAI_STATUS_FAILURE; + for (uint32_t i = 0; i < count; i++) + counters[i] = computeValue(rid, static_cast(ids[i])); + return SAI_STATUS_SUCCESS; + }; + + // Same per-RID logic; PORT uses getStats, but mock this as a safety net. + sai->mock_getStatsExt = [&](sai_object_type_t, sai_object_id_t rid, + uint32_t count, const sai_stat_id_t *ids, + sai_stats_mode_t, uint64_t *counters) -> sai_status_t + { + if (!isAllSupported(rid, count, ids)) + return SAI_STATUS_FAILURE; + for (uint32_t i = 0; i < count; i++) + counters[i] = computeValue(rid, static_cast(ids[i])); + return SAI_STATUS_SUCCESS; + }; + + // Skip HW capability query; counter support is determined by getStats probing. + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, + sai_stat_capability_list_t *) + { + return SAI_STATUS_FAILURE; + }; + + // Force non-bulk path so per-port counter groups are polled individually. + sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, + const sai_object_key_t *, uint32_t, + const sai_stat_id_t *, sai_stats_mode_t, + sai_status_t *, uint64_t *) + { + return SAI_STATUS_FAILURE; + }; + + FlexCounter fc("test", sai, "COUNTERS_DB"); + + std::vector pluginValues; + pluginValues.emplace_back(POLL_INTERVAL_FIELD, "1000"); + pluginValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + pluginValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + fc.addCounterPlugin(pluginValues); + + std::vector counterValues; + counterValues.emplace_back(PORT_COUNTER_ID_LIST, join(allCounterNames)); + + for (auto oid : oids) + { + fc.addCounter(oid, oid, counterValues); + } + + EXPECT_FALSE(fc.isEmpty()); + + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + waitForCounterKeys(countersTable, 3); + + // Verify each port has exactly its supported counters with correct values, + // and unsupported counters are absent. + std::set allStats = { + SAI_PORT_STAT_IF_IN_OCTETS, + SAI_PORT_STAT_IF_OUT_OCTETS, + SAI_PORT_STAT_IF_IN_UCAST_PKTS, + SAI_PORT_STAT_IF_OUT_UCAST_PKTS, + SAI_PORT_STAT_IF_IN_ERRORS, + SAI_PORT_STAT_IF_OUT_ERRORS + }; + + for (size_t p = 0; p < oids.size(); p++) + { + std::string key = toOid(oids[p]); + const auto &supported = supportedMap[oids[p]]; + + // Wait for one of the supported counters to be populated + sai_port_stat_t firstSupported = *supported.begin(); + std::string firstField = sai_serialize_port_stat(firstSupported); + std::string expectedFirstVal = std::to_string(computeValue(oids[p], firstSupported)); + waitForCounterValues(countersTable, key, {firstField}, {expectedFirstVal}); + + // Verify all supported counters have correct values + for (auto stat : supported) + { + std::string field = sai_serialize_port_stat(stat); + std::string value; + ASSERT_TRUE(countersTable.hget(key, field, value)) + << "Port " << p << " missing supported counter " << field; + std::string expected = std::to_string(computeValue(oids[p], stat)); + EXPECT_EQ(value, expected) + << "Port " << p << " counter " << field << " value mismatch"; + } + + // Verify unsupported counters are absent + for (auto stat : allStats) + { + if (supported.count(stat)) + continue; + std::string field = sai_serialize_port_stat(stat); + std::string value; + EXPECT_FALSE(countersTable.hget(key, field, value)) + << "Port " << p << " should NOT have unsupported counter " << field + << " but found value '" << value << "'"; + } + } + + // Cleanup + for (auto oid : oids) + { + fc.removeCounter(oid); + countersTable.del(toOid(oid)); + } + EXPECT_TRUE(fc.isEmpty()); + + std::vector keys; + countersTable.getKeys(keys); + removeTimeStamp(keys, countersTable); + ASSERT_TRUE(keys.empty()); +} From 43822df90d9f87e262db07be108a3aa6ff695d8f Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 21 Apr 2026 21:18:27 +0000 Subject: [PATCH 11/22] Fix typo ecah -> each Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 4db4c75d73..77505b227e 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2126,7 +2126,7 @@ TEST_F(FlexCounterTcpFallback, tcpFallbackWhenNoUnixSocket) TEST(FlexCounter, dynamicCounterGroups) { - // This test tests counter group functionality. It ensures ecah interface only polls the counters they support. + // This test tests counter group functionality. It ensures each interface only polls the counters they support. // All 6 counters are requested for every port, but getStats fails for // unsupported ones, so each port's counter group only contains its subset. From d378c43ae72904385698a89108248542787f2816 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 21 Apr 2026 22:16:25 +0000 Subject: [PATCH 12/22] Add additional intf to unit test for more code and logic coverage Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 77505b227e..9a4edf7604 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2130,9 +2130,14 @@ TEST(FlexCounter, dynamicCounterGroups) // All 6 counters are requested for every port, but getStats fails for // unsupported ones, so each port's counter group only contains its subset. - // Port 0: IN_OCTETS, OUT_OCTETS, IN_ERRORS (3 of 6) - // Port 1: IN_UCAST_PKTS, OUT_UCAST_PKTS, OUT_ERRORS (3 of 6) - // Port 2: IN_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (4 of 6, overlaps both) + // Port 0: IN_OCTETS, OUT_OCTETS, IN_ERRORS (3 of 6) + // Port 1: IN_UCAST_PKTS, OUT_UCAST_PKTS, OUT_ERRORS (3 of 6) + // Port 2: IN_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (4 of 6, overlaps both) + // Port 3: IN_OCTETS, OUT_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (5 of 6, superset of Port 2) + // + // Port 3 matches Port 2's existing group, but supports an extra counter + // (OUT_OCTETS), so a new larger group must be created rather than reusing + // Port 2's group. // // Unsupported counters must not appear in Redis for any port. @@ -2146,14 +2151,15 @@ TEST(FlexCounter, dynamicCounterGroups) }; test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); - auto oids = generateOids(3, SAI_OBJECT_TYPE_PORT); - ASSERT_EQ(oids.size(), 3u); + auto oids = generateOids(4, SAI_OBJECT_TYPE_PORT); + ASSERT_EQ(oids.size(), 4u); // Per-RID supported counter sets (keyed by object_id since RID == VID in tests) std::map> supportedMap; supportedMap[oids[0]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_OUT_OCTETS, SAI_PORT_STAT_IF_IN_ERRORS}; supportedMap[oids[1]] = {SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_ERRORS}; supportedMap[oids[2]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_IN_ERRORS, SAI_PORT_STAT_IF_OUT_ERRORS}; + supportedMap[oids[3]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_OUT_OCTETS, SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_IN_ERRORS, SAI_PORT_STAT_IF_OUT_ERRORS}; // Deterministic counter values: value = (port_index + 1) * 1000 + stat_enum auto computeValue = [&](sai_object_id_t rid, sai_port_stat_t stat) -> uint64_t { @@ -2238,7 +2244,7 @@ TEST(FlexCounter, dynamicCounterGroups) swss::RedisPipeline pipeline(&db); swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); - waitForCounterKeys(countersTable, 3); + waitForCounterKeys(countersTable, 4); // Verify each port has exactly its supported counters with correct values, // and unsupported counters are absent. From f916eb05eca2ded29bb6f996b964f9314c9f32ea Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Tue, 21 Apr 2026 23:05:46 +0000 Subject: [PATCH 13/22] Fix typo superset -> super-set Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 9a4edf7604..34117feaca 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2133,7 +2133,7 @@ TEST(FlexCounter, dynamicCounterGroups) // Port 0: IN_OCTETS, OUT_OCTETS, IN_ERRORS (3 of 6) // Port 1: IN_UCAST_PKTS, OUT_UCAST_PKTS, OUT_ERRORS (3 of 6) // Port 2: IN_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (4 of 6, overlaps both) - // Port 3: IN_OCTETS, OUT_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (5 of 6, superset of Port 2) + // Port 3: IN_OCTETS, OUT_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (5 of 6, super-set of Port 2) // // Port 3 matches Port 2's existing group, but supports an extra counter // (OUT_OCTETS), so a new larger group must be created rather than reusing From 87d899d0d3649f721af0c9d40534cd5499ca98b9 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Wed, 22 Apr 2026 15:58:18 +0000 Subject: [PATCH 14/22] Use bulkAddCounter for broader logic check Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 34117feaca..6373374add 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2233,10 +2233,7 @@ TEST(FlexCounter, dynamicCounterGroups) std::vector counterValues; counterValues.emplace_back(PORT_COUNTER_ID_LIST, join(allCounterNames)); - for (auto oid : oids) - { - fc.addCounter(oid, oid, counterValues); - } + fc.bulkAddCounter(SAI_OBJECT_TYPE_PORT, oids, oids, counterValues); EXPECT_FALSE(fc.isEmpty()); From 1c936974455a5f70440b7d86fe813b1599f95206 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Wed, 27 May 2026 21:17:21 +0000 Subject: [PATCH 15/22] Allow 3 poll fails before ERR syslogs There are some ERR syslogs that originate from poll fails due to the reconfiguration and recreation of Port PHY Serdes SAI attributes at the initialization stage. These are expected to fail - adding a mechanism to only log ERR syslogs when a poll on a specific {rid, vid} combination fails more than 3 times. Tested with sonic-mgmt tests, no issues. Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 15 ++++++++++++++- syncd/FlexCounter.h | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 6d76d1ecc7..9c5a0d88bf 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1532,7 +1532,20 @@ class CounterContext : public BaseCounterContext std::vector stats(statIds.size(), 0); if (!collectData(rid, statIds, effective_stats_mode, true, stats)) { - SWSS_LOG_ERROR("counter read failed on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); + if (m_failedPolls.find({rid, vid}) == m_failedPolls.end()) + { + m_failedPolls[{rid, vid}] = 1; + SWSS_LOG_DEBUG("counter read failed 1 time on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); + } + else if (m_failedPolls[{rid, vid}] < 3) + { + m_failedPolls[{rid, vid}] += 1; + SWSS_LOG_DEBUG("counter read failed %d times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, m_failedPolls[{rid, vid}], rid, vid); + } + else + { + SWSS_LOG_ERROR("counter read failed more than 3 times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); + } continue; } diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index 24e750cedf..2e559ea418 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -78,6 +78,7 @@ namespace syncd std::string m_instanceId; std::set m_plugins; std::string m_bulkChunkSizePerPrefix; + std::map, uint32_t> m_failedPolls; public: bool always_check_supported_counters = false; From 453f5270b0d9e8f40aca0e2e9f283ad905720758 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Fri, 5 Jun 2026 23:24:59 +0000 Subject: [PATCH 16/22] Cast changes addressing review comments from 202511 PR Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 7 ++++++- unittest/syncd/TestFlexCounter.cpp | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 529278a379..7e28dce535 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -657,6 +657,11 @@ class CounterContext : public BaseCounterContext } // Perform a remove and re-add to simplify the logic here + // This remove function removes the counter from m_objectIdsMap and clears counter group mappings: + // - There is special handling logic for m_objectIdsMap below. + // - Counter group mappings are no longer needed for this vid since it is already stored in supportedIds. + // - This vid will also not have the same counter_ids for discovery later on, except from intentional discoveries + // via addObject, which we would want to rediscover anyways. removeObject(vid, false); bool supportBulk; @@ -2042,7 +2047,7 @@ class CounterContext : public BaseCounterContext // Vectors need to be sorted for set_difference for defined behavior - counter_ids are already sorted std::sort(countersToPoll.begin(), countersToPoll.end()); std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), - std::inserter(extraCounters, extraCounters.begin())); + std::back_inserter(extraCounters)); for (const StatType &counter : extraCounters) { std::vector singleCounter {counter}; diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 6d412bd850..40f8d9aebb 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2538,6 +2538,8 @@ TEST(FlexCounter, dynamicCounterGroups) }; // Force non-bulk path so per-port counter groups are polled individually. + // (per-port counter group discovery cannot be validated by the bulk-path, + // // it has different counter discovery logic.) sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, const sai_object_key_t *, uint32_t, const sai_stat_id_t *, sai_stats_mode_t, From e75405d2913c3d99a799b0ada6b32c0e3931d533 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Fri, 5 Jun 2026 23:26:34 +0000 Subject: [PATCH 17/22] Fix comment indentation Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 40f8d9aebb..1e0784378a 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2539,7 +2539,7 @@ TEST(FlexCounter, dynamicCounterGroups) // Force non-bulk path so per-port counter groups are polled individually. // (per-port counter group discovery cannot be validated by the bulk-path, - // // it has different counter discovery logic.) + // it has different counter discovery logic.) sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, const sai_object_key_t *, uint32_t, const sai_stat_id_t *, sai_stats_mode_t, @@ -2628,4 +2628,4 @@ TEST(FlexCounter, dynamicCounterGroups) countersTable.getKeys(keys); removeTimeStamp(keys, countersTable); ASSERT_TRUE(keys.empty()); -} \ No newline at end of file +} From 3782ae4683a9c2aae94eececf57a1374b480c73d Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Thu, 18 Jun 2026 00:57:59 +0000 Subject: [PATCH 18/22] Test added, removeObject condition added, sorting done in lambda Addressing review comments in [202511 PR](https://github.com/sonic-net/sonic-sairedis/pull/1862) and casting to master: - Added bulk path test case equivalent of dynamicCounterGroups - Added logic to determine when to run removeObject when adding object via counter groups - Changed poll retry ERR logging to print only once rather than continuous logging - Refactored static sort function to inline-lambdas to avoid CI build checks failing on SWSS_LOG_ENTER requirement. Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 146 ++++++++++---- unittest/syncd/TestFlexCounter.cpp | 302 +++++++++++++++++++++++++++++ 2 files changed, 407 insertions(+), 41 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 7e28dce535..a5c01824d5 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -553,24 +553,6 @@ class CounterContext : public BaseCounterContext SWSS_LOG_ENTER(); return &m_supportedCounterGroups[cgr.idx]; } - static bool counterGroupRefDscSorter(CounterGroupRef const& lhs, CounterGroupRef const& rhs) - { - SWSS_LOG_ENTER(); - // Must adhere to strict weak ordering - if (lhs.idx == rhs.idx) - { - return false; // The sets are equivalent - } - else if (lhs.size == rhs.size) - { - return lhs.idx < rhs.idx; // order by earliest creation when size is same - } - else - { - return lhs.size > rhs.size; - } - } - public: typedef CounterIds CounterIdsType; typedef BulkStatsContext BulkContextType; @@ -629,7 +611,6 @@ class CounterContext : public BaseCounterContext std::set counter_ids_set = setupBaseCounterGroup(rid, counter_ids, effective_stats_mode); counter_ids = std::vector(counter_ids_set.begin(), counter_ids_set.end()); - std::sort(counter_ids.begin(), counter_ids.end()); updateSupportedCounterGroups(rid, vid, counter_ids, effective_stats_mode); if (m_objectSupportedCountersGroupMap.count(vid) == 0) @@ -656,14 +637,6 @@ class CounterContext : public BaseCounterContext } } - // Perform a remove and re-add to simplify the logic here - // This remove function removes the counter from m_objectIdsMap and clears counter group mappings: - // - There is special handling logic for m_objectIdsMap below. - // - Counter group mappings are no longer needed for this vid since it is already stored in supportedIds. - // - This vid will also not have the same counter_ids for discovery later on, except from intentional discoveries - // via addObject, which we would want to rediscover anyways. - removeObject(vid, false); - bool supportBulk; // TODO: use if const expression when cpp17 is supported if (HasStatsMode::value) @@ -1583,8 +1556,9 @@ class CounterContext : public BaseCounterContext m_failedPolls[{rid, vid}] += 1; SWSS_LOG_DEBUG("counter read failed %d times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, m_failedPolls[{rid, vid}], rid, vid); } - else + else if (m_failedPolls[{rid, vid}] == 3) { + m_failedPolls[{rid, vid}] += 1; SWSS_LOG_ERROR("counter read failed more than 3 times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); } continue; @@ -1692,7 +1666,24 @@ class CounterContext : public BaseCounterContext { m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size(), counter_ids_set.size())); m_supportedCounterGroups.push_back(counter_ids_set); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + // Must adhere to strict weak ordering + // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + }); } } return counter_ids_set; @@ -2024,10 +2015,25 @@ class CounterContext : public BaseCounterContext _In_ sai_stats_mode_t stats_mode) { SWSS_LOG_ENTER(); - if (m_objectSupportedCountersGroupMap.count(vid) && !always_check_supported_counters) + if (m_objectSupportedCountersGroupMap.find(vid) != m_objectSupportedCountersGroupMap.end()) { - SWSS_LOG_NOTICE("Ignore checking of supported counters"); - return; + if (!always_check_supported_counters) + { + SWSS_LOG_NOTICE("Ignore checking of supported counters"); + return; + } + // If there is already a counter group for this vid, and that counter group contains a counter that is not + // included in counter_ids, then it means we are handling a different set of counters. + // Remove the previous group and mappings to this vid + std::set existingGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + for (auto &counter : existingGroup) + { + if (find(counter_ids.begin(), counter_ids.end(), counter) == counter_ids.end()) + { + removeObject(vid, false); + break; + } + } } // Check if a matching counter group already exists @@ -2042,12 +2048,18 @@ class CounterContext : public BaseCounterContext // Success - Check support for counters not in counter group (extra counters) std::vector extraCounters; std::set newCounters; + std::set intersectedCounters; + bool newGroup = false; - // Counter groups are a subset of counter_ids in any situation - // Vectors need to be sorted for set_difference for defined behavior - counter_ids are already sorted - std::sort(countersToPoll.begin(), countersToPoll.end()); - std::set_difference(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), + // Make it such that the counter group is a subset of counter_ids + std::set_intersection(counter_ids.begin(), counter_ids.end(), countersToPoll.begin(), countersToPoll.end(), + std::inserter(intersectedCounters, intersectedCounters.begin())); + newGroup = intersectedCounters.size() < countersToPoll.size(); + + // Vectors need to be sorted for set_difference for defined behavior - sets are sorted in C++ + std::set_difference(counter_ids.begin(), counter_ids.end(), intersectedCounters.begin(), intersectedCounters.end(), std::back_inserter(extraCounters)); + for (const StatType &counter : extraCounters) { std::vector singleCounter {counter}; @@ -2055,6 +2067,7 @@ class CounterContext : public BaseCounterContext if (collectData(rid, singleCounter, stats_mode, false, singleValue)) { newCounters.insert(counter); + newGroup = true; } else { @@ -2063,10 +2076,10 @@ class CounterContext : public BaseCounterContext sai_serialize_object_id(vid).c_str()); } } - if (!newCounters.empty()) + if (newGroup) { // New counters discovered, create new counter group - newCounters.insert(counterSet->begin(), counterSet->end()); + newCounters.insert(intersectedCounters.begin(), intersectedCounters.end()); // If vid already has assigned counter group, merge the two groups if dont_clear flag is set if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter) @@ -2078,7 +2091,24 @@ class CounterContext : public BaseCounterContext m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(newCounters); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + // Must adhere to strict weak ordering + // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + }); } else { @@ -2094,7 +2124,24 @@ class CounterContext : public BaseCounterContext m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(newCounters); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + // Must adhere to strict weak ordering + // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + }); } else { @@ -2126,7 +2173,24 @@ class CounterContext : public BaseCounterContext m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); m_supportedCounterGroups.push_back(supportedIds); m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), &counterGroupRefDscSorter); + // Must adhere to strict weak ordering + // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + }); } void updateSupportedCounters( diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 1e0784378a..85d030214d 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2629,3 +2629,305 @@ TEST(FlexCounter, dynamicCounterGroups) removeTimeStamp(keys, countersTable); ASSERT_TRUE(keys.empty()); } + +TEST(FlexCounter, dynamicCounterGroupsBulkPath) +{ + // Bulk-path variant of dynamicCounterGroups. Uses + // bulkAddObjectWithCounterGroups, which selects the largest counter group + // for bulkGetStats and falls back to single-object polling for ports whose + // supported set is smaller. + + // All 6 counters are requested for every port, but getStats fails for + // unsupported ones, so each port's counter group only contains its subset. + // Port 0: IN_OCTETS, OUT_OCTETS, IN_ERRORS (3 of 6) + // Port 1: IN_UCAST_PKTS, OUT_UCAST_PKTS, OUT_ERRORS (3 of 6) + // Port 2: IN_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (4 of 6, overlaps both) + // Port 3: IN_OCTETS, OUT_OCTETS, IN_UCAST_PKTS, IN_ERRORS, OUT_ERRORS (5 of 6, super-set of Port 2) + // + // Port 3 matches Port 2's existing group, but supports an extra counter + // (OUT_OCTETS), so a new larger group must be created rather than reusing + // Port 2's group. + // + // Unsupported counters must not appear in Redis for any port. + + std::vector allCounterNames = { + "SAI_PORT_STAT_IF_IN_OCTETS", + "SAI_PORT_STAT_IF_OUT_OCTETS", + "SAI_PORT_STAT_IF_IN_UCAST_PKTS", + "SAI_PORT_STAT_IF_OUT_UCAST_PKTS", + "SAI_PORT_STAT_IF_IN_ERRORS", + "SAI_PORT_STAT_IF_OUT_ERRORS" + }; + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + auto oids = generateOids(4, SAI_OBJECT_TYPE_PORT); + ASSERT_EQ(oids.size(), 4u); + + // Per-RID supported counter sets (keyed by object_id since RID == VID in tests) + std::map> supportedMap; + supportedMap[oids[0]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_OUT_OCTETS, SAI_PORT_STAT_IF_IN_ERRORS}; + supportedMap[oids[1]] = {SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_UCAST_PKTS, SAI_PORT_STAT_IF_OUT_ERRORS}; + supportedMap[oids[2]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_IN_ERRORS, SAI_PORT_STAT_IF_OUT_ERRORS}; + supportedMap[oids[3]] = {SAI_PORT_STAT_IF_IN_OCTETS, SAI_PORT_STAT_IF_OUT_OCTETS, SAI_PORT_STAT_IF_IN_UCAST_PKTS, SAI_PORT_STAT_IF_IN_ERRORS, SAI_PORT_STAT_IF_OUT_ERRORS}; + + // Deterministic counter values: value = (port_index + 1) * 1000 + stat_enum + auto computeValue = [&](sai_object_id_t rid, sai_port_stat_t stat) -> uint64_t { + int portIdx = 0; + for (size_t i = 0; i < oids.size(); i++) + { + if (oids[i] == rid) { portIdx = static_cast(i); break; } + } + return static_cast((portIdx + 1) * 1000 + static_cast(stat)); + }; + + auto isAllSupported = [&](sai_object_id_t rid, uint32_t count, const sai_stat_id_t *ids) -> bool { + auto it = supportedMap.find(rid); + if (it == supportedMap.end()) return false; + for (uint32_t i = 0; i < count; i++) + { + if (it->second.count(static_cast(ids[i])) == 0) + return false; + } + return true; + }; + + // Succeed only when all requested counters are in the RID's supported set. + sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t rid, + uint32_t count, const sai_stat_id_t *ids, + uint64_t *counters) -> sai_status_t + { + if (!isAllSupported(rid, count, ids)) + return SAI_STATUS_FAILURE; + for (uint32_t i = 0; i < count; i++) + counters[i] = computeValue(rid, static_cast(ids[i])); + return SAI_STATUS_SUCCESS; + }; + + // Same per-RID logic; PORT uses getStats, but mock this as a safety net. + sai->mock_getStatsExt = [&](sai_object_type_t, sai_object_id_t rid, + uint32_t count, const sai_stat_id_t *ids, + sai_stats_mode_t, uint64_t *counters) -> sai_status_t + { + if (!isAllSupported(rid, count, ids)) + return SAI_STATUS_FAILURE; + for (uint32_t i = 0; i < count; i++) + counters[i] = computeValue(rid, static_cast(ids[i])); + return SAI_STATUS_SUCCESS; + }; + + // Skip HW capability query; counter support is determined by getStats probing. + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, + sai_stat_capability_list_t *) + { + return SAI_STATUS_FAILURE; + }; + + // Bulk path: succeed for any counter set so that the largest counter group + // is polled via bulkGetStats. Return the same deterministic values as the + // single-object path so verification is identical. + sai->mock_bulkGetStats = [&](sai_object_id_t, + sai_object_type_t, + uint32_t object_count, + const sai_object_key_t *object_keys, + uint32_t number_of_counters, + const sai_stat_id_t *counter_ids, + sai_stats_mode_t, + sai_status_t *object_statuses, + uint64_t *counters) -> sai_status_t + { + for (uint32_t i = 0; i < object_count; i++) + { + sai_object_id_t rid = object_keys[i].key.object_id; + object_statuses[i] = SAI_STATUS_SUCCESS; + for (uint32_t j = 0; j < number_of_counters; j++) + { + counters[i * number_of_counters + j] = + computeValue(rid, static_cast(counter_ids[j])); + } + } + return SAI_STATUS_SUCCESS; + }; + + FlexCounter fc("test", sai, "COUNTERS_DB"); + + std::vector pluginValues; + pluginValues.emplace_back(POLL_INTERVAL_FIELD, "1000"); + pluginValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + pluginValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + fc.addCounterPlugin(pluginValues); + + std::vector counterValues; + counterValues.emplace_back(PORT_COUNTER_ID_LIST, join(allCounterNames)); + + fc.bulkAddCounter(SAI_OBJECT_TYPE_PORT, oids, oids, counterValues); + + EXPECT_FALSE(fc.isEmpty()); + + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + waitForCounterKeys(countersTable, 4); + + // Verify each port has exactly its supported counters with correct values, + // and unsupported counters are absent. + std::set allStats = { + SAI_PORT_STAT_IF_IN_OCTETS, + SAI_PORT_STAT_IF_OUT_OCTETS, + SAI_PORT_STAT_IF_IN_UCAST_PKTS, + SAI_PORT_STAT_IF_OUT_UCAST_PKTS, + SAI_PORT_STAT_IF_IN_ERRORS, + SAI_PORT_STAT_IF_OUT_ERRORS + }; + + for (size_t p = 0; p < oids.size(); p++) + { + std::string key = toOid(oids[p]); + const auto &supported = supportedMap[oids[p]]; + + // Wait for one of the supported counters to be populated + sai_port_stat_t firstSupported = *supported.begin(); + std::string firstField = sai_serialize_port_stat(firstSupported); + std::string expectedFirstVal = std::to_string(computeValue(oids[p], firstSupported)); + waitForCounterValues(countersTable, key, {firstField}, {expectedFirstVal}); + + // Verify all supported counters have correct values + for (auto stat : supported) + { + std::string field = sai_serialize_port_stat(stat); + std::string value; + ASSERT_TRUE(countersTable.hget(key, field, value)) + << "Port " << p << " missing supported counter " << field; + std::string expected = std::to_string(computeValue(oids[p], stat)); + EXPECT_EQ(value, expected) + << "Port " << p << " counter " << field << " value mismatch"; + } + + // Verify unsupported counters are absent + for (auto stat : allStats) + { + if (supported.count(stat)) + continue; + std::string field = sai_serialize_port_stat(stat); + std::string value; + EXPECT_FALSE(countersTable.hget(key, field, value)) + << "Port " << p << " should NOT have unsupported counter " << field + << " but found value '" << value << "'"; + } + } + + // Cleanup + for (auto oid : oids) + { + fc.removeCounter(oid); + countersTable.del(toOid(oid)); + } + EXPECT_TRUE(fc.isEmpty()); + + std::vector keys; + countersTable.getKeys(keys); + removeTimeStamp(keys, countersTable); + ASSERT_TRUE(keys.empty()); +} + +class FlexCounterTcpFallback : public ::testing::Test +{ +protected: + static constexpr const char *configPath = "/tmp/test_tcp_fallback_db_config.json"; + + void SetUp() override + { + const std::string configContent = R"({ + "INSTANCES": { + "redis": { + "hostname": "127.0.0.1", + "port": 6379, + "unix_socket_path": "" + } + }, + "DATABASES": { + "COUNTERS_DB": { + "id": 2, + "separator": ":", + "instance": "redis" + } + }, + "VERSION": "1.0" + })"; + + std::ofstream ofs(configPath); + ofs << configContent; + ofs.close(); + + swss::SonicDBConfig::reset(); + swss::SonicDBConfig::initialize(configPath); + } + + void TearDown() override + { + std::remove(configPath); + swss::SonicDBConfig::reset(); + swss::SonicDBConfig::initialize(); + } +}; + +TEST_F(FlexCounterTcpFallback, tcpFallbackWhenNoUnixSocket) +{ + EXPECT_TRUE(swss::SonicDBConfig::getDbSock("COUNTERS_DB").empty()); + + sai->mock_getStatsExt = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, sai_stats_mode_t, uint64_t *counters) { + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, sai_stat_capability_list_t *) { + return SAI_STATUS_FAILURE; + }; + sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, const sai_object_key_t *, uint32_t, const sai_stat_id_t *, sai_stats_mode_t, sai_status_t *, uint64_t *) { + return SAI_STATUS_FAILURE; + }; + + // FlexCounter should detect empty socket path and use TCP + FlexCounter fc("test_tcp", sai, "COUNTERS_DB"); + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + + std::vector values; + values.emplace_back(POLL_INTERVAL_FIELD, "1000"); + values.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + values.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + fc.addCounterPlugin(values); + + values.clear(); + values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_UCAST_PKTS"); + + auto object_ids = generateOids(1, SAI_OBJECT_TYPE_PORT); + fc.addCounter(object_ids[0], object_ids[0], values); + EXPECT_FALSE(fc.isEmpty()); + + // Use TCP to connect and verify counters were written + swss::DBConnector db("COUNTERS_DB", 0, true); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + waitForCounterKeys(countersTable, 1); + + std::string key = toOid(object_ids[0]); + waitForCounterValues(countersTable, key, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS"}, + {"100", "200"}); + + fc.removeCounter(object_ids[0]); + EXPECT_TRUE(fc.isEmpty()); + + countersTable.del(key); +} From 3eb7802fdbb8bddb585bd769018d4c82455e6b96 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Mon, 29 Jun 2026 18:57:44 +0000 Subject: [PATCH 19/22] Remove duplicated test from cast Signed-off-by: Justin Wong --- unittest/syncd/TestFlexCounter.cpp | 102 ----------------------------- 1 file changed, 102 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 85d030214d..305d5086ce 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2829,105 +2829,3 @@ TEST(FlexCounter, dynamicCounterGroupsBulkPath) removeTimeStamp(keys, countersTable); ASSERT_TRUE(keys.empty()); } - -class FlexCounterTcpFallback : public ::testing::Test -{ -protected: - static constexpr const char *configPath = "/tmp/test_tcp_fallback_db_config.json"; - - void SetUp() override - { - const std::string configContent = R"({ - "INSTANCES": { - "redis": { - "hostname": "127.0.0.1", - "port": 6379, - "unix_socket_path": "" - } - }, - "DATABASES": { - "COUNTERS_DB": { - "id": 2, - "separator": ":", - "instance": "redis" - } - }, - "VERSION": "1.0" - })"; - - std::ofstream ofs(configPath); - ofs << configContent; - ofs.close(); - - swss::SonicDBConfig::reset(); - swss::SonicDBConfig::initialize(configPath); - } - - void TearDown() override - { - std::remove(configPath); - swss::SonicDBConfig::reset(); - swss::SonicDBConfig::initialize(); - } -}; - -TEST_F(FlexCounterTcpFallback, tcpFallbackWhenNoUnixSocket) -{ - EXPECT_TRUE(swss::SonicDBConfig::getDbSock("COUNTERS_DB").empty()); - - sai->mock_getStatsExt = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, sai_stats_mode_t, uint64_t *counters) { - for (uint32_t i = 0; i < number_of_counters; i++) - { - counters[i] = (i + 1) * 100; - } - return SAI_STATUS_SUCCESS; - }; - sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { - for (uint32_t i = 0; i < number_of_counters; i++) - { - counters[i] = (i + 1) * 100; - } - return SAI_STATUS_SUCCESS; - }; - sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, sai_stat_capability_list_t *) { - return SAI_STATUS_FAILURE; - }; - sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, const sai_object_key_t *, uint32_t, const sai_stat_id_t *, sai_stats_mode_t, sai_status_t *, uint64_t *) { - return SAI_STATUS_FAILURE; - }; - - // FlexCounter should detect empty socket path and use TCP - FlexCounter fc("test_tcp", sai, "COUNTERS_DB"); - - test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); - - std::vector values; - values.emplace_back(POLL_INTERVAL_FIELD, "1000"); - values.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); - values.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); - fc.addCounterPlugin(values); - - values.clear(); - values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_UCAST_PKTS"); - - auto object_ids = generateOids(1, SAI_OBJECT_TYPE_PORT); - fc.addCounter(object_ids[0], object_ids[0], values); - EXPECT_FALSE(fc.isEmpty()); - - // Use TCP to connect and verify counters were written - swss::DBConnector db("COUNTERS_DB", 0, true); - swss::RedisPipeline pipeline(&db); - swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); - - waitForCounterKeys(countersTable, 1); - - std::string key = toOid(object_ids[0]); - waitForCounterValues(countersTable, key, - {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS"}, - {"100", "200"}); - - fc.removeCounter(object_ids[0]); - EXPECT_TRUE(fc.isEmpty()); - - countersTable.del(key); -} From 10b82166949545808e1b8dda64201cfadb5118cb Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Fri, 3 Jul 2026 23:17:53 +0000 Subject: [PATCH 20/22] Address review comments List of changes made: - Added tracking of free indicies for reuse - Added m_failedPolls cleanup when object is removed - Improved m_failedPolls lookup logic to only use a single lookup - Added addGroup helper - Used const& refs for sets instead of copies where applicable - Removed getSortedCGRef() for to avoid performance impact from the SWSS_LOG_ENTER() requirement in functions - Better clarity by changing find() -> std::find() - Simplied erase-emplace logic by using built in [] operator logic - Normalized key check method when dealing with maps - Indentation fixes - Minor optimization when iterating through counter groups in setupBaseCounterGroup() - Added warning comments on parts of code where more care is required (consuming of const refs) - Added unit test for m_failedPolls logic Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 229 ++++++++++------------------- unittest/syncd/TestFlexCounter.cpp | 117 +++++++++++++++ 2 files changed, 193 insertions(+), 153 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index a5c01824d5..fa2975ced4 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -548,11 +548,6 @@ class CounterContext : public BaseCounterContext SWSS_LOG_ENTER(); return {idx, size}; } - std::set* getSortedCGRef(CounterGroupRef const& cgr) - { - SWSS_LOG_ENTER(); - return &m_supportedCounterGroups[cgr.idx]; - } public: typedef CounterIds CounterIdsType; typedef BulkStatsContext BulkContextType; @@ -1129,15 +1124,7 @@ class CounterContext : public BaseCounterContext sai_serialize_object_id(rid).c_str(), sai_serialize_object_id(vid).c_str()); throw std::runtime_error("Test counter poll failed on populating m_objectIdsMap"); } - auto it_vid = m_objectIdsMap.find(vid); - if (it_vid != m_objectIdsMap.end()) - { - // Remove and re-add if vid already exists - m_objectIdsMap.erase(it_vid); - } - - auto counter_data = std::make_shared>(rid, intf_counter_ids); - m_objectIdsMap.emplace(vid, counter_data); + m_objectIdsMap[vid] = std::make_shared>(rid, intf_counter_ids); SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); } return; @@ -1171,7 +1158,8 @@ class CounterContext : public BaseCounterContext }; // Use counter group with the most counters - std::vector supportedIds(getSortedCGRef(m_counterGroupsSorted[0])->begin(), getSortedCGRef(m_counterGroupsSorted[0])->end()); + const std::set& largest_set = m_supportedCounterGroups[m_counterGroupsSorted[0].idx]; + std::vector supportedIds(largest_set.begin(), largest_set.end()); if (m_counterChunkSizeMapFromPrefix.empty()) { @@ -1221,18 +1209,10 @@ class CounterContext : public BaseCounterContext { SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); - auto it_vid = m_objectIdsMap.find(vid); - if (it_vid != m_objectIdsMap.end()) - { - // Remove and re-add if vid already exists - m_objectIdsMap.erase(it_vid); - } - size_t groupIndex = m_objectSupportedCountersGroupMap[vid]; std::vector objCounterIds(m_supportedCounterGroups[groupIndex].begin(), m_supportedCounterGroups[groupIndex].end()); - auto counter_data = std::make_shared>(rid, objCounterIds); - m_objectIdsMap.emplace(vid, counter_data); + m_objectIdsMap[vid] = std::make_shared>(rid, objCounterIds); } else { @@ -1334,17 +1314,7 @@ class CounterContext : public BaseCounterContext auto vid = vids[i]; std::vector stats(counter_ids.size()); if (collectData(rid, counter_ids, effective_stats_mode, false, stats)) { - - auto it_vid = m_objectIdsMap.find(vid); - if (it_vid != m_objectIdsMap.end()) - { - // Remove and re-add if vid already exists - m_objectIdsMap.erase(it_vid); - } - - auto counter_data = std::make_shared>(rid, counter_ids); - m_objectIdsMap.emplace(vid, counter_data); - + m_objectIdsMap[vid] = std::make_shared>(rid, counter_ids); SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); } else { SWSS_LOG_WARN("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); @@ -1428,16 +1398,7 @@ class CounterContext : public BaseCounterContext else if (!double_confirm_supported_counters || collectData(rid, it.first, effective_stats_mode, false, stats)) { SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); - - auto it_vid = m_objectIdsMap.find(vid); - if (it_vid != m_objectIdsMap.end()) - { - // Remove and re-add if vid already exists - m_objectIdsMap.erase(it_vid); - } - - auto counter_data = std::make_shared>(rid, supportedIds); - m_objectIdsMap.emplace(vid, counter_data); + m_objectIdsMap[vid] = std::make_shared>(rid, supportedIds); } else { @@ -1462,6 +1423,45 @@ class CounterContext : public BaseCounterContext removeObject(vid, true); } + size_t addGroup(std::set group) + { + SWSS_LOG_ENTER(); + size_t idx; + if (!m_freeGroupIndices.empty()) + { + idx = m_freeGroupIndices.back(); + m_freeGroupIndices.pop_back(); + m_supportedCounterGroups[idx] = std::move(group); + } + else + { + idx = m_supportedCounterGroups.size(); + m_supportedCounterGroups.push_back(std::move(group)); + } + m_counterGroupsSorted.push_back(makeCounterGroupRef(idx, m_supportedCounterGroups[idx].size())); + + // Must adhere to strict weak ordering + // Use lambda instead of static func to avoid SWSS_LOG_ENTER CI build requirement for better performance + std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), + [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) + { + if (lhs.idx == rhs.idx) + { + return false; // The sets are equivalent + } + else if (lhs.size == rhs.size) + { + return lhs.idx < rhs.idx; // order by earliest creation when size is same + } + else + { + return lhs.size > rhs.size; + } + }); + + return idx; + } + void cleanupCounterGroupMapping( _In_ sai_object_id_t vid) { @@ -1497,6 +1497,7 @@ class CounterContext : public BaseCounterContext // Clear the group data (can't erase from vector without invalidating other indices) m_supportedCounterGroups[removedGroupIdx].clear(); + m_freeGroupIndices.push_back(removedGroupIdx); } } @@ -1510,6 +1511,8 @@ class CounterContext : public BaseCounterContext auto iter = m_objectIdsMap.find(vid); if (iter != m_objectIdsMap.end()) { + auto rid = iter->second->rid; + m_failedPolls.erase({rid, vid}); m_objectIdsMap.erase(iter); } @@ -1546,19 +1549,17 @@ class CounterContext : public BaseCounterContext std::vector stats(statIds.size(), 0); if (!collectData(rid, statIds, effective_stats_mode, true, stats)) { - if (m_failedPolls.find({rid, vid}) == m_failedPolls.end()) + uint32_t n = ++m_failedPolls[{rid, vid}]; + if (n == 1) { - m_failedPolls[{rid, vid}] = 1; SWSS_LOG_DEBUG("counter read failed 1 time on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); } - else if (m_failedPolls[{rid, vid}] < 3) + else if (n <= 3) { - m_failedPolls[{rid, vid}] += 1; - SWSS_LOG_DEBUG("counter read failed %d times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, m_failedPolls[{rid, vid}], rid, vid); + SWSS_LOG_DEBUG("counter read failed %u times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, n, rid, vid); } - else if (m_failedPolls[{rid, vid}] == 3) + else if (n == 4) { - m_failedPolls[{rid, vid}] += 1; SWSS_LOG_ERROR("counter read failed more than 3 times on RID 0x%" PRIx64 " on intf 0x%" PRIx64, rid, vid); } continue; @@ -1647,8 +1648,7 @@ class CounterContext : public BaseCounterContext // Create base counter group if not yet instantiated if (m_supportedCounterGroups.empty()) { - m_supportedCounterGroups.push_back(counter_ids_set); - m_counterGroupsSorted.push_back(makeCounterGroupRef(0, counter_ids_set.size())); + addGroup(counter_ids_set); } // Replace base counter group if full counter set differs else @@ -1656,7 +1656,7 @@ class CounterContext : public BaseCounterContext bool groupExists = false; for (size_t i = 0; i < m_supportedCounterGroups.size(); i++) { - if (m_supportedCounterGroups[i] == counter_ids_set) + if (!m_supportedCounterGroups[i].empty() && m_supportedCounterGroups[i] == counter_ids_set) { groupExists = true; break; @@ -1664,26 +1664,7 @@ class CounterContext : public BaseCounterContext } if (!groupExists) { - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size(), counter_ids_set.size())); - m_supportedCounterGroups.push_back(counter_ids_set); - // Must adhere to strict weak ordering - // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), - [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) - { - if (lhs.idx == rhs.idx) - { - return false; // The sets are equivalent - } - else if (lhs.size == rhs.size) - { - return lhs.idx < rhs.idx; // order by earliest creation when size is same - } - else - { - return lhs.size > rhs.size; - } - }); + addGroup(counter_ids_set); } } return counter_ids_set; @@ -2015,7 +1996,7 @@ class CounterContext : public BaseCounterContext _In_ sai_stats_mode_t stats_mode) { SWSS_LOG_ENTER(); - if (m_objectSupportedCountersGroupMap.find(vid) != m_objectSupportedCountersGroupMap.end()) + if (m_objectSupportedCountersGroupMap.count(vid)) { if (!always_check_supported_counters) { @@ -2025,10 +2006,10 @@ class CounterContext : public BaseCounterContext // If there is already a counter group for this vid, and that counter group contains a counter that is not // included in counter_ids, then it means we are handling a different set of counters. // Remove the previous group and mappings to this vid - std::set existingGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + const std::set& existingGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; for (auto &counter : existingGroup) { - if (find(counter_ids.begin(), counter_ids.end(), counter) == counter_ids.end()) + if (std::find(counter_ids.begin(), counter_ids.end(), counter) == counter_ids.end()) { removeObject(vid, false); break; @@ -2040,9 +2021,9 @@ class CounterContext : public BaseCounterContext for (size_t i = 0; i < m_counterGroupsSorted.size(); i++) { // Try match - std::set* counterSet = getSortedCGRef(m_counterGroupsSorted[i]); - std::vector values(counterSet->size(), 0); - std::vector countersToPoll(counterSet->begin(), counterSet->end()); + const std::set& counterSet = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; + std::vector values(counterSet.size(), 0); + std::vector countersToPoll(counterSet.begin(), counterSet.end()); if (collectData(rid, countersToPoll, stats_mode, false, values)) { // Success - Check support for counters not in counter group (extra counters) @@ -2084,68 +2065,29 @@ class CounterContext : public BaseCounterContext // If vid already has assigned counter group, merge the two groups if dont_clear flag is set if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter) { - std::set oldGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; - newCounters.insert(oldGroup.begin(), oldGroup.end()); + const std::set& oldGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + newCounters.insert(oldGroup.begin(), oldGroup.end()); } - - m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); - m_supportedCounterGroups.push_back(newCounters); - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); - // Must adhere to strict weak ordering - // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), - [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) - { - if (lhs.idx == rhs.idx) - { - return false; // The sets are equivalent - } - else if (lhs.size == rhs.size) - { - return lhs.idx < rhs.idx; // order by earliest creation when size is same - } - else - { - return lhs.size > rhs.size; - } - }); + // References to oldGroup and newCounters must be consumed before this call + m_objectSupportedCountersGroupMap[vid] = addGroup(newCounters); } else { // Use existing counter group // If vid already has assigned counter group, merge the two groups if dont_clear flag is set if (m_objectSupportedCountersGroupMap.count(vid) && dont_clear_support_counter && - m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]] != m_supportedCounterGroups[m_counterGroupsSorted[i].idx]) + m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]] != m_supportedCounterGroups[m_counterGroupsSorted[i].idx]) { - std::set prevGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; - newCounters.insert(prevGroup.begin(), prevGroup.end()); - std::set currGroup = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; - newCounters.insert(currGroup.begin(), currGroup.end()); - m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); - m_supportedCounterGroups.push_back(newCounters); - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, newCounters.size())); - // Must adhere to strict weak ordering - // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), - [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) - { - if (lhs.idx == rhs.idx) - { - return false; // The sets are equivalent - } - else if (lhs.size == rhs.size) - { - return lhs.idx < rhs.idx; // order by earliest creation when size is same - } - else - { - return lhs.size > rhs.size; - } - }); + const std::set& prevGroup = m_supportedCounterGroups[m_objectSupportedCountersGroupMap[vid]]; + newCounters.insert(prevGroup.begin(), prevGroup.end()); + const std::set& currGroup = m_supportedCounterGroups[m_counterGroupsSorted[i].idx]; + newCounters.insert(currGroup.begin(), currGroup.end()); + // References to prevGroup, currGroup, and newCounters must be consumed before this call + m_objectSupportedCountersGroupMap[vid] = addGroup(newCounters); } else { - m_objectSupportedCountersGroupMap[vid] = m_counterGroupsSorted[i].idx; + m_objectSupportedCountersGroupMap[vid] = m_counterGroupsSorted[i].idx; } } return; @@ -2170,27 +2112,7 @@ class CounterContext : public BaseCounterContext } // Make new counter group and assign the index if not assigned - m_objectSupportedCountersGroupMap[vid] = m_supportedCounterGroups.size(); - m_supportedCounterGroups.push_back(supportedIds); - m_counterGroupsSorted.push_back(makeCounterGroupRef(m_supportedCounterGroups.size()-1, supportedIds.size())); - // Must adhere to strict weak ordering - // Use lambda instead of static function to avoid SWSS_LOG_ENTER CI build requirement for better performance - std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), - [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) - { - if (lhs.idx == rhs.idx) - { - return false; // The sets are equivalent - } - else if (lhs.size == rhs.size) - { - return lhs.idx < rhs.idx; // order by earliest creation when size is same - } - else - { - return lhs.size > rhs.size; - } - }); + m_objectSupportedCountersGroupMap[vid] = addGroup(supportedIds); } void updateSupportedCounters( @@ -2307,6 +2229,7 @@ class CounterContext : public BaseCounterContext std::map m_objectSupportedCountersGroupMap; std::vector> m_supportedCounterGroups; std::vector m_counterGroupsSorted; + std::vector m_freeGroupIndices; std::map> m_objectIdsMap; std::map, std::shared_ptr> m_bulkContexts; diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 305d5086ce..3069eadb92 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -2829,3 +2829,120 @@ TEST(FlexCounter, dynamicCounterGroupsBulkPath) removeTimeStamp(keys, countersTable); ASSERT_TRUE(keys.empty()); } + +TEST(FlexCounter, failedPollsCountAndCleanUp) +{ + // This test verifies that: + // + // 1. When getStats starts failing after successful polls on an already-added object, + // counter DB values go stale (stop updating) and the poll continues. + // 2. After removing and re-adding the same object, the failure count resets + // (m_failedPolls cleanup on remove) so it polls successfully again. + + std::atomic failGetStats{false}; + std::atomic pollCycleCount{0}; + + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, + sai_stat_capability_list_t *) + { + return SAI_STATUS_FAILURE; + }; + + sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t, + uint32_t number_of_counters, const sai_stat_id_t *, + uint64_t *counters) -> sai_status_t + { + if (failGetStats.load()) + { + pollCycleCount++; + return SAI_STATUS_FAILURE; + } + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + + sai->mock_bulkGetStats = [](sai_object_id_t, sai_object_type_t, uint32_t, + const sai_object_key_t *, uint32_t, + const sai_stat_id_t *, sai_stats_mode_t, + sai_status_t *, uint64_t *) + { + return SAI_STATUS_FAILURE; + }; + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + + sai_object_id_t oid{0x1000000000000}; + std::string expectedKey = toOid(oid); + + FlexCounter fc("test", sai, "COUNTERS_DB"); + + std::vector pluginValues; + pluginValues.emplace_back(POLL_INTERVAL_FIELD, "1000"); + pluginValues.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + pluginValues.emplace_back(STATS_MODE_FIELD, STATS_MODE_READ); + fc.addCounterPlugin(pluginValues); + + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + // Verify counters will stop updating DB after 3 or more failed polls + // Add object and verify counters in DB + std::vector counterValues; + counterValues.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_ERRORS"); + fc.addCounter(oid, oid, counterValues); + EXPECT_FALSE(fc.isEmpty()); + + waitForCounterKeys(countersTable, 1); + waitForCounterValues(countersTable, expectedKey, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_ERRORS"}, + {"100", "200"}); + + // getStats starts failing and DB values should go stale (not updated, not cleared). + failGetStats = true; + pollCycleCount = 0; + + auto deadline = std::chrono::steady_clock::now() + std::chrono::milliseconds(5000); + while (std::chrono::steady_clock::now() < deadline && pollCycleCount.load() < 3) + { + usleep(100 * 1000); + } + EXPECT_GE(pollCycleCount.load(), 3u); + + // Stale values should remain in the DB, unchanged + std::string value; + countersTable.hget(expectedKey, "SAI_PORT_STAT_IF_IN_OCTETS", value); + EXPECT_EQ(value, "100"); + countersTable.hget(expectedKey, "SAI_PORT_STAT_IF_IN_ERRORS", value); + EXPECT_EQ(value, "200"); + + // Verify m_failedPolls is cleaned up on removeObject so the + // re-added object can poll fresh without stale failing state. + // Remove and re-add with getStats succeeding + fc.removeCounter(oid); + countersTable.del(expectedKey); + EXPECT_TRUE(fc.isEmpty()); + + failGetStats = false; + + fc.addCounter(oid, oid, counterValues); + EXPECT_FALSE(fc.isEmpty()); + + waitForCounterKeys(countersTable, 1); + waitForCounterValues(countersTable, expectedKey, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_ERRORS"}, + {"100", "200"}); + + // Cleanup + fc.removeCounter(oid); + countersTable.del(expectedKey); + EXPECT_TRUE(fc.isEmpty()); + + std::vector keys; + countersTable.getKeys(keys); + removeTimeStamp(keys, countersTable); + ASSERT_TRUE(keys.empty()); +} From 99858e509fb672e9030ac0f480b13ebb5237e5ee Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Sat, 4 Jul 2026 00:14:32 +0000 Subject: [PATCH 21/22] Remove duplicated counter set emplacement Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index bed30e6376..66441a62cd 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1319,10 +1319,6 @@ class CounterContext : public BaseCounterContext } else { SWSS_LOG_WARN("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); } - - auto counter_data = std::make_shared>(rid, counter_ids); - m_objectIdsMap.emplace(vid, counter_data); - SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); } return; } From 7627bd9f6eead071991aa077bb5b8c6f496810d9 Mon Sep 17 00:00:00 2001 From: Justin Wong Date: Sat, 4 Jul 2026 01:08:33 +0000 Subject: [PATCH 22/22] func -> function Signed-off-by: Justin Wong --- syncd/FlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 66441a62cd..11f3e4e5c2 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1440,7 +1440,7 @@ class CounterContext : public BaseCounterContext m_counterGroupsSorted.push_back(makeCounterGroupRef(idx, m_supportedCounterGroups[idx].size())); // Must adhere to strict weak ordering - // Use lambda instead of static func to avoid SWSS_LOG_ENTER CI build requirement for better performance + // Use lambda instead of a static function to avoid SWSS_LOG_ENTER CI build requirement for better performance std::sort(m_counterGroupsSorted.begin(), m_counterGroupsSorted.end(), [](CounterGroupRef const& lhs, CounterGroupRef const& rhs) {