From a75a5ed4783e0eb7b2bae31da95e0d009d060aa8 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 27 May 2024 04:42:07 +0300 Subject: [PATCH 01/32] Support bulk step 1: deserialize bulk but handle it in single mode Signed-off-by: Stephen Sun --- syncd/Syncd.cpp | 67 ++++++++++++++++++++++++++----------------------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index fb82d9878..2e831c09e 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2791,49 +2791,54 @@ sai_status_t Syncd::processFlexCounterEvent( } auto groupName = key.substr(0, delimiter); - auto strVid = key.substr(delimiter + 1); + auto strVids = key.substr(delimiter + 1); + auto vidStringVector = swss::tokenize(strVids, ','); - auto effective_op = op; + for(auto &strVid : vidStringVector) + { + auto effective_op = op; + auto singleKey = groupName + ":" + strVid; - sai_object_id_t vid; - sai_deserialize_object_id(strVid, vid); + sai_object_id_t vid; + sai_deserialize_object_id(strVid, vid); - sai_object_id_t rid; + sai_object_id_t rid; - if (!m_translator->tryTranslateVidToRid(vid, rid)) - { - if (fromAsicChannel) + if (!m_translator->tryTranslateVidToRid(vid, rid)) { - SWSS_LOG_ERROR("port VID %s, was not found (probably port was removed/splitted) and will remove from counters now", - sai_serialize_object_id(vid).c_str()); + if (fromAsicChannel) + { + SWSS_LOG_ERROR("port VID %s, was not found (probably port was removed/splitted) and will remove from counters now", + sai_serialize_object_id(vid).c_str()); + } + else + { + SWSS_LOG_WARN("port VID %s, was not found (probably port was removed/splitted) and will remove from counters now", + sai_serialize_object_id(vid).c_str()); + } + effective_op = DEL_COMMAND; } - else + + if (effective_op == SET_COMMAND) { - SWSS_LOG_WARN("port VID %s, was not found (probably port was removed/splitted) and will remove from counters now", - sai_serialize_object_id(vid).c_str()); + m_manager->addCounter(vid, rid, groupName, values); + if (fromAsicChannel) + { + m_flexCounterTable->set(singleKey, values); + } } - effective_op = DEL_COMMAND; - } - - if (effective_op == SET_COMMAND) - { - m_manager->addCounter(vid, rid, groupName, values); - if (fromAsicChannel) + else if (effective_op == DEL_COMMAND) { - m_flexCounterTable->set(key, values); + if (fromAsicChannel) + { + m_flexCounterTable->del(singleKey); + } + m_manager->removeCounter(vid, groupName); } - } - else if (effective_op == DEL_COMMAND) - { - if (fromAsicChannel) + else { - m_flexCounterTable->del(key); + SWSS_LOG_ERROR("unknown command: %s", op.c_str()); } - m_manager->removeCounter(vid, groupName); - } - else - { - SWSS_LOG_ERROR("unknown command: %s", op.c_str()); } if (fromAsicChannel) From 4098581b9ac5180119bb5ce1146a4556caf8486d Mon Sep 17 00:00:00 2001 From: Junchao-Mellanox Date: Mon, 27 May 2024 12:11:07 +0300 Subject: [PATCH 02/32] Support bulk add flex counter --- syncd/FlexCounter.cpp | 307 ++++++++++++++++++++++++++++++++++++++++++ syncd/FlexCounter.h | 12 ++ 2 files changed, 319 insertions(+) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index c742c1688..090bdc07a 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -823,6 +823,114 @@ class CounterContext : public BaseCounterContext } } + virtual void bulkAddObject( + _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 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); + } + + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + sai_stat_capability_list_t stats_capability; + auto status = queryObjectSupportedCounters(rid, stats_capability); + if (status != SAI_STATUS_SUCCESS) + { + // Fall back to old way + addObject(vid, rid, idStrings, per_object_stats_mode); + continue; + } + + if (stats_capability.count == 0) + { + SWSS_LOG_NOTICE("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); + continue; + } + + std::unordered_map stats_cap_map; + for (size_t j = 0; j < stats_capability.count; j++) + { + stats_cap_map.emplace(stats_capability.list[j].stat_enum, stats_capability.list[j].stat_modes); + } + + bool supportBulk = HasStatsMode::value; + std::vector supportedIds; + for (auto &counter : counter_ids) + { + auto iter = stats_cap_map.find(counter); + if (iter != stats_cap_map.end()) + { + if (effective_stats_mode & iter->second) + { + supportedIds.push_back(counter); + } + + auto bulkMode = effective_stats_mode == SAI_STATS_MODE_READ ? SAI_STATS_MODE_BULK_READ : SAI_STATS_MODE_BULK_READ_AND_CLEAR; + if ((bulkMode & iter->second) == 0) + { + supportBulk = false; + } + } + } + + // Perform a remove and re-add to simplify the logic here + removeObject(vid, false); + + 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 + { + std::sort(supportedIds.begin(), supportedIds.end()); + auto bulkContext = getBulkStatsContext(supportedIds); + addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); + } + } + } + void removeObject( _In_ sai_object_id_t vid) override { @@ -1229,6 +1337,40 @@ class CounterContext : public BaseCounterContext } } + sai_status_t queryObjectSupportedCounters( + _In_ sai_object_id_t rid, + _Out_ sai_stat_capability_list_t &stats_capability) + { + SWSS_LOG_ENTER(); + stats_capability.count = 0; + stats_capability.list = nullptr; + + /* First call is to check the size needed to allocate */ + sai_status_t status = m_vendorSai->queryObjectStatsCapability( + rid, + m_objectType, + &stats_capability); + + /* Second call is for query statistics capability */ + if (status == SAI_STATUS_BUFFER_OVERFLOW) + { + std::vector statCapabilityList(stats_capability.count); + stats_capability.list = statCapabilityList.data(); + status = m_vendorSai->queryObjectStatsCapability( + rid, + m_objectType, + &stats_capability); + + if (status != SAI_STATUS_SUCCESS) + { + SWSS_LOG_INFO("Unable to get %s supported counters for %s", + m_name.c_str(), + sai_serialize_object_id(rid).c_str()); + } + } + return status; + } + sai_status_t querySupportedCounters( _In_ sai_object_id_t rid, _In_ sai_stats_mode_t stats_mode) @@ -2618,6 +2760,171 @@ void FlexCounter::addCounter( notifyPoll(); } +void FlexCounter::bulkAddCounter( + _In_ sai_object_type_t objectType, + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& values) +{ + MUTEX; + + SWSS_LOG_ENTER(); + + std::vector counterIds; + + std::string statsMode; + + for (const auto& valuePair: values) + { + const auto field = fvField(valuePair); + const auto value = fvValue(valuePair); + + auto idStrings = swss::tokenize(value, ','); + + if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_PORT)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_DEBUG_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_PORT_DEBUG)->bulkAddObject( + vid, + rid, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_QUEUE)->bulkAddObject( + vids, + rids, + idStrings, + ""); + + } + else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_ATTR_ID_LIST) + { + getCounterContext(ATTR_TYPE_QUEUE)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_PG)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_ATTR_ID_LIST) + { + getCounterContext(ATTR_TYPE_PG)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_ROUTER_INTERFACE && field == RIF_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_RIF)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_SWITCH && field == SWITCH_DEBUG_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_SWITCH_DEBUG)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_MACSEC_FLOW && field == MACSEC_FLOW_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_MACSEC_FLOW)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_MACSEC_SA)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_ATTR_ID_LIST) + { + getCounterContext(ATTR_TYPE_MACSEC_SA)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_ACL_COUNTER && field == ACL_COUNTER_ATTR_ID_LIST) + { + getCounterContext(ATTR_TYPE_ACL_COUNTER)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_COUNTER && field == FLOW_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_FLOW)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == BUFFER_POOL_COUNTER_ID_LIST) + { + counterIds = idStrings; + } + else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == STATS_MODE_FIELD) + { + statsMode = value; + } + else if (objectType == SAI_OBJECT_TYPE_TUNNEL && field == TUNNEL_COUNTER_ID_LIST) + { + getCounterContext(COUNTER_TYPE_TUNNEL)->bulkAddObject( + vids, + rids, + idStrings, + ""); + } + else + { + SWSS_LOG_ERROR("Object type and field combination is not supported, object type %s, field %s", + sai_serialize_object_type(objectType).c_str(), + field.c_str()); + } + } + + // outside loop since required 2 fields BUFFER_POOL_COUNTER_ID_LIST and STATS_MODE_FIELD + + if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && counterIds.size()) + { + getCounterContext(COUNTER_TYPE_BUFFER_POOL)->bulkAddObject( + vids, + rids, + counterIds, + statsMode); + } + + // notify thread to start polling + notifyPoll(); +} + void FlexCounter::waitPoll() { SWSS_LOG_ENTER(); diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index 9053046bf..132fd3237 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -43,6 +43,12 @@ namespace syncd _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, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) = 0; + virtual void removeObject( _In_ sai_object_id_t vid) = 0; @@ -95,6 +101,12 @@ namespace syncd _In_ sai_object_id_t rid, _In_ const std::vector& values); + void bulkAddCounter( + _In_ sai_object_type_t objectType, + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& values); + void removeCounter( _In_ sai_object_id_t vid); From 56d2977c67bdb2760b835bd37be002b8618d09b2 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 27 May 2024 14:14:05 +0000 Subject: [PATCH 03/32] Fix compiling errors Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 090bdc07a..463642882 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -869,6 +869,7 @@ class CounterContext : public BaseCounterContext auto rid = rids[i]; auto vid = vids[i]; sai_stat_capability_list_t stats_capability; + auto status = queryObjectSupportedCounters(rid, stats_capability); if (status != SAI_STATUS_SUCCESS) { @@ -886,7 +887,7 @@ class CounterContext : public BaseCounterContext std::unordered_map stats_cap_map; for (size_t j = 0; j < stats_capability.count; j++) { - stats_cap_map.emplace(stats_capability.list[j].stat_enum, stats_capability.list[j].stat_modes); + stats_cap_map.emplace(StatType(stats_capability.list[j].stat_enum), stats_capability.list[j].stat_modes); } bool supportBulk = HasStatsMode::value; @@ -1345,9 +1346,13 @@ class CounterContext : public BaseCounterContext stats_capability.count = 0; stats_capability.list = nullptr; + sai_object_key_t key; + key.key.object_id = rid; + /* First call is to check the size needed to allocate */ - sai_status_t status = m_vendorSai->queryObjectStatsCapability( + sai_status_t status = reinterpret_cast(m_vendorSai)->queryObjectStatsCapability( rid, + key, m_objectType, &stats_capability); @@ -1356,8 +1361,9 @@ class CounterContext : public BaseCounterContext { std::vector statCapabilityList(stats_capability.count); stats_capability.list = statCapabilityList.data(); - status = m_vendorSai->queryObjectStatsCapability( + status = reinterpret_cast(m_vendorSai)->queryObjectStatsCapability( rid, + key, m_objectType, &stats_capability); @@ -2792,8 +2798,8 @@ void FlexCounter::bulkAddCounter( else if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_DEBUG_COUNTER_ID_LIST) { getCounterContext(COUNTER_TYPE_PORT_DEBUG)->bulkAddObject( - vid, - rid, + vids, + rids, idStrings, ""); } From 09483f867038f626947c4a67d23ca82658e29de5 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 27 May 2024 14:53:03 +0000 Subject: [PATCH 04/32] Compiling pass Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 87 +++++++++++++++++++++++++------------------ 1 file changed, 50 insertions(+), 37 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 463642882..fc5879185 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -864,57 +864,70 @@ class CounterContext : public BaseCounterContext counter_ids.push_back(stat); } - for (size_t i = 0; i < vids.size(); i++) - { - auto rid = rids[i]; - auto vid = vids[i]; - sai_stat_capability_list_t stats_capability; + updateSupportedCounters(rids[0]/*hacking. it is not really used*/, counter_ids, effective_stats_mode); + vector stats_capabilities(rids.size()); + + bool supportBulk = HasStatsMode::value; + bool fallback = !supportBulk; + + if (supportBulk) + { + BulkContextType ctx; + ctx.object_vids = vids; + transform(rids.begin(), rids.end(), back_inserter(ctx.object_keys), [](sai_object_id_t rid) { + sai_object_key_t key; + key.key.object_id = rid; + return key; + }); + ctx.counter_ids = counter_ids; + ctx.object_statuses = vector(vids.size(), SAI_STATUS_SUCCESS); + ctx.counters.resize(counter_ids.size() * ctx.object_keys.size()); + auto statsMode = m_groupStatsMode == SAI_STATS_MODE_READ ? SAI_STATS_MODE_BULK_READ : SAI_STATS_MODE_BULK_READ_AND_CLEAR; + auto 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()); + fallback = (status != SAI_STATUS_SUCCESS); + } - auto status = queryObjectSupportedCounters(rid, stats_capability); - if (status != SAI_STATUS_SUCCESS) + if (fallback) + { + // Fall back to old way + for (size_t i = 0; i < vids.size(); i++) { - // Fall back to old way + auto rid = rids[i]; + auto vid = vids[i]; addObject(vid, rid, idStrings, per_object_stats_mode); - continue; - } - - if (stats_capability.count == 0) - { - SWSS_LOG_NOTICE("%s RID %s can't provide the statistic", m_name.c_str(), sai_serialize_object_id(rid).c_str()); - continue; } - std::unordered_map stats_cap_map; - for (size_t j = 0; j < stats_capability.count; j++) - { - stats_cap_map.emplace(StatType(stats_capability.list[j].stat_enum), stats_capability.list[j].stat_modes); - } + return; + } - bool supportBulk = HasStatsMode::value; - std::vector supportedIds; - for (auto &counter : counter_ids) + std::vector supportedIds; + for (auto &counter : counter_ids) + { + if (isCounterSupported(counter)) { - auto iter = stats_cap_map.find(counter); - if (iter != stats_cap_map.end()) - { - if (effective_stats_mode & iter->second) - { - supportedIds.push_back(counter); - } - - auto bulkMode = effective_stats_mode == SAI_STATS_MODE_READ ? SAI_STATS_MODE_BULK_READ : SAI_STATS_MODE_BULK_READ_AND_CLEAR; - if ((bulkMode & iter->second) == 0) - { - supportBulk = false; - } - } + supportedIds.push_back(counter); } + } + for (size_t i = 0; i < vids.size(); i++) + { + auto &vid = vids[i]; + auto &rid = rids[i]; // Perform a remove and re-add to simplify the logic here removeObject(vid, false); if (!supportBulk) { + // Unlikely auto counter_data = std::make_shared>(rid, supportedIds); // TODO: use if const expression when cpp17 is supported if (HasStatsMode::value) From 55999b76a22b4a3de3d7e87af25678e205138b76 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 28 May 2024 00:33:21 +0000 Subject: [PATCH 05/32] bulk counter: syncd Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 2 ++ syncd/FlexCounterManager.cpp | 19 +++++++++++++++++++ syncd/FlexCounterManager.h | 6 ++++++ syncd/Syncd.cpp | 27 +++++++++++++++++++++++++++ 4 files changed, 54 insertions(+) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index fc5879185..ccce6ffc4 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2818,6 +2818,7 @@ void FlexCounter::bulkAddCounter( } else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_COUNTER_ID_LIST) { + SWSS_LOG_INFO("BULKCOUNTER bulk add queue"); getCounterContext(COUNTER_TYPE_QUEUE)->bulkAddObject( vids, rids, @@ -2835,6 +2836,7 @@ void FlexCounter::bulkAddCounter( } else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_COUNTER_ID_LIST) { + SWSS_LOG_INFO("BULKCOUNTER bulk add queue"); getCounterContext(COUNTER_TYPE_PG)->bulkAddObject( vids, rids, diff --git a/syncd/FlexCounterManager.cpp b/syncd/FlexCounterManager.cpp index 5088a19c5..ee451d7f7 100644 --- a/syncd/FlexCounterManager.cpp +++ b/syncd/FlexCounterManager.cpp @@ -1,4 +1,5 @@ #include "FlexCounterManager.h" +#include "VidManager.h" #include "swss/logger.h" @@ -100,6 +101,24 @@ void FlexCounterManager::addCounter( } } +void FlexCounterManager::bulkAddCounter( + _In_ const std::vector vids, + _In_ const std::vector rids, + _In_ const std::string& instanceId, + _In_ const std::vector& values) +{ + SWSS_LOG_ENTER(); + + auto fc = getInstance(instanceId); + sai_object_type_t objectType = VidManager::objectTypeQuery(vids[0]); // VID and RID will have the same object type + fc->bulkAddCounter(objectType, vids, rids, values); + + if (fc->isDiscarded()) + { + removeInstance(instanceId); + } +} + void FlexCounterManager::removeCounter( _In_ sai_object_id_t vid, _In_ const std::string& instanceId) diff --git a/syncd/FlexCounterManager.h b/syncd/FlexCounterManager.h index 5ba2f9992..687d4260c 100644 --- a/syncd/FlexCounterManager.h +++ b/syncd/FlexCounterManager.h @@ -38,6 +38,12 @@ namespace syncd _In_ const std::string& instanceId, _In_ const std::vector& values); + void bulkAddCounter( + _In_ const std::vector vids, + _In_ const std::vector rids, + _In_ const std::string& instanceId, + _In_ const std::vector& values); + void removeCounter( _In_ sai_object_id_t vid, _In_ const std::string& instanceId); diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 2e831c09e..69b81d189 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2793,7 +2793,34 @@ sai_status_t Syncd::processFlexCounterEvent( auto groupName = key.substr(0, delimiter); auto strVids = key.substr(delimiter + 1); auto vidStringVector = swss::tokenize(strVids, ','); +#if 1 + if (fromAsicChannel && op == SET_COMMAND) + { + std::vector vids(vidStringVector.size()); + std::vector rids(vidStringVector.size()); + + for (auto &strVid: vidStringVector) + { + sai_object_id_t vid, rid; + sai_deserialize_object_id(strVid, vid); + vids.emplace_back(vid); + + if (!m_translator->tryTranslateVidToRid(vid, rid)) + { + SWSS_LOG_ERROR("port VID %s, was not found (probably port was removed/splitted) and will remove from counters now", + sai_serialize_object_id(vid).c_str()); + } + rids.emplace_back(rid); + } + + m_manager->bulkAddCounter(vids, rids, groupName, values); + + m_flexCounterTable->set(key, values); + + return SAI_STATUS_SUCCESS; + } +#endif for(auto &strVid : vidStringVector) { auto effective_op = op; From 0451b9de03d53458e1d774c7e40696bf5e274935 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 28 May 2024 03:29:51 +0000 Subject: [PATCH 06/32] Working solution Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 8 ++++---- syncd/FlexCounterManager.cpp | 1 + syncd/Syncd.cpp | 15 ++++++++++----- 3 files changed, 15 insertions(+), 9 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index ccce6ffc4..1fa5b26d7 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -867,8 +867,8 @@ class CounterContext : public BaseCounterContext updateSupportedCounters(rids[0]/*hacking. it is not really used*/, counter_ids, effective_stats_mode); vector stats_capabilities(rids.size()); - bool supportBulk = HasStatsMode::value; - bool fallback = !supportBulk; + bool fallback = HasStatsMode::value; + bool supportBulk = !fallback; if (supportBulk) { @@ -898,6 +898,8 @@ class CounterContext : public BaseCounterContext if (fallback) { + SWSS_LOG_NOTICE("BULKCOUNTER Fallback to single init"); + // Fall back to old way for (size_t i = 0; i < vids.size(); i++) { @@ -2818,7 +2820,6 @@ void FlexCounter::bulkAddCounter( } else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_COUNTER_ID_LIST) { - SWSS_LOG_INFO("BULKCOUNTER bulk add queue"); getCounterContext(COUNTER_TYPE_QUEUE)->bulkAddObject( vids, rids, @@ -2836,7 +2837,6 @@ void FlexCounter::bulkAddCounter( } else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_COUNTER_ID_LIST) { - SWSS_LOG_INFO("BULKCOUNTER bulk add queue"); getCounterContext(COUNTER_TYPE_PG)->bulkAddObject( vids, rids, diff --git a/syncd/FlexCounterManager.cpp b/syncd/FlexCounterManager.cpp index ee451d7f7..b02bce2cc 100644 --- a/syncd/FlexCounterManager.cpp +++ b/syncd/FlexCounterManager.cpp @@ -111,6 +111,7 @@ void FlexCounterManager::bulkAddCounter( auto fc = getInstance(instanceId); sai_object_type_t objectType = VidManager::objectTypeQuery(vids[0]); // VID and RID will have the same object type + fc->bulkAddCounter(objectType, vids, rids, values); if (fc->isDiscarded()) diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 69b81d189..441a9fd3e 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2793,11 +2793,11 @@ sai_status_t Syncd::processFlexCounterEvent( auto groupName = key.substr(0, delimiter); auto strVids = key.substr(delimiter + 1); auto vidStringVector = swss::tokenize(strVids, ','); -#if 1 - if (fromAsicChannel && op == SET_COMMAND) + + if (fromAsicChannel && op == SET_COMMAND && vidStringVector.size() > 1) { - std::vector vids(vidStringVector.size()); - std::vector rids(vidStringVector.size()); + std::vector vids; + std::vector rids; for (auto &strVid: vidStringVector) { @@ -2818,9 +2818,14 @@ sai_status_t Syncd::processFlexCounterEvent( m_flexCounterTable->set(key, values); + if (fromAsicChannel) + { + sendApiResponse(SAI_COMMON_API_SET, SAI_STATUS_SUCCESS); + } + return SAI_STATUS_SUCCESS; } -#endif + for(auto &strVid : vidStringVector) { auto effective_op = op; From 4ea9f7437077bd64e272b1ace645250a8e618b20 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 13 Dec 2024 03:34:31 +0200 Subject: [PATCH 07/32] Fix compiling errors Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 1fa5b26d7..62e39eeb9 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -941,7 +941,7 @@ class CounterContext : public BaseCounterContext else { std::sort(supportedIds.begin(), supportedIds.end()); - auto bulkContext = getBulkStatsContext(supportedIds); + auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); } } @@ -1692,6 +1692,15 @@ class DashMeterCounterContext : public BaseCounterContext } } + void bulkAddObject( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) override + { + SWSS_LOG_ENTER(); + } + bool hasObject() const override { SWSS_LOG_ENTER(); From c092ac263fc0147882d01da7050f02452866062a Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 20 Dec 2024 07:55:39 +0000 Subject: [PATCH 08/32] Bulk counter init + bulk chunk size Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 61 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 62e39eeb9..d2cc6fad3 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -898,7 +898,7 @@ class CounterContext : public BaseCounterContext if (fallback) { - SWSS_LOG_NOTICE("BULKCOUNTER Fallback to single init"); + SWSS_LOG_NOTICE("All objects do not support bulk. Fallback to single call"); // Fall back to old way for (size_t i = 0; i < vids.size(); i++) @@ -920,15 +920,15 @@ class CounterContext : public BaseCounterContext } } - for (size_t i = 0; i < vids.size(); i++) + if (!supportBulk) { - auto &vid = vids[i]; - auto &rid = rids[i]; - // Perform a remove and re-add to simplify the logic here - removeObject(vid, false); - - if (!supportBulk) + for (size_t i = 0; i < vids.size(); i++) { + auto &vid = vids[i]; + auto &rid = rids[i]; + // Perform a remove and re-add to simplify the logic here + removeObject(vid, false); + // Unlikely auto counter_data = std::make_shared>(rid, supportedIds); // TODO: use if const expression when cpp17 is supported @@ -938,13 +938,52 @@ class CounterContext : public BaseCounterContext } m_objectIdsMap.emplace(vid, counter_data); } - else + } + else if (m_counterChunkSizeMapFromPrefix.empty()) + { + std::sort(supportedIds.begin(), supportedIds.end()); + auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); + + for (size_t i = 0; i < vids.size(); i++) { - std::sort(supportedIds.begin(), supportedIds.end()); - auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); + auto &vid = vids[i]; + auto &rid = rids[i]; + // Perform a remove and re-add to simplify the logic here + removeObject(vid, false); + addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); } } + else + { + std::map> counter_prefix_map; + std::vector default_partition; + mapCountersByPrefix(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 (size_t i = 0; i < vids.size(); i++) + { + auto &vid = vids[i]; + auto &rid = rids[i]; + // Perform a remove and re-add to simplify the logic here + removeObject(vid, false); + + for (auto &counterPrefix : counter_prefix_map) + { + auto bulkContext = getBulkStatsContext(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); + addBulkStatsContext(vid, rid, counterPrefix.second, *bulkContext.get()); + } + + auto bulkContext = getBulkStatsContext(default_partition, "default", default_bulk_chunk_size); + addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); + } + } } void removeObject( From 4751d5925e2fd5ff25620a9b8202ff31e88e00ae Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 22 Dec 2024 06:50:18 +0200 Subject: [PATCH 09/32] Support addBulkStatsContext using vector Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 71 ++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 28 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index d2cc6fad3..33ce3bda5 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -939,49 +939,42 @@ class CounterContext : public BaseCounterContext m_objectIdsMap.emplace(vid, counter_data); } } - else if (m_counterChunkSizeMapFromPrefix.empty()) + else { - std::sort(supportedIds.begin(), supportedIds.end()); - auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); - - for (size_t i = 0; i < vids.size(); i++) + // Perform a remove and re-add to simplify the logic here + for (auto vid: vids) { - auto &vid = vids[i]; - auto &rid = rids[i]; - // Perform a remove and re-add to simplify the logic here removeObject(vid, false); - - addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); } - } - else - { - std::map> counter_prefix_map; - std::vector default_partition; - mapCountersByPrefix(supportedIds, counter_prefix_map, default_partition); - for (auto &counterPrefix : counter_prefix_map) + if (m_counterChunkSizeMapFromPrefix.empty()) { - std::sort(counterPrefix.second.begin(), counterPrefix.second.end()); - } + std::sort(supportedIds.begin(), supportedIds.end()); + auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); - std::sort(default_partition.begin(), default_partition.end()); - - for (size_t i = 0; i < vids.size(); i++) + addBulkStatsContext(vids, rids, supportedIds, *bulkContext.get()); + } + else { - auto &vid = vids[i]; - auto &rid = rids[i]; - // Perform a remove and re-add to simplify the logic here - removeObject(vid, false); + std::map> counter_prefix_map; + std::vector default_partition; + mapCountersByPrefix(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) { auto bulkContext = getBulkStatsContext(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); - addBulkStatsContext(vid, rid, counterPrefix.second, *bulkContext.get()); + addBulkStatsContext(vids, rids, counterPrefix.second, *bulkContext.get()); } auto bulkContext = getBulkStatsContext(default_partition, "default", default_bulk_chunk_size); - addBulkStatsContext(vid, rid, supportedIds, *bulkContext.get()); + addBulkStatsContext(vids, rids, default_partition, *bulkContext.get()); } } } @@ -1295,6 +1288,28 @@ class CounterContext : public BaseCounterContext ctx.counters.resize(counterIds.size() * ctx.object_keys.size()); } + void addBulkStatsContext( + _In_ const std::vector &vids, + _In_ const std::vector &rids, + _In_ const std::vector& counterIds, + _Inout_ BulkContextType &ctx) + { + SWSS_LOG_ENTER(); + ctx.object_vids.insert(ctx.object_vids.end(), vids.begin(), vids.end()); + + ctx.object_keys.reserve(ctx.object_keys.size() + vids.size()); + ctx.object_statuses.reserve(ctx.object_statuses.size() + vids.size()); + for (auto rid : rids) + { + sai_object_key_t object_key; + object_key.key.object_id = rid; + ctx.object_keys.push_back(object_key); + ctx.object_statuses.push_back(SAI_STATUS_SUCCESS); + } + + ctx.counters.resize(counterIds.size() * ctx.object_keys.size()); + } + bool removeBulkStatsContext( _In_ sai_object_id_t vid) { From 254eff26f6a9a258a06b59cb7285560384a3b92b Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 23 Dec 2024 04:04:49 +0000 Subject: [PATCH 10/32] Support test for bulk counter init Signed-off-by: Stephen Sun --- unittest/syncd/TestFlexCounter.cpp | 54 +++++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 4 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 9b70e314b..665ca9c5c 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -86,6 +86,7 @@ void testAddRemoveCounter( VerifyStatsFunc verifyFunc, bool autoRemoveDbEntry, const std::string statsMode = STATS_MODE_READ, + bool bulkAdd = false, const std::string bulkChunkSize = "", const std::string bulkChunkSizePerCounter = "", bool bulkChunkSizeAfterPort = true, @@ -123,9 +124,17 @@ void testAddRemoveCounter( values.clear(); values.emplace_back(counterIdFieldName, join(counterIdNames)); - for (auto object_id : object_ids) + + if (bulkAdd) { - fc.addCounter(object_id, object_id, values); + fc.bulkAddCounter(SAI_OBJECT_TYPE_PORT, object_ids, object_ids, values); + } + else + { + for (auto object_id : object_ids) + { + fc.addCounter(object_id, object_id, values); + } } if (bulkChunkSizeAfterPort) @@ -856,6 +865,7 @@ TEST(FlexCounter, bulkChunksize) std::vector> counterRecord; std::vector> valueRecord; sai_uint64_t counterSeed = 0; + uint32_t expectedInitObjectCount; uint32_t unifiedBulkChunkSize = 0; sai->mock_bulkGetStats = [&](sai_object_id_t, sai_object_type_t, @@ -870,7 +880,7 @@ TEST(FlexCounter, bulkChunksize) EXPECT_TRUE(mode == SAI_STATS_MODE_BULK_READ); std::vector record; std::vector value; - if (number_of_counters >= 5 && object_count == 1) + if (number_of_counters >= 5 && object_count == expectedInitObjectCount) { allObjectIds.insert(toOid(object_keys[0].key.object_id)); // This call is to check whether bulk counter polling is supported during initialization @@ -945,6 +955,7 @@ TEST(FlexCounter, bulkChunksize) allObjectIds.erase(key); }; + expectedInitObjectCount = 1; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -954,6 +965,7 @@ TEST(FlexCounter, bulkChunksize) counterVerifyFunc, false, STATS_MODE_READ, + false, "3", "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); @@ -967,11 +979,12 @@ TEST(FlexCounter, bulkChunksize) counterVerifyFunc, false, STATS_MODE_READ, + false, "3", "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", false, PORT_PLUGIN_FIELD); - EXPECT_TRUE(allObjectIds.empty()); + EXPECT_TRUE(allObjectIds.empty()); unifiedBulkChunkSize = 3; testAddRemoveCounter( @@ -983,12 +996,45 @@ TEST(FlexCounter, bulkChunksize) counterVerifyFunc, false, STATS_MODE_READ, + false, "3", "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", true, "", true); EXPECT_TRUE(allObjectIds.empty()); + unifiedBulkChunkSize = 0; + + expectedInitObjectCount = 6; + testAddRemoveCounter( + 6, + SAI_OBJECT_TYPE_PORT, + PORT_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS", "SAI_PORT_STAT_IF_OUT_QLEN", "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES"}, + {}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true, + "3", + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); + EXPECT_TRUE(allObjectIds.empty()); + + testAddRemoveCounter( + 6, + SAI_OBJECT_TYPE_PORT, + PORT_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS", "SAI_PORT_STAT_IF_OUT_QLEN", "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES"}, + {}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true, + "3", + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", + false, + PORT_PLUGIN_FIELD); + EXPECT_TRUE(allObjectIds.empty()); } TEST(FlexCounter, counterIdChange) From 22737a9a405b607c23d30d5ad8d3aca331ba0f0a Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 24 Dec 2024 09:40:41 +0000 Subject: [PATCH 11/32] Optimize addBulkStatsContext Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 33ce3bda5..eca6f6bb0 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1296,17 +1296,12 @@ class CounterContext : public BaseCounterContext { SWSS_LOG_ENTER(); ctx.object_vids.insert(ctx.object_vids.end(), vids.begin(), vids.end()); - - ctx.object_keys.reserve(ctx.object_keys.size() + vids.size()); - ctx.object_statuses.reserve(ctx.object_statuses.size() + vids.size()); - for (auto rid : rids) - { - sai_object_key_t object_key; - object_key.key.object_id = rid; - ctx.object_keys.push_back(object_key); - ctx.object_statuses.push_back(SAI_STATUS_SUCCESS); - } - + transform(rids.begin(), rids.end(), back_inserter(ctx.object_keys), [](sai_object_id_t rid) { + sai_object_key_t key; + key.key.object_id = rid; + return key; + }); + ctx.object_statuses.insert(ctx.object_statuses.end(), vids.size(), SAI_STATUS_SUCCESS); ctx.counters.resize(counterIds.size() * ctx.object_keys.size()); } From 1f4796f7e17e2b19c62c478ad420cf35e6251ff4 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 24 Dec 2024 09:42:01 +0000 Subject: [PATCH 12/32] Insert entry with single OID as key into FLEX_COUNTER_DB Signed-off-by: Stephen Sun --- syncd/Syncd.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 441a9fd3e..724a8d232 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2796,8 +2796,9 @@ sai_status_t Syncd::processFlexCounterEvent( if (fromAsicChannel && op == SET_COMMAND && vidStringVector.size() > 1) { - std::vector vids; - std::vector rids; + std::vector vids(vidStringVector.size()); + std::vector rids(vidStringVector.size()); + std::vector keys(vidStringVector.size()); for (auto &strVid: vidStringVector) { @@ -2812,11 +2813,15 @@ sai_status_t Syncd::processFlexCounterEvent( } rids.emplace_back(rid); + keys.emplace_back(groupName + ":" + strVid); } m_manager->bulkAddCounter(vids, rids, groupName, values); - m_flexCounterTable->set(key, values); + for (auto &singleKey: keys) + { + m_flexCounterTable->set(singleKey, values); + } if (fromAsicChannel) { From df95629c674a467c59234fbc789158793245c770 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 24 Dec 2024 09:42:44 +0000 Subject: [PATCH 13/32] Cover bulk counter initialization Signed-off-by: Stephen Sun --- syncd/tests/TestSyncdMlnx.cpp | 80 +++++++++++++++++++++++++++-------- 1 file changed, 62 insertions(+), 18 deletions(-) diff --git a/syncd/tests/TestSyncdMlnx.cpp b/syncd/tests/TestSyncdMlnx.cpp index 3c0c852c6..51443b1c2 100644 --- a/syncd/tests/TestSyncdMlnx.cpp +++ b/syncd/tests/TestSyncdMlnx.cpp @@ -222,29 +222,47 @@ TEST_F(SyncdMlnxTest, queryAttrEnumValuesCapability) TEST_F(SyncdMlnxTest, portBulkAddRemove) { - const std::uint32_t portCount = 1; + const std::uint32_t portCount = 4; const std::uint32_t laneCount = 4; // Generate port config - std::array laneList = { 1000, 1001, 1002, 1003 }; + std::array, portCount> laneLists/* = { + { 1000, 1001, 1002, 1003 }, + { 1004, 1005, 1006, 1007 }, + { 1008, 1009, 1010, 1011 }, + { 1012, 1013, 1014, 1015 } + }*/; sai_attribute_t attr; - std::vector attrList; + std::array, portCount> attrLists; - attr.id = SAI_PORT_ATTR_HW_LANE_LIST; - attr.value.u32list.count = static_cast(laneList.size()); - attr.value.u32list.list = laneList.data(); - attrList.push_back(attr); + std::array attrCountList;// = { static_cast(attrList.size()) }; + std::array attrPtrList;// = { attrList.data() }; - attr.id = SAI_PORT_ATTR_SPEED; - attr.value.u32 = 1000; - attrList.push_back(attr); - - std::array attrCountList = { static_cast(attrList.size()) }; - std::array attrPtrList = { attrList.data() }; + uint32_t lane = 1000; + for (auto i = 0u; i < portCount; i++) + { + auto &laneList = laneLists[i]; + auto &attrList = attrLists[i]; + for (auto j = 0u; j < laneCount; j++) + { + laneList.push_back(lane++); + } + attr.id = SAI_PORT_ATTR_HW_LANE_LIST; + attr.value.u32list.count = static_cast(laneList.size()); + attr.value.u32list.list = laneList.data(); + attrList.push_back(attr); + + attr.id = SAI_PORT_ATTR_SPEED; + attr.value.u32 = 1000; + attrList.push_back(attr); + + attrPtrList[i] = attrList.data(); + attrCountList[i] = static_cast(attrList.size()); + } - std::array oidList = { SAI_NULL_OBJECT_ID }; - std::array statusList = { SAI_STATUS_SUCCESS }; + std::vector oidList(portCount, SAI_NULL_OBJECT_ID); + std::vector statusList(portCount, SAI_STATUS_SUCCESS); // Validate port bulk add auto status = m_sairedis->bulkCreate( @@ -334,15 +352,41 @@ TEST_F(SyncdMlnxTest, portBulkAddRemove) fvVectorExpected.emplace_back(counter_field_name, counters); ASSERT_EQ(fvVectorExpected, fvVector); + // Try with bulk initialization + key = "PORT_STAT_COUNTER:"; + for (auto i = 1u; i < portCount; i++) + { + key += sai_serialize_object_id(oidList[i]) + ","; + } + key.pop_back(); + + flexCounterParam.counter_key.list = (int8_t*)const_cast(key.c_str()); + flexCounterParam.counter_key.count = (uint32_t)key.length(); + status = m_sairedis->set(SAI_OBJECT_TYPE_SWITCH, m_switchId, &attr); + ASSERT_EQ(status, SAI_STATUS_SUCCESS); + + for (auto i = 1u; i < portCount; i++) + { + key = "PORT_STAT_COUNTER:" + sai_serialize_object_id(oidList[i]); + ASSERT_TRUE(m_flexCounterTable->get(key, fvVector)); + ASSERT_EQ(fvVectorExpected, fvVector); + } + flexCounterParam.counter_ids.list = nullptr; flexCounterParam.counter_ids.count = 0; flexCounterParam.counter_field_name.list = nullptr; flexCounterParam.counter_field_name.count = 0; // 3. Stop counter polling for the port - status = m_sairedis->set(SAI_OBJECT_TYPE_SWITCH, m_switchId, &attr); - ASSERT_EQ(status, SAI_STATUS_SUCCESS); - ASSERT_FALSE(m_flexCounterTable->get(key, fvVector)); + for (auto i = 0u; i < portCount; i++) + { + key = "PORT_STAT_COUNTER:" + sai_serialize_object_id(oidList[i]); + flexCounterParam.counter_key.list = (int8_t*)const_cast(key.c_str()); + flexCounterParam.counter_key.count = (uint32_t)key.length(); + status = m_sairedis->set(SAI_OBJECT_TYPE_SWITCH, m_switchId, &attr); + ASSERT_EQ(status, SAI_STATUS_SUCCESS); + ASSERT_FALSE(m_flexCounterTable->get(key, fvVector)); + } // 4. Disable counter polling for the group flexCounterGroupParam.poll_interval.list = nullptr; From d6ac21d62d2bb8cd8f2efcb4c4f727be543d597e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 24 Dec 2024 10:03:43 +0000 Subject: [PATCH 14/32] Remove non-used function queryObjectSupportedCounters Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 39 --------------------------------------- 1 file changed, 39 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index eca6f6bb0..e337af6cd 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1402,45 +1402,6 @@ class CounterContext : public BaseCounterContext } } - sai_status_t queryObjectSupportedCounters( - _In_ sai_object_id_t rid, - _Out_ sai_stat_capability_list_t &stats_capability) - { - SWSS_LOG_ENTER(); - stats_capability.count = 0; - stats_capability.list = nullptr; - - sai_object_key_t key; - key.key.object_id = rid; - - /* First call is to check the size needed to allocate */ - sai_status_t status = reinterpret_cast(m_vendorSai)->queryObjectStatsCapability( - rid, - key, - m_objectType, - &stats_capability); - - /* Second call is for query statistics capability */ - if (status == SAI_STATUS_BUFFER_OVERFLOW) - { - std::vector statCapabilityList(stats_capability.count); - stats_capability.list = statCapabilityList.data(); - status = reinterpret_cast(m_vendorSai)->queryObjectStatsCapability( - rid, - key, - m_objectType, - &stats_capability); - - if (status != SAI_STATUS_SUCCESS) - { - SWSS_LOG_INFO("Unable to get %s supported counters for %s", - m_name.c_str(), - sai_serialize_object_id(rid).c_str()); - } - } - return status; - } - sai_status_t querySupportedCounters( _In_ sai_object_id_t rid, _In_ sai_stats_mode_t stats_mode) From 220645172f9fc090d6c0396c7ace2169bb66687c Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 24 Dec 2024 10:07:47 +0000 Subject: [PATCH 15/32] Remove unused code Signed-off-by: Stephen Sun --- syncd/tests/TestSyncdMlnx.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/syncd/tests/TestSyncdMlnx.cpp b/syncd/tests/TestSyncdMlnx.cpp index 51443b1c2..ed94b33be 100644 --- a/syncd/tests/TestSyncdMlnx.cpp +++ b/syncd/tests/TestSyncdMlnx.cpp @@ -226,18 +226,11 @@ TEST_F(SyncdMlnxTest, portBulkAddRemove) const std::uint32_t laneCount = 4; // Generate port config - std::array, portCount> laneLists/* = { - { 1000, 1001, 1002, 1003 }, - { 1004, 1005, 1006, 1007 }, - { 1008, 1009, 1010, 1011 }, - { 1012, 1013, 1014, 1015 } - }*/; - sai_attribute_t attr; + std::array, portCount> laneLists; std::array, portCount> attrLists; - - std::array attrCountList;// = { static_cast(attrList.size()) }; - std::array attrPtrList;// = { attrList.data() }; + std::array attrCountList; + std::array attrPtrList; uint32_t lane = 1000; for (auto i = 0u; i < portCount; i++) From c124814f7547c15c3f0fb0f443ee126242d6f074 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 31 Dec 2024 03:53:28 +0200 Subject: [PATCH 16/32] Minor changes 1. add more comments 2. change function name 3. reduce log severity to DEBUG for counter polling to avoid rate limit Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index e337af6cd..6e976a1e1 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -958,7 +958,7 @@ class CounterContext : public BaseCounterContext { std::map> counter_prefix_map; std::vector default_partition; - mapCountersByPrefix(supportedIds, counter_prefix_map, default_partition); + createCounterBulkChunkSizePerPrefixPartition(supportedIds, counter_prefix_map, default_partition); for (auto &counterPrefix : counter_prefix_map) { @@ -1190,7 +1190,7 @@ class CounterContext : public BaseCounterContext } uint32_t current = 0; - SWSS_LOG_INFO("Before getting bulk %s %s %s size %u bulk chunk size %u current %u", m_instanceId.c_str(), m_name.c_str(), ctx.name.c_str(), size, bulk_chunk_size, current); + SWSS_LOG_DEBUG("Before getting bulk %s %s %s size %u bulk chunk size %u current %u", m_instanceId.c_str(), m_name.c_str(), ctx.name.c_str(), size, bulk_chunk_size, current); while (current < size) { From c7bc1b67cc56e55180ec778ed159dc18f341022f Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 6 Jan 2025 01:36:35 +0000 Subject: [PATCH 17/32] Simplify the logic Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 159 ++++++++++++++++++++---------------------- 1 file changed, 76 insertions(+), 83 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 6e976a1e1..1edf22ec1 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -856,33 +856,47 @@ class CounterContext : public BaseCounterContext effective_stats_mode = m_groupStatsMode; } - std::vector counter_ids; + if (HasStatsMode::value) + { + // Bulk operation is not supported by the counter group. + SWSS_LOG_NOTICE("Counter group %s does not support bulk. Fallback to single call", m_name.c_str()); + + // Fall back to old way + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + addObject(vid, rid, idStrings, per_object_stats_mode); + } + + return; + } + + std::vector allCounterIds, supportedIds; for (const auto &str : idStrings) { StatType stat; deserializeStat(str.c_str(), &stat); - counter_ids.push_back(stat); + { + allCounterIds.push_back(stat); + } } - updateSupportedCounters(rids[0]/*hacking. it is not really used*/, counter_ids, effective_stats_mode); - vector stats_capabilities(rids.size()); - - bool fallback = HasStatsMode::value; - bool supportBulk = !fallback; + updateSupportedCounters(rids[0]/*it is not really used*/, allCounterIds, effective_stats_mode); + for (auto stat : allCounterIds) + { + if (isCounterSupported(stat)) + { + supportedIds.push_back(stat); + } + } - if (supportBulk) + std::vector 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; - ctx.object_vids = vids; - transform(rids.begin(), rids.end(), back_inserter(ctx.object_keys), [](sai_object_id_t rid) { - sai_object_key_t key; - key.key.object_id = rid; - return key; - }); - ctx.counter_ids = counter_ids; - ctx.object_statuses = vector(vids.size(), SAI_STATUS_SUCCESS); - ctx.counters.resize(counter_ids.size() * ctx.object_keys.size()); - auto statsMode = m_groupStatsMode == SAI_STATS_MODE_READ ? SAI_STATS_MODE_BULK_READ : SAI_STATS_MODE_BULK_READ_AND_CLEAR; + auto bulkContext = getBulkStatsContext(counter_ids, prefix, bulk_chunk_size); + auto &ctx = *bulkContext.get(); auto status = m_vendorSai->bulkGetStats( SAI_NULL_OBJECT_ID, m_objectType, @@ -893,88 +907,67 @@ class CounterContext : public BaseCounterContext statsMode, ctx.object_statuses.data(), ctx.counters.data()); - fallback = (status != SAI_STATUS_SUCCESS); - } - - if (fallback) - { - SWSS_LOG_NOTICE("All objects do not support bulk. Fallback to single call"); - - // Fall back to old way - for (size_t i = 0; i < vids.size(); i++) + if (status == SAI_STATUS_SUCCESS) { - auto rid = rids[i]; - auto vid = vids[i]; - addObject(vid, rid, idStrings, per_object_stats_mode); + addBulkStatsContext(vids, rids, counter_ids, ctx); } - - return; - } - - std::vector supportedIds; - for (auto &counter : counter_ids) - { - if (isCounterSupported(counter)) + else { - supportedIds.push_back(counter); + // Bulk is not supported for this counter prefix + // Append it to bulkUnsupportedCounters + bulkUnsupportedCounters.insert(bulkUnsupportedCounters.end(), counter_ids.begin(), counter_ids.end()); + SWSS_LOG_NOTICE("Counters starting with %s do not support bulk. Fallback to single call for these counters", prefix.c_str()); } + }; + + // Perform a remove and re-add to simplify the logic here + for (auto vid: vids) + { + removeObject(vid, false); } - if (!supportBulk) + if (m_counterChunkSizeMapFromPrefix.empty()) { - for (size_t i = 0; i < vids.size(); i++) - { - auto &vid = vids[i]; - auto &rid = rids[i]; - // Perform a remove and re-add to simplify the logic here - removeObject(vid, false); - - // Unlikely - 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); - } + std::sort(supportedIds.begin(), supportedIds.end()); + checkAndUpdateBulkCapability(supportedIds, "default", default_bulk_chunk_size); } else { - // Perform a remove and re-add to simplify the logic here - for (auto vid: vids) + std::map> counter_prefix_map; + std::vector default_partition; + createCounterBulkChunkSizePerPrefixPartition(supportedIds, counter_prefix_map, default_partition); + + for (auto &counterPrefix : counter_prefix_map) { - removeObject(vid, false); + std::sort(counterPrefix.second.begin(), counterPrefix.second.end()); } - if (m_counterChunkSizeMapFromPrefix.empty()) - { - std::sort(supportedIds.begin(), supportedIds.end()); - auto bulkContext = getBulkStatsContext(supportedIds, "default", default_bulk_chunk_size); + std::sort(default_partition.begin(), default_partition.end()); - addBulkStatsContext(vids, rids, supportedIds, *bulkContext.get()); - } - else + for (auto &counterPrefix : counter_prefix_map) { - 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()); - } + checkAndUpdateBulkCapability(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); + } - std::sort(default_partition.begin(), default_partition.end()); + checkAndUpdateBulkCapability(default_partition, "default", default_bulk_chunk_size); + } - for (auto &counterPrefix : counter_prefix_map) - { - auto bulkContext = getBulkStatsContext(counterPrefix.second, counterPrefix.first, m_counterChunkSizeMapFromPrefix[counterPrefix.first]); - addBulkStatsContext(vids, rids, counterPrefix.second, *bulkContext.get()); - } + if (!bulkUnsupportedCounters.empty()) + { + SWSS_LOG_NOTICE("Partial counters do not support bulk. Fallback to single call"); - auto bulkContext = getBulkStatsContext(default_partition, "default", default_bulk_chunk_size); - addBulkStatsContext(vids, rids, default_partition, *bulkContext.get()); + // Fall back to old way + std::vector bulkUnsupportedidCounterStrings; + bulkUnsupportedidCounterStrings.reserve(bulkUnsupportedCounters.size()); + transform(bulkUnsupportedCounters.begin(), + bulkUnsupportedCounters.end(), + std::back_inserter(bulkUnsupportedidCounterStrings), + [] (auto &counterId) { return serializeStat(counterId); }); + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + addObject(vid, rid, idStrings, per_object_stats_mode); } } } From 683210482aebe5f69f430109cbdee6f4fdc15d5e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 6 Jan 2025 11:41:46 +0000 Subject: [PATCH 18/32] Fix issue: fail to parse virtual ID vector which causes error message Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 3 ++- syncd/FlexCounterManager.cpp | 4 ++-- syncd/FlexCounterManager.h | 4 ++-- syncd/Syncd.cpp | 10 +++++++--- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 1edf22ec1..9ad6bf993 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1199,7 +1199,8 @@ class CounterContext : public BaseCounterContext ctx.counters.data() + current * ctx.counter_ids.size()); if (SAI_STATUS_SUCCESS != status) { - SWSS_LOG_WARN("Failed to bulk get stats for %s: %u", m_name.c_str(), status); + SWSS_LOG_WARN("Failed to bulk get stats for %s %s %s %s starting object %u bulk chunk size %u: %d", + m_instanceId.c_str(), m_name.c_str(), ctx.name.c_str(), sai_serialize_object_type(m_objectType).c_str(), current, bulk_chunk_size, status); } current += bulk_chunk_size; diff --git a/syncd/FlexCounterManager.cpp b/syncd/FlexCounterManager.cpp index b02bce2cc..121d52400 100644 --- a/syncd/FlexCounterManager.cpp +++ b/syncd/FlexCounterManager.cpp @@ -102,8 +102,8 @@ void FlexCounterManager::addCounter( } void FlexCounterManager::bulkAddCounter( - _In_ const std::vector vids, - _In_ const std::vector rids, + _In_ const std::vector &vids, + _In_ const std::vector &rids, _In_ const std::string& instanceId, _In_ const std::vector& values) { diff --git a/syncd/FlexCounterManager.h b/syncd/FlexCounterManager.h index 687d4260c..4b50a6ccd 100644 --- a/syncd/FlexCounterManager.h +++ b/syncd/FlexCounterManager.h @@ -39,8 +39,8 @@ namespace syncd _In_ const std::vector& values); void bulkAddCounter( - _In_ const std::vector vids, - _In_ const std::vector rids, + _In_ const std::vector &vids, + _In_ const std::vector &rids, _In_ const std::string& instanceId, _In_ const std::vector& values); diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 724a8d232..50de1f34e 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2796,9 +2796,13 @@ sai_status_t Syncd::processFlexCounterEvent( if (fromAsicChannel && op == SET_COMMAND && vidStringVector.size() > 1) { - std::vector vids(vidStringVector.size()); - std::vector rids(vidStringVector.size()); - std::vector keys(vidStringVector.size()); + std::vector vids; + std::vector rids; + std::vector keys; + + vids.reserve(vidStringVector.size()); + rids.reserve(vidStringVector.size()); + keys.reserve(vidStringVector.size()); for (auto &strVid: vidStringVector) { From e247996ebb1f3f0924cce4b62cd1e21289e05d51 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 6 Jan 2025 11:42:42 +0000 Subject: [PATCH 19/32] Fix alignment Signed-off-by: Stephen Sun --- 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 665ca9c5c..6cd4c4139 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -904,7 +904,7 @@ TEST(FlexCounter, bulkChunksize) { if (object_count != unifiedBulkChunkSize) { - EXPECT_EQ(object_count, unifiedBulkChunkSize); + EXPECT_EQ(object_count, unifiedBulkChunkSize); } continue; } From 9ca5b17cd766cbea8b19b0ae1f1a003caae105ea Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 8 Jan 2025 10:07:09 +0000 Subject: [PATCH 20/32] Fix issue: check bulk with empty OID list and add unit test case Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 10 +++-- unittest/syncd/TestFlexCounter.cpp | 68 +++++++++++++++++++++++++++--- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 9ad6bf993..d254c01a7 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -895,8 +895,9 @@ class CounterContext : public BaseCounterContext 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) { - auto bulkContext = getBulkStatsContext(counter_ids, prefix, bulk_chunk_size); - auto &ctx = *bulkContext.get(); + BulkContextType ctx; + ctx.counter_ids = counter_ids; + addBulkStatsContext(vids, rids, counter_ids, ctx); auto status = m_vendorSai->bulkGetStats( SAI_NULL_OBJECT_ID, m_objectType, @@ -909,7 +910,8 @@ class CounterContext : public BaseCounterContext ctx.counters.data()); if (status == SAI_STATUS_SUCCESS) { - addBulkStatsContext(vids, rids, counter_ids, ctx); + auto bulkContext = getBulkStatsContext(counter_ids, prefix, bulk_chunk_size); + addBulkStatsContext(vids, rids, counter_ids, *bulkContext.get()); } else { @@ -1221,7 +1223,7 @@ class CounterContext : public BaseCounterContext { if (SAI_STATUS_SUCCESS != ctx.object_statuses[i]) { - SWSS_LOG_ERROR("Failed to get stats of %s 0x%" PRIx64 ": %d", m_name.c_str(), ctx.object_keys[i].key.object_id, ctx.object_statuses[i]); + SWSS_LOG_ERROR("Failed to get stats of %s 0x%" PRIx64 " 0x%" PRIx64 ": %d", m_name.c_str(), ctx.object_vids[i], ctx.object_keys[i].key.object_id, ctx.object_statuses[i]); continue; } const auto &vid = ctx.object_vids[i]; diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 6cd4c4139..81bc98f7b 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -812,9 +812,6 @@ TEST(FlexCounter, bulkChunksize) * 3. Verify whether values of all counter IDs of all objects have been generated * 4. Verify whether the bulk chunk size is correct */ - 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) { - 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) { return SAI_STATUS_SUCCESS; }; @@ -862,11 +859,31 @@ TEST(FlexCounter, bulkChunksize) } }; + std::map bulkUnsupportedCounters; + // > + std::map> bulkUnsupportedCounterExpectedValues; + sai->mock_getStatsExt = [&](sai_object_type_t, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, sai_stats_mode_t, uint64_t *counters) { + for (auto i = 0u; i < number_of_counters; i++) + { + if (bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) + { + counters[i] = bulkUnsupportedCounters[counter_ids[i]] * (uint64_t)oid; + if (counterValuesMap.find(oid) == counterValuesMap.end()) + { + counterValuesMap[oid] = {}; + } + counterValuesMap[oid][counter_ids[i]] = counters[i]; + } + } + return SAI_STATUS_SUCCESS; + }; + std::vector> counterRecord; std::vector> valueRecord; sai_uint64_t counterSeed = 0; - uint32_t expectedInitObjectCount; + // non zero unifiedBulkChunkSize indicates all counter IDs share the same bulk chunk size uint32_t unifiedBulkChunkSize = 0; + int32_t initialCheckCount; sai->mock_bulkGetStats = [&](sai_object_id_t, sai_object_type_t, uint32_t object_count, @@ -880,10 +897,18 @@ TEST(FlexCounter, bulkChunksize) EXPECT_TRUE(mode == SAI_STATS_MODE_BULK_READ); std::vector record; std::vector value; - if (number_of_counters >= 5 && object_count == expectedInitObjectCount) + if (initialCheckCount-- > 0) { allObjectIds.insert(toOid(object_keys[0].key.object_id)); // This call is to check whether bulk counter polling is supported during initialization + if (!bulkUnsupportedCounters.empty()) + { + // Simulate counters that are not supported being polled in bulk mode + if (bulkUnsupportedCounters.find(counter_ids[0]) != bulkUnsupportedCounters.end()) + { + return SAI_STATUS_FAILURE; + } + } return SAI_STATUS_SUCCESS; } for (uint32_t i = 0; i < object_count; i++) @@ -955,7 +980,7 @@ TEST(FlexCounter, bulkChunksize) allObjectIds.erase(key); }; - expectedInitObjectCount = 1; + initialCheckCount = 6; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -970,6 +995,7 @@ TEST(FlexCounter, bulkChunksize) "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); + initialCheckCount = 6; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -987,6 +1013,7 @@ TEST(FlexCounter, bulkChunksize) EXPECT_TRUE(allObjectIds.empty()); unifiedBulkChunkSize = 3; + initialCheckCount = 6; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -1005,7 +1032,7 @@ TEST(FlexCounter, bulkChunksize) EXPECT_TRUE(allObjectIds.empty()); unifiedBulkChunkSize = 0; - expectedInitObjectCount = 6; + initialCheckCount = 3; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -1020,6 +1047,33 @@ TEST(FlexCounter, bulkChunksize) "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); + initialCheckCount = 3; + testAddRemoveCounter( + 6, + SAI_OBJECT_TYPE_PORT, + PORT_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS", "SAI_PORT_STAT_IF_OUT_QLEN", "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES"}, + {}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true, + "3", + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", + false, + PORT_PLUGIN_FIELD); + EXPECT_TRUE(allObjectIds.empty()); + + initialCheckCount = 3; // check bulk for 3 prefixes + initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects + bulkUnsupportedCounters = { + {SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES, 3}, + {SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES, 5} + }; + std::set bulkUnsupportedCounterNames = { + "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", + "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES" + }; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, From 9b761889bbaf328fcd78a7e80a66a3e7ef176a9c Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Thu, 9 Jan 2025 05:59:30 +0000 Subject: [PATCH 21/32] Update log severity before and after bulk counter polling to info Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index d254c01a7..f008b8003 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1185,7 +1185,7 @@ class CounterContext : public BaseCounterContext } uint32_t current = 0; - SWSS_LOG_DEBUG("Before getting bulk %s %s %s size %u bulk chunk size %u current %u", m_instanceId.c_str(), m_name.c_str(), ctx.name.c_str(), size, bulk_chunk_size, current); + SWSS_LOG_INFO("Before getting bulk %s %s %s size %u bulk chunk size %u current %u", m_instanceId.c_str(), m_name.c_str(), ctx.name.c_str(), size, bulk_chunk_size, current); while (current < size) { From 8ddf7e5b13cb46cbf4d9f892add590b34bdae385 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 12 Jan 2025 23:20:02 +0000 Subject: [PATCH 22/32] Enhance unit test Signed-off-by: Stephen Sun --- unittest/syncd/TestFlexCounter.cpp | 115 +++++++++++++++++++++++------ 1 file changed, 93 insertions(+), 22 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 81bc98f7b..78ff3eb11 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -178,7 +178,7 @@ void testAddRemoveCounter( countersTable.getKeys(keys); ASSERT_TRUE(keys.empty()); } - +#if 0 TEST(FlexCounter, addRemoveCounter) { 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) { @@ -801,7 +801,7 @@ TEST(FlexCounter, bulkCounter) // buffer pool stats does not support bulk EXPECT_EQ(false, clearCalled); } - +#endif TEST(FlexCounter, bulkChunksize) { /* @@ -811,10 +811,28 @@ TEST(FlexCounter, bulkChunksize) * and verify whether the database content aligns with the stored values * 3. Verify whether values of all counter IDs of all objects have been generated * 4. Verify whether the bulk chunk size is correct + * 5. Simulate bulk-unsupported counter IDs which should be fetched by single call + * with both per counter bulk size supported or not + * + * A counter can be polled in initialization phase and runtime + * For each test, it is expected to call bulk for "initialCheckCount" times during initialization. + * In this stage, the mock function just return succeed for failure to indicate whehter bulk poll is supported + * but it does not provide a counter value for further check + * + * The calls to bulk starting from "initialCheckCount+1"-th are treated as runtime calls. + * The counter values objects are generated as following: + * - a counterSeed maintains the current counter value to return + * - If the counterValuesMap[object_id][counter_id] exists, returns it as the counter's value + * - Otherwise, it's the first time the (object, counter ID) is polled + * - return the current value of counterSeed as the counter's value + * - store the couter's value into counterValuesMap + * - increase the counterSeed + * When the test finishes, the counterSeed should equal (number_of_objects * number_of_bulk_supported_counters) + * And all integer < counterSeed should be returned to one and only one (object, counter ID) tuple, + * which can be verified by the verify function. + * + * For the bulk-unsupported counters, getStatExt will be called to poll counters, and return counter_id * OID as the counter's value */ - sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { - return SAI_STATUS_SUCCESS; - }; sai->mock_queryStatsCapability = [&](sai_object_id_t switch_id, sai_object_type_t object_type, sai_stat_capability_list_t *stats_capability) { // For now, just return failure to make test simple, will write a singe test to cover querySupportedCounters return SAI_STATUS_FAILURE; @@ -859,15 +877,18 @@ TEST(FlexCounter, bulkChunksize) } }; - std::map bulkUnsupportedCounters; + // Bulk-unsupported counter IDs should be polled using single call (getStatExt) + std::set bulkUnsupportedCounters; + bool forceSingleCall = false; // > - std::map> bulkUnsupportedCounterExpectedValues; - sai->mock_getStatsExt = [&](sai_object_type_t, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, sai_stats_mode_t, uint64_t *counters) { +// std::map> bulkUnsupportedCounterExpectedValues; + auto _getStatsExt = [&](sai_object_type_t, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, sai_stats_mode_t, uint64_t *counters) { for (auto i = 0u; i < number_of_counters; i++) { - if (bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) + if (forceSingleCall || bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) { - counters[i] = bulkUnsupportedCounters[counter_ids[i]] * (uint64_t)oid; + // avoid counter_id == 0 which causes the same counter value (0) for all objects + counters[i] = (1 + counter_ids[i]) * (uint64_t)oid; if (counterValuesMap.find(oid) == counterValuesMap.end()) { counterValuesMap[oid] = {}; @@ -878,6 +899,14 @@ TEST(FlexCounter, bulkChunksize) return SAI_STATUS_SUCCESS; }; + sai->mock_getStats = [&](sai_object_type_t type, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, uint64_t *counters) { + return _getStatsExt(type, oid, number_of_counters, counter_ids, SAI_STATS_MODE_READ, counters); + }; + + sai->mock_getStatsExt = [&](sai_object_type_t type, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, sai_stats_mode_t mode, uint64_t *counters) { + return _getStatsExt(type, oid, number_of_counters, counter_ids, mode, counters); + }; + std::vector> counterRecord; std::vector> valueRecord; sai_uint64_t counterSeed = 0; @@ -904,13 +933,19 @@ TEST(FlexCounter, bulkChunksize) if (!bulkUnsupportedCounters.empty()) { // Simulate counters that are not supported being polled in bulk mode - if (bulkUnsupportedCounters.find(counter_ids[0]) != bulkUnsupportedCounters.end()) + for (auto i = 0u; i < number_of_counters; i++) { - return SAI_STATUS_FAILURE; + if (bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) + { + return SAI_STATUS_FAILURE; + } } } return SAI_STATUS_SUCCESS; } + + EXPECT_TRUE(!forceSingleCall); + for (uint32_t i = 0; i < object_count; i++) { object_status[i] = SAI_STATUS_SUCCESS; @@ -979,7 +1014,8 @@ TEST(FlexCounter, bulkChunksize) allObjectIds.erase(key); }; - +#if 1 + // create ports first and then set bulk chunk size + per counter bulk chunk size initialCheckCount = 6; testAddRemoveCounter( 6, @@ -995,6 +1031,7 @@ TEST(FlexCounter, bulkChunksize) "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); + // set bulk chunk size + per counter bulk chunk size first and then create ports initialCheckCount = 6; testAddRemoveCounter( 6, @@ -1010,8 +1047,11 @@ TEST(FlexCounter, bulkChunksize) "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", false, PORT_PLUGIN_FIELD); - EXPECT_TRUE(allObjectIds.empty()); + EXPECT_TRUE(allObjectIds.empty()); + // Remove per counter bulk chunk size after initializing it + // This is to cover the scenario of removing per counter bulk chunk size filed + // All counters share a unified bulk chunk size unifiedBulkChunkSize = 3; initialCheckCount = 6; testAddRemoveCounter( @@ -1032,6 +1072,7 @@ TEST(FlexCounter, bulkChunksize) EXPECT_TRUE(allObjectIds.empty()); unifiedBulkChunkSize = 0; + // add ports counters in bulk mode first and then set bulk chunk size + per counter bulk chunk size initialCheckCount = 3; testAddRemoveCounter( 6, @@ -1047,6 +1088,7 @@ TEST(FlexCounter, bulkChunksize) "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); + // set bulk chunk size + per counter bulk chunk size first and then add ports counters in bulk mode initialCheckCount = 3; testAddRemoveCounter( 6, @@ -1063,17 +1105,44 @@ TEST(FlexCounter, bulkChunksize) false, PORT_PLUGIN_FIELD); EXPECT_TRUE(allObjectIds.empty()); - - initialCheckCount = 3; // check bulk for 3 prefixes +#endif + +//PROBLEMATIC CASE + // add ports counters in bulk mode with some bulk-unsupported counters first and then set bulk chunk size + per counter bulk chunk size + // all counters will be polled using single call in runtime + #if 1 + forceSingleCall = true; + initialCheckCount = 1; // check bulk for all counter IDs altogether initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects bulkUnsupportedCounters = { + SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES, + SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES + }; + testAddRemoveCounter( + 6, + SAI_OBJECT_TYPE_PORT, + PORT_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS", "SAI_PORT_STAT_IF_OUT_QLEN", "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES"}, + {}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true, + "3", + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"/*, + false, + PORT_PLUGIN_FIELD*/); + EXPECT_TRUE(allObjectIds.empty()); + forceSingleCall = false; + #endif + #if 1 + // set bulk chunk size + per counter bulk chunk size first and then add ports counters in bulk mode with some bulk-unsupported counters + initialCheckCount = 3; // check bulk for 3 prefixes + initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects +/* bulkUnsupportedCounters = { {SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES, 3}, {SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES, 5} - }; - std::set bulkUnsupportedCounterNames = { - "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", - "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES" - }; + };*/ testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -1089,8 +1158,9 @@ TEST(FlexCounter, bulkChunksize) false, PORT_PLUGIN_FIELD); EXPECT_TRUE(allObjectIds.empty()); + #endif } - +#if 0 TEST(FlexCounter, counterIdChange) { sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { @@ -1482,3 +1552,4 @@ TEST(FlexCounter, noEniDashMeterCounter) counterVerifyFunc, false); } +#endif From ffb93e3ab41aed4880becdf36a17d7ea90664b83 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 13 Jan 2025 08:26:31 +0000 Subject: [PATCH 23/32] Handle corner case that partial counters are not supported to be polled by bulk call Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 69 ++++++++++++++++++++++++++----------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index f008b8003..8b9fcc43a 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -891,7 +891,7 @@ class CounterContext : public BaseCounterContext } } - std::vector bulkUnsupportedCounters; + 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) { @@ -917,17 +917,12 @@ class CounterContext : public BaseCounterContext { // Bulk is not supported for this counter prefix // Append it to bulkUnsupportedCounters - bulkUnsupportedCounters.insert(bulkUnsupportedCounters.end(), counter_ids.begin(), counter_ids.end()); + std::tuple value(prefix, bulk_chunk_size); + bulkUnsupportedCounters.emplace(counter_ids, value); SWSS_LOG_NOTICE("Counters starting with %s do not support bulk. Fallback to single call for these counters", prefix.c_str()); } }; - // Perform a remove and re-add to simplify the logic here - for (auto vid: vids) - { - removeObject(vid, false); - } - if (m_counterChunkSizeMapFromPrefix.empty()) { std::sort(supportedIds.begin(), supportedIds.end()); @@ -956,20 +951,40 @@ class CounterContext : public BaseCounterContext if (!bulkUnsupportedCounters.empty()) { - SWSS_LOG_NOTICE("Partial counters do not support bulk. Fallback to single call"); + SWSS_LOG_NOTICE("Partial counters do not support bulk. Re-check bulk capability for each object"); - // Fall back to old way - std::vector bulkUnsupportedidCounterStrings; - bulkUnsupportedidCounterStrings.reserve(bulkUnsupportedCounters.size()); - transform(bulkUnsupportedCounters.begin(), - bulkUnsupportedCounters.end(), - std::back_inserter(bulkUnsupportedidCounterStrings), - [] (auto &counterId) { return serializeStat(counterId); }); - for (size_t i = 0; i < vids.size(); i++) + for (auto &it : bulkUnsupportedCounters) { - auto rid = rids[i]; - auto vid = vids[i]; - addObject(vid, rid, idStrings, per_object_stats_mode); + std::vector bulkSupportedRIDs; + std::vector bulkSupportedVIDs; + for (size_t i = 0; i < vids.size(); i++) + { + auto rid = rids[i]; + auto vid = vids[i]; + + if (checkBulkCapability(vid, rid, it.first)) + { + bulkSupportedVIDs.push_back(vid); + bulkSupportedRIDs.push_back(rid); + } + else + { + SWSS_LOG_INFO("Fallback to single call for object 0x%" PRIx64, vid); + 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); + } + } + + 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()); + } } } } @@ -993,14 +1008,14 @@ class CounterContext : public BaseCounterContext { m_objectIdsMap.erase(iter); } - else + + // An object can be in both m_objectIdsMap and the bulk context + // when bulk polling is supported by some counter prefixes but unsupported by some others + if (!removeBulkStatsContext(vid) && log) { - if (!removeBulkStatsContext(vid) && log) - { - SWSS_LOG_NOTICE("Trying to remove nonexisting %s %s", - sai_serialize_object_type(m_objectType).c_str(), - sai_serialize_object_id(vid).c_str()); - } + SWSS_LOG_NOTICE("Trying to remove nonexisting %s %s", + sai_serialize_object_type(m_objectType).c_str(), + sai_serialize_object_id(vid).c_str()); } } From 7424742b1121a420b58953d30c556beb7f5070ca Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 13 Jan 2025 11:23:18 +0200 Subject: [PATCH 24/32] Enhance the unit test Signed-off-by: Stephen Sun --- unittest/syncd/TestFlexCounter.cpp | 65 +++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 78ff3eb11..07270a402 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -178,7 +178,7 @@ void testAddRemoveCounter( countersTable.getKeys(keys); ASSERT_TRUE(keys.empty()); } -#if 0 + TEST(FlexCounter, addRemoveCounter) { 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) { @@ -801,7 +801,7 @@ TEST(FlexCounter, bulkCounter) // buffer pool stats does not support bulk EXPECT_EQ(false, clearCalled); } -#endif + TEST(FlexCounter, bulkChunksize) { /* @@ -879,13 +879,15 @@ TEST(FlexCounter, bulkChunksize) // Bulk-unsupported counter IDs should be polled using single call (getStatExt) std::set bulkUnsupportedCounters; + std::set bulkUnsupportedObjectIds; bool forceSingleCall = false; // > -// std::map> bulkUnsupportedCounterExpectedValues; auto _getStatsExt = [&](sai_object_type_t, sai_object_id_t oid, uint32_t number_of_counters, const sai_stat_id_t *counter_ids, sai_stats_mode_t, uint64_t *counters) { for (auto i = 0u; i < number_of_counters; i++) { - if (forceSingleCall || bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) + if (forceSingleCall + || (bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end() + && (bulkUnsupportedObjectIds.empty() || bulkUnsupportedObjectIds.find(oid) != bulkUnsupportedObjectIds.end()))) { // avoid counter_id == 0 which causes the same counter value (0) for all objects counters[i] = (1 + counter_ids[i]) * (uint64_t)oid; @@ -913,6 +915,7 @@ TEST(FlexCounter, bulkChunksize) // non zero unifiedBulkChunkSize indicates all counter IDs share the same bulk chunk size uint32_t unifiedBulkChunkSize = 0; int32_t initialCheckCount; + int32_t partialSupportingBulkObjectFactor; sai->mock_bulkGetStats = [&](sai_object_id_t, sai_object_type_t, uint32_t object_count, @@ -937,6 +940,16 @@ TEST(FlexCounter, bulkChunksize) { if (bulkUnsupportedCounters.find(counter_ids[i]) != bulkUnsupportedCounters.end()) { + if (partialSupportingBulkObjectFactor != 0) + { + for(auto j = 0u; j < object_count; j++) + { + if (j % partialSupportingBulkObjectFactor == 0) + { + bulkUnsupportedObjectIds.insert(object_keys[j].key.object_id); + } + } + } return SAI_STATUS_FAILURE; } } @@ -1014,7 +1027,7 @@ TEST(FlexCounter, bulkChunksize) allObjectIds.erase(key); }; -#if 1 + // create ports first and then set bulk chunk size + per counter bulk chunk size initialCheckCount = 6; testAddRemoveCounter( @@ -1105,12 +1118,9 @@ TEST(FlexCounter, bulkChunksize) false, PORT_PLUGIN_FIELD); EXPECT_TRUE(allObjectIds.empty()); -#endif -//PROBLEMATIC CASE // add ports counters in bulk mode with some bulk-unsupported counters first and then set bulk chunk size + per counter bulk chunk size // all counters will be polled using single call in runtime - #if 1 forceSingleCall = true; initialCheckCount = 1; // check bulk for all counter IDs altogether initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects @@ -1129,20 +1139,37 @@ TEST(FlexCounter, bulkChunksize) STATS_MODE_READ, true, "3", - "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"/*, - false, - PORT_PLUGIN_FIELD*/); + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2"); EXPECT_TRUE(allObjectIds.empty()); forceSingleCall = false; - #endif - #if 1 + + // set bulk chunk size + per counter bulk chunk size first and then add ports counters in bulk mode with some bulk-unsupported counters + // All bulk-unsupported counters are polled using single call and all the rest counters are polled using bulk call + // For each OID, it will be in both m_bulkContexts and m_objectIdsMap + initialCheckCount = 3; // check bulk for 3 prefixes + initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects + testAddRemoveCounter( + 6, + SAI_OBJECT_TYPE_PORT, + PORT_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IF_IN_OCTETS", "SAI_PORT_STAT_IF_IN_UCAST_PKTS", "SAI_PORT_STAT_IF_OUT_QLEN", "SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES", "SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES"}, + {}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true, + "3", + "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", + false, + PORT_PLUGIN_FIELD); + EXPECT_TRUE(allObjectIds.empty()); + // set bulk chunk size + per counter bulk chunk size first and then add ports counters in bulk mode with some bulk-unsupported counters + // All bulk-unsupported counters are polled using single call and all the rest counters are polled using bulk call + // For each OID, it will be in both m_bulkContexts and m_objectIdsMap initialCheckCount = 3; // check bulk for 3 prefixes initialCheckCount += 6; // for bulk unsupported counter prefix, check bulk again for each objects -/* bulkUnsupportedCounters = { - {SAI_PORT_STAT_IF_IN_FEC_CORRECTABLE_FRAMES, 3}, - {SAI_PORT_STAT_IF_IN_FEC_NOT_CORRECTABLE_FRAMES, 5} - };*/ + partialSupportingBulkObjectFactor = 2; testAddRemoveCounter( 6, SAI_OBJECT_TYPE_PORT, @@ -1158,9 +1185,8 @@ TEST(FlexCounter, bulkChunksize) false, PORT_PLUGIN_FIELD); EXPECT_TRUE(allObjectIds.empty()); - #endif } -#if 0 + TEST(FlexCounter, counterIdChange) { sai->mock_getStats = [&](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { @@ -1552,4 +1578,3 @@ TEST(FlexCounter, noEniDashMeterCounter) counterVerifyFunc, false); } -#endif From 66e2e6135eefb1c2fe5d9fe30bbca380a35c9a3f Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 30 Dec 2024 23:28:40 +0000 Subject: [PATCH 25/32] Add comments Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 8b9fcc43a..68d69c613 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -697,6 +697,7 @@ class CounterContext : public BaseCounterContext { // Only one bulk context exists which means // it is the first time per counter chunk size is configured and a unified counter ID set is polled for all objects + SWSS_LOG_NOTICE("Split counter IDs set by prefix for the first time %s", bulkChunkSizePerPrefix.c_str()); auto it = m_bulkContexts.begin(); std::shared_ptr singleBulkContext = it->second; const std::vector &allCounterIds = singleBulkContext.get()->counter_ids; From 135200d5f55d2b8f38a71815fe1f233d0facc05e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 31 Dec 2024 00:36:29 +0000 Subject: [PATCH 26/32] Move the syslog to avoid confusion Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 68d69c613..8b9fcc43a 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -697,7 +697,6 @@ class CounterContext : public BaseCounterContext { // Only one bulk context exists which means // it is the first time per counter chunk size is configured and a unified counter ID set is polled for all objects - SWSS_LOG_NOTICE("Split counter IDs set by prefix for the first time %s", bulkChunkSizePerPrefix.c_str()); auto it = m_bulkContexts.begin(); std::shared_ptr singleBulkContext = it->second; const std::vector &allCounterIds = singleBulkContext.get()->counter_ids; From 6ba9c47d9f3210231c04cef8ed4492343e172ad8 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sat, 25 Jan 2025 10:13:52 +0000 Subject: [PATCH 27/32] Do not check bulk capability if it is configured for counter groups without plugin Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 8b9fcc43a..07563bad5 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2338,7 +2338,15 @@ std::shared_ptr FlexCounter::getCounterContext( return iter->second; } - auto ret = m_counterContext.emplace(name, createCounterContext(name, m_instanceId)); + auto counterContext = createCounterContext(name, m_instanceId); + + if (m_noDoubleCheckBulkCapability) + { + counterContext->setNoDoubleCheckBulkCapability(true); + SWSS_LOG_NOTICE("Do not double check bulk capability counter context %s %s", m_instanceId.c_str(), name.c_str()); + } + + auto ret = m_counterContext.emplace(name, counterContext); return ret.first->second; } From 139202db02263f28771eac282dc3d1dad9c513fd Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Sun, 9 Feb 2025 02:33:04 +0000 Subject: [PATCH 28/32] Fix spelling check error and tail white spaces Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 2 +- unittest/syncd/TestFlexCounter.cpp | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 07563bad5..0ee3b2fe6 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2819,7 +2819,7 @@ void FlexCounter::addCounter( notifyPoll(); } -void FlexCounter::bulkAddCounter( +void FlexCounter::bulkAddCounter( _In_ sai_object_type_t objectType, _In_ const std::vector& vids, _In_ const std::vector& rids, diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 07270a402..192d83ed8 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -816,16 +816,16 @@ TEST(FlexCounter, bulkChunksize) * * A counter can be polled in initialization phase and runtime * For each test, it is expected to call bulk for "initialCheckCount" times during initialization. - * In this stage, the mock function just return succeed for failure to indicate whehter bulk poll is supported + * In this stage, the mock function just return succeed for failure to indicate whether bulk poll is supported * but it does not provide a counter value for further check * - * The calls to bulk starting from "initialCheckCount+1"-th are treated as runtime calls. + * The calls to bulk starting from "initialCheckCount+1" are treated as runtime calls. * The counter values objects are generated as following: * - a counterSeed maintains the current counter value to return * - If the counterValuesMap[object_id][counter_id] exists, returns it as the counter's value * - Otherwise, it's the first time the (object, counter ID) is polled * - return the current value of counterSeed as the counter's value - * - store the couter's value into counterValuesMap + * - store the counter's value into counterValuesMap * - increase the counterSeed * When the test finishes, the counterSeed should equal (number_of_objects * number_of_bulk_supported_counters) * And all integer < counterSeed should be returned to one and only one (object, counter ID) tuple, From 9a4bb031669619b6a29266d6c485b2536705c6ce Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 10 Feb 2025 04:40:54 +0000 Subject: [PATCH 29/32] Fix issue and simplify code add a map to simplify bulkAddCounter and addCounter fix issue that bulkAddCounter isn't supported by attr counter Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 287 +++++------------------------ syncd/FlexCounter.h | 2 + unittest/syncd/TestFlexCounter.cpp | 159 +++++++++++++++- 3 files changed, 201 insertions(+), 247 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 0ee3b2fe6..b0c71a46a 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -47,6 +47,26 @@ const std::map FlexCounter::m_plugIn2CounterType = { {WRED_QUEUE_PLUGIN_FIELD, COUNTER_TYPE_WRED_ECN_QUEUE}, {WRED_PORT_PLUGIN_FIELD, COUNTER_TYPE_WRED_ECN_PORT}}; +const std::map, std::string> FlexCounter::m_objectTypeField2CounterType = { + {{SAI_OBJECT_TYPE_PORT, PORT_COUNTER_ID_LIST}, COUNTER_TYPE_PORT}, + {{SAI_OBJECT_TYPE_PORT, PORT_DEBUG_COUNTER_ID_LIST}, COUNTER_TYPE_PORT_DEBUG}, + {{SAI_OBJECT_TYPE_QUEUE, QUEUE_COUNTER_ID_LIST}, COUNTER_TYPE_QUEUE}, + {{SAI_OBJECT_TYPE_QUEUE, QUEUE_ATTR_ID_LIST}, ATTR_TYPE_QUEUE}, + {{SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP, PG_COUNTER_ID_LIST}, COUNTER_TYPE_PG}, + {{SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP, PG_ATTR_ID_LIST}, ATTR_TYPE_PG}, + {{SAI_OBJECT_TYPE_ROUTER_INTERFACE, RIF_COUNTER_ID_LIST}, COUNTER_TYPE_RIF}, + {{SAI_OBJECT_TYPE_SWITCH, SWITCH_DEBUG_COUNTER_ID_LIST}, COUNTER_TYPE_SWITCH_DEBUG}, + {{SAI_OBJECT_TYPE_MACSEC_FLOW, MACSEC_FLOW_COUNTER_ID_LIST}, COUNTER_TYPE_MACSEC_FLOW}, + {{SAI_OBJECT_TYPE_MACSEC_SA, MACSEC_SA_COUNTER_ID_LIST}, COUNTER_TYPE_MACSEC_SA}, + {{SAI_OBJECT_TYPE_MACSEC_SA, MACSEC_SA_ATTR_ID_LIST}, ATTR_TYPE_MACSEC_SA}, + {{SAI_OBJECT_TYPE_ACL_COUNTER, ACL_COUNTER_ATTR_ID_LIST}, ATTR_TYPE_ACL_COUNTER}, + {{SAI_OBJECT_TYPE_COUNTER, FLOW_COUNTER_ID_LIST}, COUNTER_TYPE_FLOW}, + {{SAI_OBJECT_TYPE_POLICER, POLICER_COUNTER_ID_LIST}, COUNTER_TYPE_POLICER}, + {{SAI_OBJECT_TYPE_TUNNEL, TUNNEL_COUNTER_ID_LIST}, COUNTER_TYPE_TUNNEL}, + {{(sai_object_type_t)SAI_OBJECT_TYPE_ENI, ENI_COUNTER_ID_LIST}, COUNTER_TYPE_ENI}, + {{(sai_object_type_t)SAI_OBJECT_TYPE_ENI, DASH_METER_COUNTER_ID_LIST}, COUNTER_TYPE_METER_BUCKET} +}; + BaseCounterContext::BaseCounterContext(const std::string &name, const std::string &instance): m_name(name), m_instanceId(instance) @@ -1541,6 +1561,20 @@ class AttrContext : public CounterContext Base::m_objectIdsMap.emplace(vid, attr_ids); } + void bulkAddObject( + _In_ const std::vector& vids, + _In_ const std::vector& rids, + _In_ const std::vector& idStrings, + _In_ const std::string &per_object_stats_mode) override + { + SWSS_LOG_ENTER(); + + for (auto i = 0uL; i < vids.size(); i++) + { + addObject(vids[i], rids[i], idStrings, per_object_stats_mode); + } + } + void collectData( _In_ swss::Table &countersTable) override { @@ -1720,6 +1754,7 @@ class DashMeterCounterContext : public BaseCounterContext _In_ const std::string &per_object_stats_mode) override { SWSS_LOG_ENTER(); + SWSS_LOG_ERROR("It does not support to add counter in bulk mode for DashMeterCounterContext"); } bool hasObject() const override @@ -2651,114 +2686,10 @@ void FlexCounter::addCounter( auto idStrings = swss::tokenize(value, ','); - if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PORT)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_DEBUG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PORT_DEBUG)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_QUEUE)->addObject( - vid, - rid, - idStrings, - ""); - - } - else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_ATTR_ID_LIST) + const auto &counterGroupRef = m_objectTypeField2CounterType.find({objectType, field}); + if (counterGroupRef != m_objectTypeField2CounterType.end()) { - getCounterContext(ATTR_TYPE_QUEUE)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PG)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_PG)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_ROUTER_INTERFACE && field == RIF_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_RIF)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_SWITCH && field == SWITCH_DEBUG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_SWITCH_DEBUG)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_FLOW && field == MACSEC_FLOW_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_MACSEC_FLOW)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_MACSEC_SA)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_MACSEC_SA)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_ACL_COUNTER && field == ACL_COUNTER_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_ACL_COUNTER)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_COUNTER && field == FLOW_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_FLOW)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_POLICER && field == POLICER_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_POLICER)->addObject( + getCounterContext(counterGroupRef->second)->addObject( vid, rid, idStrings, @@ -2772,30 +2703,6 @@ void FlexCounter::addCounter( { statsMode = value; } - else if (objectType == SAI_OBJECT_TYPE_TUNNEL && field == TUNNEL_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_TUNNEL)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == (sai_object_type_t)SAI_OBJECT_TYPE_ENI && field == ENI_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_ENI)->addObject( - vid, - rid, - idStrings, - ""); - } - else if (objectType == (sai_object_type_t)SAI_OBJECT_TYPE_ENI && field == DASH_METER_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_METER_BUCKET)->addObject( - vid, - rid, - idStrings, - ""); - } else { SWSS_LOG_ERROR("Object type and field combination is not supported, object type %s, field %s", @@ -2840,122 +2747,10 @@ void FlexCounter::bulkAddCounter( auto idStrings = swss::tokenize(value, ','); - if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PORT)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_PORT && field == PORT_DEBUG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PORT_DEBUG)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_QUEUE)->bulkAddObject( - vids, - rids, - idStrings, - ""); - - } - else if (objectType == SAI_OBJECT_TYPE_QUEUE && field == QUEUE_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_QUEUE)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_PG)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP && field == PG_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_PG)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_ROUTER_INTERFACE && field == RIF_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_RIF)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_SWITCH && field == SWITCH_DEBUG_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_SWITCH_DEBUG)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_FLOW && field == MACSEC_FLOW_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_MACSEC_FLOW)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_MACSEC_SA)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_MACSEC_SA && field == MACSEC_SA_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_MACSEC_SA)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_ACL_COUNTER && field == ACL_COUNTER_ATTR_ID_LIST) - { - getCounterContext(ATTR_TYPE_ACL_COUNTER)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_COUNTER && field == FLOW_COUNTER_ID_LIST) - { - getCounterContext(COUNTER_TYPE_FLOW)->bulkAddObject( - vids, - rids, - idStrings, - ""); - } - else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == BUFFER_POOL_COUNTER_ID_LIST) - { - counterIds = idStrings; - } - else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == STATS_MODE_FIELD) - { - statsMode = value; - } - else if (objectType == SAI_OBJECT_TYPE_TUNNEL && field == TUNNEL_COUNTER_ID_LIST) + const auto &counterGroupRef = m_objectTypeField2CounterType.find({objectType, field}); + if (counterGroupRef != m_objectTypeField2CounterType.end()) { - getCounterContext(COUNTER_TYPE_TUNNEL)->bulkAddObject( + getCounterContext(counterGroupRef->second)->bulkAddObject( vids, rids, idStrings, diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index 132fd3237..5a2191c0b 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -200,5 +200,7 @@ namespace syncd bool m_noDoubleCheckBulkCapability; static const std::map m_plugIn2CounterType; + + static const std::map, std::string> m_objectTypeField2CounterType; }; } diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 192d83ed8..e5a353494 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -127,7 +127,7 @@ void testAddRemoveCounter( if (bulkAdd) { - fc.bulkAddCounter(SAI_OBJECT_TYPE_PORT, object_ids, object_ids, values); + fc.bulkAddCounter(object_type, object_ids, object_ids, values); } else { @@ -224,6 +224,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, true); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_COUNTER, + FLOW_COUNTER_ID_LIST, + {"SAI_COUNTER_STAT_PACKETS", "SAI_COUNTER_STAT_BYTES"}, + {"100", "200"}, + counterVerifyFunc, + true, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_MACSEC_FLOW, @@ -233,6 +245,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_MACSEC_FLOW, + MACSEC_FLOW_COUNTER_ID_LIST, + {"SAI_MACSEC_FLOW_STAT_CONTROL_PKTS", "SAI_MACSEC_FLOW_STAT_PKTS_UNTAGGED"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_MACSEC_SA, @@ -242,6 +266,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_MACSEC_SA, + MACSEC_SA_COUNTER_ID_LIST, + {"SAI_MACSEC_SA_STAT_OCTETS_ENCRYPTED", "SAI_MACSEC_SA_STAT_OCTETS_PROTECTED"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_PORT, @@ -260,6 +296,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_PORT, + PORT_DEBUG_COUNTER_ID_LIST, + {"SAI_PORT_STAT_IN_CONFIGURED_DROP_REASONS_0_DROPPED_PKTS", "SAI_PORT_STAT_IN_CONFIGURED_DROP_REASONS_1_DROPPED_PKTS"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + bool clearCalled = false; sai->mock_clearStats = [&] (sai_object_type_t object_type, sai_object_id_t object_id, uint32_t number_of_counters, const sai_stat_id_t *counter_ids) { clearCalled = true; @@ -277,6 +325,19 @@ TEST(FlexCounter, addRemoveCounter) STATS_MODE_READ_AND_CLEAR); EXPECT_EQ(true, clearCalled); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_QUEUE, + QUEUE_COUNTER_ID_LIST, + {"SAI_QUEUE_STAT_PACKETS", "SAI_QUEUE_STAT_BYTES"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ_AND_CLEAR, + true); + EXPECT_EQ(true, clearCalled); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP, @@ -286,6 +347,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP, + PG_COUNTER_ID_LIST, + {"SAI_INGRESS_PRIORITY_GROUP_STAT_PACKETS", "SAI_INGRESS_PRIORITY_GROUP_STAT_BYTES"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_ROUTER_INTERFACE, @@ -295,6 +368,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_ROUTER_INTERFACE, + RIF_COUNTER_ID_LIST, + {"SAI_ROUTER_INTERFACE_STAT_IN_OCTETS", "SAI_ROUTER_INTERFACE_STAT_IN_PACKETS"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_SWITCH, @@ -304,6 +389,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_SWITCH, + SWITCH_DEBUG_COUNTER_ID_LIST, + {"SAI_SWITCH_STAT_IN_CONFIGURED_DROP_REASONS_0_DROPPED_PKTS", "SAI_SWITCH_STAT_IN_CONFIGURED_DROP_REASONS_1_DROPPED_PKTS"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, SAI_OBJECT_TYPE_TUNNEL, @@ -313,6 +410,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_TUNNEL, + TUNNEL_COUNTER_ID_LIST, + {"SAI_TUNNEL_STAT_IN_OCTETS", "SAI_TUNNEL_STAT_IN_PACKETS"}, + {"100", "200"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + testAddRemoveCounter( 1, (sai_object_type_t)SAI_OBJECT_TYPE_ENI, @@ -353,6 +462,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_QUEUE, + QUEUE_ATTR_ID_LIST, + {"SAI_QUEUE_ATTR_PAUSE_STATUS"}, + {"false"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + sai->mock_get = [] (sai_object_type_t objectType, sai_object_id_t objectId, uint32_t attr_count, sai_attribute_t *attr_list) { for (uint32_t i = 0; i < attr_count; i++) { @@ -373,6 +494,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP, + PG_ATTR_ID_LIST, + {"SAI_INGRESS_PRIORITY_GROUP_ATTR_PORT"}, + {"oid:0x1"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + sai->mock_get = [] (sai_object_type_t objectType, sai_object_id_t objectId, uint32_t attr_count, sai_attribute_t *attr_list) { for (uint32_t i = 0; i < attr_count; i++) { @@ -397,6 +530,18 @@ TEST(FlexCounter, addRemoveCounter) counterVerifyFunc, false); + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_MACSEC_SA, + MACSEC_SA_ATTR_ID_LIST, + {"SAI_MACSEC_SA_ATTR_CONFIGURED_EGRESS_XPN", "SAI_MACSEC_SA_ATTR_AN"}, + {"0", "1"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); + sai->mock_get = [] (sai_object_type_t objectType, sai_object_id_t objectId, uint32_t attr_count, sai_attribute_t *attr_list) { for (uint32_t i = 0; i < attr_count; i++) { @@ -416,6 +561,18 @@ TEST(FlexCounter, addRemoveCounter) {"1000"}, counterVerifyFunc, false); + + // Bulk create mode to satisfy the coverage requirement + testAddRemoveCounter( + 1, + SAI_OBJECT_TYPE_ACL_COUNTER, + ACL_COUNTER_ATTR_ID_LIST, + {"SAI_ACL_COUNTER_ATTR_PACKETS"}, + {"1000"}, + counterVerifyFunc, + false, + STATS_MODE_READ, + true); } TEST(FlexCounter, queryCounterCapability) From fe9a9bea727c95f2bcfce30592091cac1ee548c3 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 14 Feb 2025 02:21:30 +0000 Subject: [PATCH 30/32] Fix review comments Signed-off-by: Stephen Sun --- syncd/FlexCounterManager.cpp | 3 ++- syncd/Syncd.cpp | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/syncd/FlexCounterManager.cpp b/syncd/FlexCounterManager.cpp index 121d52400..312d9286f 100644 --- a/syncd/FlexCounterManager.cpp +++ b/syncd/FlexCounterManager.cpp @@ -110,7 +110,8 @@ void FlexCounterManager::bulkAddCounter( SWSS_LOG_ENTER(); auto fc = getInstance(instanceId); - sai_object_type_t objectType = VidManager::objectTypeQuery(vids[0]); // VID and RID will have the same object type + + sai_object_type_t objectType = VidManager::objectTypeQuery(vids.at(0)); // VID and RID will have the same object type fc->bulkAddCounter(objectType, vids, rids, values); diff --git a/syncd/Syncd.cpp b/syncd/Syncd.cpp index 50de1f34e..deeaa7043 100644 --- a/syncd/Syncd.cpp +++ b/syncd/Syncd.cpp @@ -2794,7 +2794,7 @@ sai_status_t Syncd::processFlexCounterEvent( auto strVids = key.substr(delimiter + 1); auto vidStringVector = swss::tokenize(strVids, ','); - if (fromAsicChannel && op == SET_COMMAND && vidStringVector.size() > 1) + if (fromAsicChannel && op == SET_COMMAND && (!vidStringVector.empty())) { std::vector vids; std::vector rids; From 55881cb25a8fefbc2eaddefb9647c060a712b21f Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 17 Feb 2025 13:07:16 +0000 Subject: [PATCH 31/32] Fix an error in the test All counters are created in bulk mode. We should translate bulkAddObject to iteration of single call (addObject) for counter groups that do not support bulk. Signed-off-by: Stephen Sun --- syncd/FlexCounter.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index b0c71a46a..3a37334f0 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -1754,7 +1754,11 @@ class DashMeterCounterContext : public BaseCounterContext _In_ const std::string &per_object_stats_mode) override { SWSS_LOG_ENTER(); - SWSS_LOG_ERROR("It does not support to add counter in bulk mode for DashMeterCounterContext"); + + for (auto i = 0uL; i < vids.size(); i++) + { + addObject(vids[i], rids[i], idStrings, per_object_stats_mode); + } } bool hasObject() const override @@ -2756,6 +2760,14 @@ void FlexCounter::bulkAddCounter( idStrings, ""); } + else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == BUFFER_POOL_COUNTER_ID_LIST) + { + counterIds = idStrings; + } + else if (objectType == SAI_OBJECT_TYPE_BUFFER_POOL && field == STATS_MODE_FIELD) + { + statsMode = value; + } else { SWSS_LOG_ERROR("Object type and field combination is not supported, object type %s, field %s", From 213310a389bbe8bca711d4c9020b0c53498aa9fc Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 18 Feb 2025 00:31:49 +0000 Subject: [PATCH 32/32] Update test case to cover bulk create more than single create counters Signed-off-by: Stephen Sun --- unittest/syncd/TestFlexCounter.cpp | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index e5a353494..8ef4119ec 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -91,7 +91,8 @@ void testAddRemoveCounter( const std::string bulkChunkSizePerCounter = "", bool bulkChunkSizeAfterPort = true, const std::string pluginName = "", - bool immediatelyRemoveBulkChunkSizePerCounter = false) + bool immediatelyRemoveBulkChunkSizePerCounter = false, + bool forceSingleCreate = false) { SWSS_LOG_ENTER(); @@ -129,13 +130,21 @@ void testAddRemoveCounter( { fc.bulkAddCounter(object_type, object_ids, object_ids, values); } - else + else if (forceSingleCreate) { for (auto object_id : object_ids) { fc.addCounter(object_id, object_id, values); } } + else + { + for (auto object_id : object_ids) + { + std::vector tmp_object_ids = {object_id}; + fc.bulkAddCounter(object_type, tmp_object_ids, tmp_object_ids, values); + } + } if (bulkChunkSizeAfterPort) { @@ -1216,7 +1225,9 @@ TEST(FlexCounter, bulkChunksize) "3", "SAI_PORT_STAT_IF_OUT_QLEN:0;SAI_PORT_STAT_IF_IN_FEC:2", false, - PORT_PLUGIN_FIELD); + PORT_PLUGIN_FIELD, + false, + true); EXPECT_TRUE(allObjectIds.empty()); // Remove per counter bulk chunk size after initializing it