From 8a6fde7fa0c78d5ce74666a62fe46710018a1263 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 6 Aug 2021 11:25:18 +0800 Subject: [PATCH 1/6] [Bugfix] Don't query SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES if it is not supported by a pool Currently, SAI_BUFFER_POOL_STAT_WATERMARK_BYTES and SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES are queried for buffer pools. However, the latter is not supported on all pools and all platforms, which will results in sairedis complaint To avoid that, we need to test whether it is supported before putting it to FLEX_COUNTER table Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 57 ++++++++++++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 5aa42f4265..7b48023ddb 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -23,7 +23,11 @@ extern sai_object_id_t gSwitchId; static const vector bufferPoolWatermarkStatIds = { - SAI_BUFFER_POOL_STAT_WATERMARK_BYTES, + SAI_BUFFER_POOL_STAT_WATERMARK_BYTES +}; + +static const vector bufferSharedHeadroomPoolWatermarkStatIds = +{ SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES }; @@ -218,22 +222,36 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) } // Detokenize the SAI watermark stats to a string, separated by comma - string statList; + string statListPoolOnly; for (const auto &it : bufferPoolWatermarkStatIds) { - statList += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); + statListPoolOnly += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); } - if (!statList.empty()) + string statListPoolAndShp = statListPoolOnly; + for (const auto &it : bufferSharedHeadroomPoolWatermarkStatIds) { - statList.pop_back(); + statListPoolAndShp += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); + } + if (!statListPoolOnly.empty()) + { + statListPoolOnly.pop_back(); + } + if (!statListPoolAndShp.empty()) + { + statListPoolAndShp.pop_back(); } // Some platforms do not support buffer pool watermark clear operation on a particular pool // Invoke the SAI clear_stats API per pool to query the capability from the API call return status + // Some platforms do not support shared headroom pool watermark read operation on a particular pool + // Invoke the SAI get_buffer_pool_stats_ext API per pool to query the capability from the API call return status. // We use bit mask to mark the clear watermark capability of each buffer pool. We use an unsigned int to place hold // these bits. This assumes the total number of buffer pools to be no greater than 32, which should satisfy all use cases. unsigned int noWmClrCapability = 0; + unsigned int noShpWmRdCapability = 0; unsigned int bitMask = 1; + uint32_t size = static_cast(bufferSharedHeadroomPoolWatermarkStatIds.size()); + vector counterData(size); for (const auto &it : *(m_buffer_type_maps[APP_BUFFER_POOL_TABLE_NAME])) { sai_status_t status = sai_buffer_api->clear_buffer_pool_stats( @@ -246,6 +264,19 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) noWmClrCapability |= bitMask; } + // Check whether shared headroom pool water mark is supported + status = sai_buffer_api->get_buffer_pool_stats_ext( + it.second.m_saiObjectId, + size, + reinterpret_cast(bufferSharedHeadroomPoolWatermarkStatIds.data()), + SAI_STATS_MODE_READ, + counterData.data()); + if (status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) + { + SWSS_LOG_NOTICE("Read shared headroom pool watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); + noShpWmRdCapability |= bitMask; + } + bitMask <<= 1; } @@ -259,11 +290,21 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) // Push buffer pool watermark COUNTER_ID_LIST to FLEX_COUNTER_TABLE on a per buffer pool basis vector fvTuples; - fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statList); + bitMask = 1; for (const auto &it : *(m_buffer_type_maps[APP_BUFFER_POOL_TABLE_NAME])) { string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(it.second.m_saiObjectId); + fvTuples.clear(); + + if (noShpWmRdCapability & bitMask) + { + fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListPoolOnly); + } + else + { + fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListPoolAndShp); + } if (noWmClrCapability) { @@ -275,13 +316,13 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) fvTuples.emplace_back(STATS_MODE_FIELD, stats_mode); m_flexCounterTable->set(key, fvTuples); - fvTuples.pop_back(); - bitMask <<= 1; } else { m_flexCounterTable->set(key, fvTuples); } + + bitMask <<= 1; } m_isBufferPoolWatermarkCounterIdListGenerated = true; From d58455be0a638740954240f3e108db9fa9928f4a Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 6 Aug 2021 05:21:48 +0000 Subject: [PATCH 2/6] Fix comments Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 7b48023ddb..d98c73d3e9 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -258,7 +258,8 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) it.second.m_saiObjectId, static_cast(bufferPoolWatermarkStatIds.size()), reinterpret_cast(bufferPoolWatermarkStatIds.data())); - if (status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) + if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) + || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) { SWSS_LOG_NOTICE("Clear watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); noWmClrCapability |= bitMask; @@ -271,7 +272,8 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) reinterpret_cast(bufferSharedHeadroomPoolWatermarkStatIds.data()), SAI_STATS_MODE_READ, counterData.data()); - if (status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) + if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) + || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) { SWSS_LOG_NOTICE("Read shared headroom pool watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); noShpWmRdCapability |= bitMask; From 87cf819618a93dcf6ed6b543b6230107911f63e3 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 11 Aug 2021 17:06:21 +0800 Subject: [PATCH 3/6] Use get_buffer_pool_stats instead of get_buffer_pool_stats_ext The latter is not supported by all vendors Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index d98c73d3e9..3912ad4e5f 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -266,11 +266,10 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) } // Check whether shared headroom pool water mark is supported - status = sai_buffer_api->get_buffer_pool_stats_ext( + status = sai_buffer_api->get_buffer_pool_stats( it.second.m_saiObjectId, size, reinterpret_cast(bufferSharedHeadroomPoolWatermarkStatIds.data()), - SAI_STATS_MODE_READ, counterData.data()); if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) From fca084b5084d86ebc22c1f5959b53dd6730d0f14 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Thu, 26 Aug 2021 03:07:07 +0000 Subject: [PATCH 4/6] Change the counter IDs being tested before enable buffer pool watermark polling - If shared headroom pool watermark is not supported, only buffer pool watermark will be tested - Otherwise, both buffer pool watermark and shared headroom pool watermark will be checked Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 3912ad4e5f..36871e181d 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -31,6 +31,12 @@ static const vector bufferSharedHeadroomPoolWatermarkSta SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES }; +static const vector bufferPoolAllWatermarkStatIds = +{ + SAI_BUFFER_POOL_STAT_WATERMARK_BYTES, + SAI_BUFFER_POOL_STAT_XOFF_ROOM_WATERMARK_BYTES +}; + type_map BufferOrch::m_buffer_type_maps = { {APP_BUFFER_POOL_TABLE_NAME, new object_reference_map()}, {APP_BUFFER_PROFILE_TABLE_NAME, new object_reference_map()}, @@ -254,17 +260,7 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) vector counterData(size); for (const auto &it : *(m_buffer_type_maps[APP_BUFFER_POOL_TABLE_NAME])) { - sai_status_t status = sai_buffer_api->clear_buffer_pool_stats( - it.second.m_saiObjectId, - static_cast(bufferPoolWatermarkStatIds.size()), - reinterpret_cast(bufferPoolWatermarkStatIds.data())); - if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) - || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) - { - SWSS_LOG_NOTICE("Clear watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); - noWmClrCapability |= bitMask; - } - + sai_status_t status; // Check whether shared headroom pool water mark is supported status = sai_buffer_api->get_buffer_pool_stats( it.second.m_saiObjectId, @@ -278,6 +274,19 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) noShpWmRdCapability |= bitMask; } + const auto &watermarkStatIds = (noShpWmRdCapability & bitMask) ? bufferPoolAllWatermarkStatIds : bufferPoolWatermarkStatIds; + + status = sai_buffer_api->clear_buffer_pool_stats( + it.second.m_saiObjectId, + static_cast(watermarkStatIds.size()), + reinterpret_cast(watermarkStatIds.data())); + if (SAI_STATUS_IS_ATTR_NOT_SUPPORTED(status) || SAI_STATUS_IS_ATTR_NOT_IMPLEMENTED(status) + || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) + { + SWSS_LOG_NOTICE("Clear watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); + noWmClrCapability |= bitMask; + } + bitMask <<= 1; } From 628949efa268dfb367a5bf263daf1d6080e2787b Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Sat, 28 Aug 2021 06:40:26 +0800 Subject: [PATCH 5/6] Update bufferorch.cpp Fix review comments. --- orchagent/bufferorch.cpp | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 36871e181d..6f25e00b83 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -250,7 +250,7 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) // Some platforms do not support buffer pool watermark clear operation on a particular pool // Invoke the SAI clear_stats API per pool to query the capability from the API call return status // Some platforms do not support shared headroom pool watermark read operation on a particular pool - // Invoke the SAI get_buffer_pool_stats_ext API per pool to query the capability from the API call return status. + // Invoke the SAI get_buffer_pool_stats API per pool to query the capability from the API call return status. // We use bit mask to mark the clear watermark capability of each buffer pool. We use an unsigned int to place hold // these bits. This assumes the total number of buffer pools to be no greater than 32, which should satisfy all use cases. unsigned int noWmClrCapability = 0; @@ -274,7 +274,7 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) noShpWmRdCapability |= bitMask; } - const auto &watermarkStatIds = (noShpWmRdCapability & bitMask) ? bufferPoolAllWatermarkStatIds : bufferPoolWatermarkStatIds; + const auto &watermarkStatIds = (noShpWmRdCapability & bitMask) ? bufferPoolWatermarkStatIds : bufferPoolAllWatermarkStatIds; status = sai_buffer_api->clear_buffer_pool_stats( it.second.m_saiObjectId, @@ -324,14 +324,10 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) stats_mode = STATS_MODE_READ; } fvTuples.emplace_back(STATS_MODE_FIELD, stats_mode); - - m_flexCounterTable->set(key, fvTuples); - } - else - { - m_flexCounterTable->set(key, fvTuples); } + m_flexCounterTable->set(key, fvTuples); + bitMask <<= 1; } From c7835cc89d79a0bfee182f7807a2630050ebe8f9 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 30 Aug 2021 06:49:45 +0000 Subject: [PATCH 6/6] Replace all "Shp" to "SharedHeadroomPool" for better understanding Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 6f25e00b83..5e9eacba98 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -228,23 +228,23 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) } // Detokenize the SAI watermark stats to a string, separated by comma - string statListPoolOnly; + string statListBufferPoolOnly; for (const auto &it : bufferPoolWatermarkStatIds) { - statListPoolOnly += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); + statListBufferPoolOnly += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); } - string statListPoolAndShp = statListPoolOnly; + string statListBufferPoolAndSharedHeadroomPool = statListBufferPoolOnly; for (const auto &it : bufferSharedHeadroomPoolWatermarkStatIds) { - statListPoolAndShp += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); + statListBufferPoolAndSharedHeadroomPool += (sai_serialize_buffer_pool_stat(it) + list_item_delimiter); } - if (!statListPoolOnly.empty()) + if (!statListBufferPoolOnly.empty()) { - statListPoolOnly.pop_back(); + statListBufferPoolOnly.pop_back(); } - if (!statListPoolAndShp.empty()) + if (!statListBufferPoolAndSharedHeadroomPool.empty()) { - statListPoolAndShp.pop_back(); + statListBufferPoolAndSharedHeadroomPool.pop_back(); } // Some platforms do not support buffer pool watermark clear operation on a particular pool @@ -254,7 +254,7 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) // We use bit mask to mark the clear watermark capability of each buffer pool. We use an unsigned int to place hold // these bits. This assumes the total number of buffer pools to be no greater than 32, which should satisfy all use cases. unsigned int noWmClrCapability = 0; - unsigned int noShpWmRdCapability = 0; + unsigned int noSharedHeadroomPoolWmRdCapability = 0; unsigned int bitMask = 1; uint32_t size = static_cast(bufferSharedHeadroomPoolWatermarkStatIds.size()); vector counterData(size); @@ -271,10 +271,10 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) || status == SAI_STATUS_NOT_SUPPORTED || status == SAI_STATUS_NOT_IMPLEMENTED) { SWSS_LOG_NOTICE("Read shared headroom pool watermark failed on %s, rv: %s", it.first.c_str(), sai_serialize_status(status).c_str()); - noShpWmRdCapability |= bitMask; + noSharedHeadroomPoolWmRdCapability |= bitMask; } - const auto &watermarkStatIds = (noShpWmRdCapability & bitMask) ? bufferPoolWatermarkStatIds : bufferPoolAllWatermarkStatIds; + const auto &watermarkStatIds = (noSharedHeadroomPoolWmRdCapability & bitMask) ? bufferPoolWatermarkStatIds : bufferPoolAllWatermarkStatIds; status = sai_buffer_api->clear_buffer_pool_stats( it.second.m_saiObjectId, @@ -307,13 +307,13 @@ void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(it.second.m_saiObjectId); fvTuples.clear(); - if (noShpWmRdCapability & bitMask) + if (noSharedHeadroomPoolWmRdCapability & bitMask) { - fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListPoolOnly); + fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListBufferPoolOnly); } else { - fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListPoolAndShp); + fvTuples.emplace_back(BUFFER_POOL_COUNTER_ID_LIST, statListBufferPoolAndSharedHeadroomPool); } if (noWmClrCapability)