From 6e7995a2cd8caa6b476beb97e3939150bb4aca44 Mon Sep 17 00:00:00 2001 From: Junhua Zhai Date: Mon, 30 May 2022 03:16:56 +0000 Subject: [PATCH 1/5] Debug flex counter --- syncd/FlexCounter.cpp | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index e3467f42d..f4f189831 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2451,6 +2451,10 @@ sai_status_t FlexCounter::querySupportedPortCounters( SAI_OBJECT_TYPE_PORT, &stats_capability); + SWSS_LOG_NOTICE("queryStatsCapability on port RID %s: %s", + sai_serialize_object_id(portRid).c_str(), + sai_serialize_status(status).c_str()); + /* Second call is for query statistics capability */ if (status == SAI_STATUS_BUFFER_OVERFLOW) { @@ -2463,8 +2467,9 @@ sai_status_t FlexCounter::querySupportedPortCounters( if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_INFO("Unable to get port supported counters for %s", - sai_serialize_object_id(portRid).c_str()); + SWSS_LOG_NOTICE("Unable to get port supported counters for %s: %s", + sai_serialize_object_id(portRid).c_str(), + sai_serialize_status(status).c_str()); } else { @@ -2472,6 +2477,9 @@ sai_status_t FlexCounter::querySupportedPortCounters( { sai_port_stat_t counter = static_cast(statCapability.stat_enum); m_supportedPortCounters.insert(counter); + SWSS_LOG_NOTICE("queryStatsCapability counter %s on port RID %s", + sai_serialize_port_stat(counter).c_str(), + sai_serialize_object_id(portRid).c_str()); } } } @@ -2493,7 +2501,7 @@ void FlexCounter::getSupportedPortCounters( if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_INFO("Counter %s is not supported on port RID %s: %s", + SWSS_LOG_NOTICE("Counter %s is not supported on port RID %s: %s", sai_serialize_port_stat(counter).c_str(), sai_serialize_object_id(portRid).c_str(), sai_serialize_status(status).c_str()); @@ -2501,6 +2509,10 @@ void FlexCounter::getSupportedPortCounters( continue; } + SWSS_LOG_NOTICE("Counter %s is supported on port RID %s: %s", + sai_serialize_port_stat(counter).c_str(), + sai_serialize_object_id(portRid).c_str(), + sai_serialize_status(status).c_str()); m_supportedPortCounters.insert(counter); } } From 7e8f0888cabac498c563893329ff69740773d73c Mon Sep 17 00:00:00 2001 From: Junhua Zhai Date: Mon, 30 May 2022 12:21:44 +0000 Subject: [PATCH 2/5] Get supportedPortCounters per port --- syncd/FlexCounter.cpp | 55 ++++++++++++++++++------------------------- syncd/FlexCounter.h | 14 +++++------ 2 files changed, 30 insertions(+), 39 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index f4f189831..6d295cb35 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -239,14 +239,15 @@ void FlexCounter::setPortCounterList( { SWSS_LOG_ENTER(); - updateSupportedPortCounters(portId); + PortCountersSet supportedPortCounters; + updateSupportedPortCounters(portId, supportedPortCounters); // Remove unsupported counters std::vector supportedIds; for (auto &counter : counterIds) { - if (isPortCounterSupported(counter)) + if (supportedPortCounters.count(counter) != 0) { supportedIds.push_back(counter); } @@ -1361,13 +1362,6 @@ bool FlexCounter::allPluginsEmpty() const m_flowCounterPlugins.empty(); } -bool FlexCounter::isPortCounterSupported(sai_port_stat_t counter) const -{ - SWSS_LOG_ENTER(); - - return m_supportedPortCounters.count(counter) != 0; -} - bool FlexCounter::isQueueCounterSupported( _In_ sai_queue_stat_t counter) const { @@ -2437,7 +2431,7 @@ void FlexCounter::endFlexCounterThread(void) } sai_status_t FlexCounter::querySupportedPortCounters( - _In_ sai_object_id_t portRid) + _In_ sai_object_id_t portRid, PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); @@ -2451,9 +2445,9 @@ sai_status_t FlexCounter::querySupportedPortCounters( SAI_OBJECT_TYPE_PORT, &stats_capability); - SWSS_LOG_NOTICE("queryStatsCapability on port RID %s: %s", - sai_serialize_object_id(portRid).c_str(), - sai_serialize_status(status).c_str()); + SWSS_LOG_NOTICE("queryStatsCapability on port RID %s: %s", + sai_serialize_object_id(portRid).c_str(), + sai_serialize_status(status).c_str()); /* Second call is for query statistics capability */ if (status == SAI_STATUS_BUFFER_OVERFLOW) @@ -2469,17 +2463,17 @@ sai_status_t FlexCounter::querySupportedPortCounters( { SWSS_LOG_NOTICE("Unable to get port supported counters for %s: %s", sai_serialize_object_id(portRid).c_str(), - sai_serialize_status(status).c_str()); + sai_serialize_status(status).c_str()); } else { for (auto statCapability: statCapabilityList) { sai_port_stat_t counter = static_cast(statCapability.stat_enum); - m_supportedPortCounters.insert(counter); - SWSS_LOG_NOTICE("queryStatsCapability counter %s on port RID %s", - sai_serialize_port_stat(counter).c_str(), - sai_serialize_object_id(portRid).c_str()); + supportedPortCounters.insert(counter); + SWSS_LOG_NOTICE("queryStatsCapability counter %s on port RID %s", + sai_serialize_port_stat(counter).c_str(), + sai_serialize_object_id(portRid).c_str()); } } } @@ -2487,7 +2481,8 @@ sai_status_t FlexCounter::querySupportedPortCounters( } void FlexCounter::getSupportedPortCounters( - _In_ sai_object_id_t portRid) + _In_ sai_object_id_t portRid, + PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); @@ -2509,30 +2504,26 @@ void FlexCounter::getSupportedPortCounters( continue; } - SWSS_LOG_NOTICE("Counter %s is supported on port RID %s: %s", - sai_serialize_port_stat(counter).c_str(), - sai_serialize_object_id(portRid).c_str(), - sai_serialize_status(status).c_str()); - m_supportedPortCounters.insert(counter); + SWSS_LOG_NOTICE("Counter %s is supported on port RID %s: %s", + sai_serialize_port_stat(counter).c_str(), + sai_serialize_object_id(portRid).c_str(), + sai_serialize_status(status).c_str()); + supportedPortCounters.insert(counter); } } void FlexCounter::updateSupportedPortCounters( - _In_ sai_object_id_t portRid) + _In_ sai_object_id_t portRid, + PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); - if (m_supportedPortCounters.size()) - { - return; - } - /* Query SAI supported port counters */ - sai_status_t status = querySupportedPortCounters(portRid); + sai_status_t status = querySupportedPortCounters(portRid, supportedPortCounters); if (status != SAI_STATUS_SUCCESS) { /* Fallback to legacy approach */ - getSupportedPortCounters(portRid); + getSupportedPortCounters(portRid, supportedPortCounters); } } diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index 1eb5e8a62..6ae52a420 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -20,6 +20,7 @@ namespace syncd { private: + typedef std::set PortCountersSet; FlexCounter(const FlexCounter&) = delete; public: @@ -213,9 +214,6 @@ namespace syncd private: // is counter supported - bool isPortCounterSupported( - _In_ sai_port_stat_t counter) const; - bool isPriorityGroupCounterSupported( _In_ sai_ingress_priority_group_stat_t counter) const; @@ -238,13 +236,16 @@ namespace syncd private: // update supported counters sai_status_t querySupportedPortCounters( - _In_ sai_object_id_t portRid); + _In_ sai_object_id_t portRid, + PortCountersSet &supportedPortCounters); void getSupportedPortCounters( - _In_ sai_object_id_t portRid); + _In_ sai_object_id_t portRid, + PortCountersSet &supportedPortCounters); void updateSupportedPortCounters( - _In_ sai_object_id_t portRid); + _In_ sai_object_id_t portRid, + PortCountersSet &supportedPortCounters); std::vector saiCheckSupportedPortDebugCounters( _In_ sai_object_id_t portRid, @@ -545,7 +546,6 @@ namespace syncd private: // supported counters - std::set m_supportedPortCounters; std::set m_supportedPriorityGroupCounters; std::set m_supportedQueueCounters; std::set m_supportedRifCounters; From 5628744c88a937dca095bf3d61cffd9e3404b0ea Mon Sep 17 00:00:00 2001 From: Junhua Zhai Date: Fri, 3 Jun 2022 02:04:48 +0000 Subject: [PATCH 3/5] Remove some debug logs --- syncd/FlexCounter.cpp | 19 ++++--------------- syncd/FlexCounter.h | 8 ++++---- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 6d295cb35..6d37fe384 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2431,7 +2431,7 @@ void FlexCounter::endFlexCounterThread(void) } sai_status_t FlexCounter::querySupportedPortCounters( - _In_ sai_object_id_t portRid, PortCountersSet &supportedPortCounters) + _In_ sai_object_id_t portRid, _Out_ PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); @@ -2445,10 +2445,6 @@ sai_status_t FlexCounter::querySupportedPortCounters( SAI_OBJECT_TYPE_PORT, &stats_capability); - SWSS_LOG_NOTICE("queryStatsCapability on port RID %s: %s", - sai_serialize_object_id(portRid).c_str(), - sai_serialize_status(status).c_str()); - /* Second call is for query statistics capability */ if (status == SAI_STATUS_BUFFER_OVERFLOW) { @@ -2461,7 +2457,7 @@ sai_status_t FlexCounter::querySupportedPortCounters( if (status != SAI_STATUS_SUCCESS) { - SWSS_LOG_NOTICE("Unable to get port supported counters for %s: %s", + SWSS_LOG_NOTICE("Unable to query port supported counters for %s: %s", sai_serialize_object_id(portRid).c_str(), sai_serialize_status(status).c_str()); } @@ -2471,9 +2467,6 @@ sai_status_t FlexCounter::querySupportedPortCounters( { sai_port_stat_t counter = static_cast(statCapability.stat_enum); supportedPortCounters.insert(counter); - SWSS_LOG_NOTICE("queryStatsCapability counter %s on port RID %s", - sai_serialize_port_stat(counter).c_str(), - sai_serialize_object_id(portRid).c_str()); } } } @@ -2482,7 +2475,7 @@ sai_status_t FlexCounter::querySupportedPortCounters( void FlexCounter::getSupportedPortCounters( _In_ sai_object_id_t portRid, - PortCountersSet &supportedPortCounters) + _Out_ PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); @@ -2504,17 +2497,13 @@ void FlexCounter::getSupportedPortCounters( continue; } - SWSS_LOG_NOTICE("Counter %s is supported on port RID %s: %s", - sai_serialize_port_stat(counter).c_str(), - sai_serialize_object_id(portRid).c_str(), - sai_serialize_status(status).c_str()); supportedPortCounters.insert(counter); } } void FlexCounter::updateSupportedPortCounters( _In_ sai_object_id_t portRid, - PortCountersSet &supportedPortCounters) + _Out_ PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); diff --git a/syncd/FlexCounter.h b/syncd/FlexCounter.h index 6ae52a420..cfc79b660 100644 --- a/syncd/FlexCounter.h +++ b/syncd/FlexCounter.h @@ -20,7 +20,6 @@ namespace syncd { private: - typedef std::set PortCountersSet; FlexCounter(const FlexCounter&) = delete; public: @@ -235,17 +234,18 @@ namespace syncd private: // update supported counters + typedef std::set PortCountersSet; sai_status_t querySupportedPortCounters( _In_ sai_object_id_t portRid, - PortCountersSet &supportedPortCounters); + _Out_ PortCountersSet &supportedPortCounters); void getSupportedPortCounters( _In_ sai_object_id_t portRid, - PortCountersSet &supportedPortCounters); + _Out_ PortCountersSet &supportedPortCounters); void updateSupportedPortCounters( _In_ sai_object_id_t portRid, - PortCountersSet &supportedPortCounters); + _Out_ PortCountersSet &supportedPortCounters); std::vector saiCheckSupportedPortDebugCounters( _In_ sai_object_id_t portRid, From ad1a46c6a2260e06d12a22aa05cc6b0f8c59b758 Mon Sep 17 00:00:00 2001 From: Junhua Zhai Date: Fri, 3 Jun 2022 04:11:51 +0000 Subject: [PATCH 4/5] Add unit test addRemoveCounterForPort --- unittest/syncd/MockableSaiInterface.cpp | 2 +- unittest/syncd/TestFlexCounter.cpp | 79 +++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/unittest/syncd/MockableSaiInterface.cpp b/unittest/syncd/MockableSaiInterface.cpp index 30b52a2d3..c96764e3a 100644 --- a/unittest/syncd/MockableSaiInterface.cpp +++ b/unittest/syncd/MockableSaiInterface.cpp @@ -165,7 +165,7 @@ sai_status_t MockableSaiInterface::queryStatsCapability( return mock_queryStatsCapability(switch_id, object_type, stats_capability); } - return SAI_STATUS_SUCCESS; + return SAI_STATUS_NOT_SUPPORTED; } sai_status_t MockableSaiInterface::getStatsExt( diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 66a9d60a7..017df6ba8 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -186,3 +186,82 @@ TEST(FlexCounter, addRemoveCounterForMACsecSA) } +TEST(FlexCounter, addRemoveCounterForPort) +{ + std::shared_ptr sai(new MockableSaiInterface()); + FlexCounter fc("test", sai, "COUNTERS_DB"); + + sai_object_id_t counterVid{100}; + sai_object_id_t counterRid{100}; + std::vector values; + values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_ERRORS"); + + test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); + sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { + for (uint32_t i = 0; i < number_of_counters; i++) + { + counters[i] = (i + 1) * 100; + } + return SAI_STATUS_SUCCESS; + }; + + fc.addCounter(counterVid, counterRid, values); + EXPECT_EQ(fc.isEmpty(), false); + + values.clear(); + values.emplace_back(POLL_INTERVAL_FIELD, "1000"); + values.emplace_back(FLEX_COUNTER_STATUS_FIELD, "enable"); + fc.addCounterPlugin(values); + + usleep(1000*1000); + swss::DBConnector db("COUNTERS_DB", 0); + swss::RedisPipeline pipeline(&db); + swss::Table countersTable(&pipeline, COUNTERS_TABLE, false); + + std::vector keys; + countersTable.getKeys(keys); + EXPECT_EQ(keys.size(), size_t(1)); + EXPECT_EQ(keys[0], "oid:0x64"); + + std::string value; + countersTable.hget("oid:0x64", "SAI_PORT_STAT_IF_IN_OCTETS", value); + EXPECT_EQ(value, "100"); + countersTable.hget("oid:0x64", "SAI_PORT_STAT_IF_IN_ERRORS", value); + EXPECT_EQ(value, "200"); + + fc.removeCounter(counterVid); + EXPECT_EQ(fc.isEmpty(), true); + countersTable.del("oid:0x64"); + countersTable.getKeys(keys); + ASSERT_TRUE(keys.empty()); + + // Test again with queryStatsCapability support + sai->mock_queryStatsCapability = [](sai_object_id_t, sai_object_type_t, sai_stat_capability_list_t *capability) { + if (capability->count < 2) + { + capability->count = 2; + return SAI_STATUS_BUFFER_OVERFLOW; + } + + capability->list[0].stat_enum = SAI_PORT_STAT_IF_IN_OCTETS; + capability->list[1].stat_enum = SAI_PORT_STAT_IF_IN_ERRORS; + return SAI_STATUS_SUCCESS; + }; + + values.clear(); + values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_ERRORS"); + fc.addCounter(counterVid, counterRid, values); + EXPECT_EQ(fc.isEmpty(), false); + + usleep(1000*1000); + countersTable.hget("oid:0x64", "SAI_PORT_STAT_IF_IN_OCTETS", value); + EXPECT_EQ(value, "100"); + countersTable.hget("oid:0x64", "SAI_PORT_STAT_IF_IN_ERRORS", value); + EXPECT_EQ(value, "200"); + + fc.removeCounter(counterVid); + EXPECT_EQ(fc.isEmpty(), true); + countersTable.del("oid:0x64"); + countersTable.getKeys(keys); + ASSERT_TRUE(keys.empty()); +} From 02f6844e68d51d05cb52f3147f017665a98bab4a Mon Sep 17 00:00:00 2001 From: Junhua Zhai Date: Thu, 9 Jun 2022 02:01:38 +0000 Subject: [PATCH 5/5] Fix comments --- syncd/FlexCounter.cpp | 3 ++- unittest/syncd/TestFlexCounter.cpp | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/syncd/FlexCounter.cpp b/syncd/FlexCounter.cpp index 6d37fe384..356668ab0 100644 --- a/syncd/FlexCounter.cpp +++ b/syncd/FlexCounter.cpp @@ -2431,7 +2431,8 @@ void FlexCounter::endFlexCounterThread(void) } sai_status_t FlexCounter::querySupportedPortCounters( - _In_ sai_object_id_t portRid, _Out_ PortCountersSet &supportedPortCounters) + _In_ sai_object_id_t portRid, + _Out_ PortCountersSet &supportedPortCounters) { SWSS_LOG_ENTER(); diff --git a/unittest/syncd/TestFlexCounter.cpp b/unittest/syncd/TestFlexCounter.cpp index 017df6ba8..090b7c533 100644 --- a/unittest/syncd/TestFlexCounter.cpp +++ b/unittest/syncd/TestFlexCounter.cpp @@ -197,10 +197,21 @@ TEST(FlexCounter, addRemoveCounterForPort) values.emplace_back(PORT_COUNTER_ID_LIST, "SAI_PORT_STAT_IF_IN_OCTETS,SAI_PORT_STAT_IF_IN_ERRORS"); test_syncd::mockVidManagerObjectTypeQuery(SAI_OBJECT_TYPE_PORT); - sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *, uint64_t *counters) { + sai->mock_getStats = [](sai_object_type_t, sai_object_id_t, uint32_t number_of_counters, const sai_stat_id_t *ids, uint64_t *counters) { for (uint32_t i = 0; i < number_of_counters; i++) { - counters[i] = (i + 1) * 100; + if (ids[i] == SAI_PORT_STAT_IF_IN_OCTETS) + { + counters[i] = 100; + } + else if (ids[i] == SAI_PORT_STAT_IF_IN_ERRORS) + { + counters[i] = 200; + } + else + { + return SAI_STATUS_FAILURE; + } } return SAI_STATUS_SUCCESS; };