From 882682dff3de57a250168bf3b914eab111037341 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Mon, 1 Nov 2021 23:08:39 +0800 Subject: [PATCH 1/8] [Reclaim buffer] Common code update 1. Loading zero_profiles when dynamic buffer manager starting The buffer manager won't consume it for now. This is to pass Azure CI. 2. Support removing a buffer pool. 3. Support exposing maximum PGs and queues per port 4. Support transmit between bitmap and map string Signed-off-by: Stephen Sun --- cfgmgr/buffermgrd.cpp | 37 +++++++---- cfgmgr/buffermgrdyn.cpp | 2 +- cfgmgr/buffermgrdyn.h | 2 +- orchagent/bufferorch.cpp | 7 ++ orchagent/bufferorch.h | 1 + orchagent/orch.cpp | 134 ++++++++++++++++++++++++++++++++++++++- orchagent/orch.h | 3 + orchagent/portsorch.cpp | 15 ++++- orchagent/portsorch.h | 1 + 9 files changed, 184 insertions(+), 18 deletions(-) diff --git a/cfgmgr/buffermgrd.cpp b/cfgmgr/buffermgrd.cpp index 9f4d979237..74e88f7337 100644 --- a/cfgmgr/buffermgrd.cpp +++ b/cfgmgr/buffermgrd.cpp @@ -38,12 +38,13 @@ mutex gDbMutex; void usage() { - cout << "Usage: buffermgrd <-l pg_lookup.ini|-a asic_table.json [-p peripheral_table.json]>" << endl; + cout << "Usage: buffermgrd <-l pg_lookup.ini|-a asic_table.json [-p peripheral_table.json] [-z zero_profiles.json]>" << endl; cout << " -l pg_lookup.ini: PG profile look up table file (mandatory for static mode)" << endl; cout << " format: csv" << endl; cout << " values: 'speed, cable, size, xon, xoff, dynamic_threshold, xon_offset'" << endl; cout << " -a asic_table.json: ASIC-specific parameters definition (mandatory for dynamic mode)" << endl; - cout << " -p peripheral_table.json: Peripheral (eg. gearbox) parameters definition (mandatory for dynamic mode)" << endl; + cout << " -p peripheral_table.json: Peripheral (eg. gearbox) parameters definition (optional for dynamic mode)" << endl; + cout << " -z zero_profiles.json: Zero profiles definition for reclaiming unused buffers (optional for dynamic mode)" << endl; } void dump_db_item(KeyOpFieldsValuesTuple &db_item) @@ -109,13 +110,13 @@ int main(int argc, char **argv) string pg_lookup_file = ""; string asic_table_file = ""; string peripherial_table_file = ""; - string json_file = ""; + string zero_profile_file = ""; Logger::linkToDbNative("buffermgrd"); SWSS_LOG_ENTER(); SWSS_LOG_NOTICE("--- Starting buffermgrd ---"); - while ((opt = getopt(argc, argv, "l:a:p:h")) != -1 ) + while ((opt = getopt(argc, argv, "l:a:p:z:h")) != -1 ) { switch (opt) { @@ -131,6 +132,9 @@ int main(int argc, char **argv) case 'p': peripherial_table_file = optarg; break; + case 'z': + zero_profile_file = optarg; + break; default: /* '?' */ usage(); return EXIT_FAILURE; @@ -141,7 +145,9 @@ int main(int argc, char **argv) { std::vector cfgOrchList; bool dynamicMode = false; - shared_ptr> db_items_ptr; + shared_ptr> asic_table_ptr = nullptr; + shared_ptr> peripherial_table_ptr = nullptr; + shared_ptr> zero_profiles_ptr = nullptr; DBConnector cfgDb("CONFIG_DB", 0); DBConnector stateDb("STATE_DB", 0); @@ -150,18 +156,23 @@ int main(int argc, char **argv) if (!asic_table_file.empty()) { // Load the json file containing the SWITCH_TABLE - db_items_ptr = load_json(asic_table_file); - if (nullptr != db_items_ptr) + asic_table_ptr = load_json(asic_table_file); + if (nullptr != asic_table_ptr) { - write_to_state_db(db_items_ptr); - db_items_ptr.reset(); + write_to_state_db(asic_table_ptr); if (!peripherial_table_file.empty()) { //Load the json file containing the PERIPHERIAL_TABLE - db_items_ptr = load_json(peripherial_table_file); - if (nullptr != db_items_ptr) - write_to_state_db(db_items_ptr); + peripherial_table_ptr = load_json(peripherial_table_file); + if (nullptr != peripherial_table_ptr) + write_to_state_db(peripherial_table_ptr); + } + + if (!zero_profile_file.empty()) + { + //Load the json file containing the zero profiles + zero_profiles_ptr = load_json(zero_profile_file); } dynamicMode = true; @@ -183,7 +194,7 @@ int main(int argc, char **argv) TableConnector(&stateDb, STATE_BUFFER_MAXIMUM_VALUE_TABLE), TableConnector(&stateDb, STATE_PORT_TABLE_NAME) }; - cfgOrchList.emplace_back(new BufferMgrDynamic(&cfgDb, &stateDb, &applDb, buffer_table_connectors, db_items_ptr)); + cfgOrchList.emplace_back(new BufferMgrDynamic(&cfgDb, &stateDb, &applDb, buffer_table_connectors, peripherial_table_ptr, zero_profiles_ptr)); } else if (!pg_lookup_file.empty()) { diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index f1bbcd395a..a1c64b7e5e 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -27,7 +27,7 @@ using namespace std; using namespace swss; -BufferMgrDynamic::BufferMgrDynamic(DBConnector *cfgDb, DBConnector *stateDb, DBConnector *applDb, const vector &tables, shared_ptr> gearboxInfo = nullptr) : +BufferMgrDynamic::BufferMgrDynamic(DBConnector *cfgDb, DBConnector *stateDb, DBConnector *applDb, const vector &tables, shared_ptr> gearboxInfo, shared_ptr> zeroProfilesInfo) : Orch(tables), m_platform(), m_applDb(applDb), diff --git a/cfgmgr/buffermgrdyn.h b/cfgmgr/buffermgrdyn.h index 68a614bbf8..fa1c69dbc2 100644 --- a/cfgmgr/buffermgrdyn.h +++ b/cfgmgr/buffermgrdyn.h @@ -128,7 +128,7 @@ typedef std::map gearbox_delay_t; class BufferMgrDynamic : public Orch { public: - BufferMgrDynamic(DBConnector *cfgDb, DBConnector *stateDb, DBConnector *applDb, const std::vector &tables, std::shared_ptr> gearboxInfo); + BufferMgrDynamic(DBConnector *cfgDb, DBConnector *stateDb, DBConnector *applDb, const std::vector &tables, std::shared_ptr> gearboxInfo, std::shared_ptr> zeroProfilesInfo); using Orch::doTask; private: diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 436926eb91..8115f6eef7 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -210,6 +210,12 @@ bool BufferOrch::isPortReady(const std::string& port_name) const return result; } +void BufferOrch::clearBufferPoolWatermarkCounterCounterIdList(const sai_object_id_t object_id) +{ + string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(object_id); + m_flexCounterTable->del(key); +} + void BufferOrch::generateBufferPoolWatermarkCounterIdList(void) { // This function will be called in FlexCounterOrch when field:value tuple "FLEX_COUNTER_STATUS":"enable" @@ -460,6 +466,7 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple) if (SAI_NULL_OBJECT_ID != sai_object) { + clearBufferPoolWatermarkCounterCounterIdList(sai_object); sai_status = sai_buffer_api->remove_buffer_pool(sai_object); if (SAI_STATUS_SUCCESS != sai_status) { diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index ef4153e6a4..b70fc9638f 100644 --- a/orchagent/bufferorch.h +++ b/orchagent/bufferorch.h @@ -45,6 +45,7 @@ class BufferOrch : public Orch void doTask() override; virtual void doTask(Consumer& consumer); + void clearBufferPoolWatermarkCounterCounterIdList(const sai_object_id_t object_id); void initTableHandlers(); void initBufferReadyLists(DBConnector *confDb, DBConnector *applDb); void initBufferReadyList(Table& table, bool isConfigDb); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 226512f388..c606801d04 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -585,7 +585,7 @@ ref_resolve_status Orch::resolveFieldRefArray( { if (!parseReference(type_maps, list_items[ind], ref_type_name, object_name)) { - SWSS_LOG_ERROR("Failed to parse profile reference:%s\n", list_items[ind].c_str()); + SWSS_LOG_NOTICE("Failed to parse profile reference:%s\n", list_items[ind].c_str()); return ref_resolve_status::not_resolved; } sai_object_id_t sai_obj = (*(type_maps[ref_type_name]))[object_name].m_saiObjectId; @@ -635,6 +635,138 @@ bool Orch::parseIndexRange(const string &input, sai_uint32_t &range_low, sai_uin return true; } +/* + * generateBitMapFromIdsStr + * + * Generates the bit map representing the idsMap in string + * Args: + * idsStr: The string representing the IDs. + * Typically it's part of a key of BUFFER_QUEUE or BUFFER_PG, like "3-4" from "Ethernet0|3-4" + * Return: + * idsMap: The bitmap of IDs. The LSB stands for ID 0. + * + * Example: + * Input idsMap: 3-4 + * Return: 00001100b + */ +unsigned long Orch::generateBitMapFromIdsStr(const string &idsStr) +{ + sai_uint32_t lowerBound, upperBound; + unsigned long idsMap = 0; + + if (!parseIndexRange(idsStr, lowerBound, upperBound)) + return 0; + + for (sai_uint32_t id = lowerBound; id <= upperBound; id ++) + { + idsMap |= (1 << id); + } + + return idsMap; +} + +/* + * generateIdListFromMap + * + * Parse the idsMap and generate a vector which contains slices representing bits in idsMap + * Args: + * idsMap: The bitmap of IDs. The LSB stands for ID 0. + * maxId: The maximum value of ID. + * Return: + * A vector which contains slices representing bits in idsMap + * + * Example: + * Input idsMap: 00100110b, maxId: 8 + * Return vector: ["1-2", "5"] + */ +set Orch::generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId) +{ + unsigned long currentIdMask = 1; + bool started = false, needGenerateMap = false; + sai_uint32_t lower, upper; + set extraIdsToReclaim; + for (sai_uint32_t id = 0; id <= maxId; id ++) + { + // currentIdMask represents the bit mask corresponding to id: (1<getDbId() == CONFIG_DB || db->getDbId() == STATE_DB || db->getDbId() == CHASSIS_APP_DB) diff --git a/orchagent/orch.h b/orchagent/orch.h index 9801f09f7d..ca75fac179 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -225,6 +225,9 @@ class Orch static void logfileReopen(); std::string dumpTuple(Consumer &consumer, const swss::KeyOpFieldsValuesTuple &tuple); ref_resolve_status resolveFieldRefValue(type_map&, const std::string&, const std::string&, swss::KeyOpFieldsValuesTuple&, sai_object_id_t&, std::string&); + std::set generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId); + unsigned long generateBitMapFromIdsStr(const std::string &idsStr); + bool isItemIdsMapContinuous(unsigned long idsMap, sai_uint32_t maxId); bool parseIndexRange(const std::string &input, sai_uint32_t &range_low, sai_uint32_t &range_high); bool parseReference(type_map &type_maps, std::string &ref, const std::string &table_name, std::string &object_name); ref_resolve_status resolveFieldRefArray(type_map&, const std::string&, const std::string&, swss::KeyOpFieldsValuesTuple&, std::vector&, std::string&); diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index 44919bfc43..dc278345de 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4084,9 +4084,19 @@ void PortsOrch::initializePortMaximumHeadroom(Port &port) return; } - vector fvVector; port.m_maximum_headroom = attr.value.u32; - fvVector.emplace_back("max_headroom_size", to_string(port.m_maximum_headroom)); +} + +void PortsOrch::initializePortBufferMaximumParameters(Port &port) +{ + vector fvVector; + if (port.m_maximum_headroom > 0) + { + fvVector.emplace_back("max_headroom_size", to_string(port.m_maximum_headroom)); + } + fvVector.emplace_back("max_priority_groups", to_string(port.m_priority_group_ids.size())); + fvVector.emplace_back("max_queues", to_string(port.m_queue_ids.size())); + m_stateBufferMaximumValueTable->set(port.m_alias, fvVector); } @@ -4099,6 +4109,7 @@ bool PortsOrch::initializePort(Port &port) initializePriorityGroups(port); initializeQueues(port); initializePortMaximumHeadroom(port); + initializePortBufferMaximumParameters(port); /* Create host interface */ if (!addHostIntfs(port, port.m_alias, port.m_hif_id)) diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index 851200b47c..bd865fb56c 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -254,6 +254,7 @@ class PortsOrch : public Orch, public Subject bool initializePort(Port &port); void initializePriorityGroups(Port &port); void initializePortMaximumHeadroom(Port &port); + void initializePortBufferMaximumParameters(Port &port); void initializeQueues(Port &port); bool addHostIntfs(Port &port, string alias, sai_object_id_t &host_intfs_id); From 82807dce4d0572a8195c12c7a9f8472fc9ba0a02 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 5 Nov 2021 10:47:45 +0200 Subject: [PATCH 2/8] Add test cases - Remove corresponding flex counter when a buffer pool is removed - Check maximum priority groups and queues of the port Signed-off-by: Stephen Sun --- tests/test_buffer_dynamic.py | 37 ++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index b3ce795ff3..a2d7c06228 100644 --- a/tests/test_buffer_dynamic.py +++ b/tests/test_buffer_dynamic.py @@ -636,3 +636,40 @@ def test_autoNegPort(self, dvs, testlog): # Shutdown interface dvs.runcmd('config interface shutdown Ethernet0') + + def test_removeBufferPool(self, dvs, testlog): + self.setup_db(dvs) + # Initialize additional databases that are used by this test only + self.counter_db = dvs.get_counters_db() + self.flex_db = dvs.get_flex_db() + + # Create a new pool + self.config_db.update_entry('BUFFER_POOL', 'ingress_test_pool', {'size': '0', 'mode': 'static', 'type': 'ingress'}) + + # Whether counterpoll is enabled? Enable it if not. + flex_counter = self.config_db.get_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") + counter_poll_disabled = (not flex_counter or flex_counter["FLEX_COUNTER_STATUS"] != 'enable') + if counter_poll_disabled: + self.config_db.update_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK", {"FLEX_COUNTER_STATUS": "enable"}) + + # Check whether counter poll has been enabled + time.sleep(1) + poolmap = self.counter_db.wait_for_entry("COUNTERS_BUFFER_POOL_NAME_MAP", "") + assert poolmap["ingress_test_pool"] + self.flex_db.wait_for_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(poolmap["ingress_test_pool"])) + + self.config_db.delete_entry('BUFFER_POOL', 'ingress_test_pool') + oid_to_remove = poolmap.pop('ingress_test_pool') + self.counter_db.wait_for_field_match("COUNTERS_BUFFER_POOL_NAME_MAP", "", poolmap) + self.flex_db.wait_for_deleted_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(oid_to_remove)) + + # Clean up: disable counterpoll if it was disabled + if counter_poll_disabled: + self.config_db.delete_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") + + def test_bufferPortMaxParameter(self, dvs, testlog): + self.setup_db(dvs) + + # Check whether port's maximum parameter has been exposed to STATE_DB + fvs = self.state_db.wait_for_entry("BUFFER_MAX_PARAM_TABLE", "Ethernet0") + assert int(fvs["max_queues"]) and int(fvs["max_priority_groups"]) From ffa802a06df6498077e6427b02e0b799e8df2d6f Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Tue, 2 Nov 2021 07:05:08 +0000 Subject: [PATCH 3/8] Optimize the initialize flow Don't call SAI API to remove an item if it doesn't exist in the local cache This means it hasn't been created in SAI yet. In most cases, this is just to notify orchagent "port ready" Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 56 +++++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 18 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 8115f6eef7..d0f32059b1 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -706,6 +706,7 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) string op = kfvOp(tuple); vector tokens; sai_uint32_t range_low, range_high; + bool need_update_sai = true; SWSS_LOG_DEBUG("Processing:%s", key.c_str()); tokens = tokenize(key, delimiter); @@ -743,6 +744,12 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) } else if (op == DEL_COMMAND) { + auto &typemap = (*m_buffer_type_maps[APP_BUFFER_PG_TABLE_NAME]); + if (typemap.find(key) == typemap.end()) + { + SWSS_LOG_INFO("%s doesn't not exist, don't need to notfiy SAI", key.c_str()); + need_update_sai = false; + } sai_buffer_profile = SAI_NULL_OBJECT_ID; SWSS_LOG_NOTICE("Remove buffer queue %s", key.c_str()); removeObject(m_buffer_type_maps, APP_BUFFER_QUEUE_TABLE_NAME, key); @@ -767,7 +774,6 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) } for (size_t ind = range_low; ind <= range_high; ind++) { - sai_object_id_t queue_id; SWSS_LOG_DEBUG("processing queue:%zd", ind); if (port.m_queue_ids.size() <= ind) { @@ -779,16 +785,20 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) SWSS_LOG_WARN("Queue %zd on port %s is locked, will retry", ind, port_name.c_str()); return task_process_status::task_need_retry; } - queue_id = port.m_queue_ids[ind]; - SWSS_LOG_DEBUG("Applying buffer profile:0x%" PRIx64 " to queue index:%zd, queue sai_id:0x%" PRIx64, sai_buffer_profile, ind, queue_id); - sai_status_t sai_status = sai_queue_api->set_queue_attribute(queue_id, &attr); - if (sai_status != SAI_STATUS_SUCCESS) + if (need_update_sai) { - SWSS_LOG_ERROR("Failed to set queue's buffer profile attribute, status:%d", sai_status); - task_process_status handle_status = handleSaiSetStatus(SAI_API_QUEUE, sai_status); - if (handle_status != task_process_status::task_success) + sai_object_id_t queue_id; + queue_id = port.m_queue_ids[ind]; + SWSS_LOG_DEBUG("Applying buffer profile:0x%" PRIx64 " to queue index:%zd, queue sai_id:0x%" PRIx64, sai_buffer_profile, ind, queue_id); + sai_status_t sai_status = sai_queue_api->set_queue_attribute(queue_id, &attr); + if (sai_status != SAI_STATUS_SUCCESS) { - return handle_status; + SWSS_LOG_ERROR("Failed to set queue's buffer profile attribute, status:%d", sai_status); + task_process_status handle_status = handleSaiSetStatus(SAI_API_QUEUE, sai_status); + if (handle_status != task_process_status::task_success) + { + return handle_status; + } } } } @@ -830,6 +840,7 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup string op = kfvOp(tuple); vector tokens; sai_uint32_t range_low, range_high; + bool need_update_sai = true; SWSS_LOG_DEBUG("processing:%s", key.c_str()); tokens = tokenize(key, delimiter); @@ -868,6 +879,12 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup } else if (op == DEL_COMMAND) { + auto &typemap = (*m_buffer_type_maps[APP_BUFFER_PG_TABLE_NAME]); + if (typemap.find(key) == typemap.end()) + { + SWSS_LOG_INFO("%s doesn't not exist, don't need to notfiy SAI", key.c_str()); + need_update_sai = false; + } sai_buffer_profile = SAI_NULL_OBJECT_ID; SWSS_LOG_NOTICE("Remove buffer PG %s", key.c_str()); removeObject(m_buffer_type_maps, APP_BUFFER_PG_TABLE_NAME, key); @@ -893,7 +910,6 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup } for (size_t ind = range_low; ind <= range_high; ind++) { - sai_object_id_t pg_id; SWSS_LOG_DEBUG("processing pg:%zd", ind); if (port.m_priority_group_ids.size() <= ind) { @@ -908,16 +924,20 @@ task_process_status BufferOrch::processPriorityGroup(KeyOpFieldsValuesTuple &tup } else { - pg_id = port.m_priority_group_ids[ind]; - SWSS_LOG_DEBUG("Applying buffer profile:0x%" PRIx64 " to port:%s pg index:%zd, pg sai_id:0x%" PRIx64, sai_buffer_profile, port_name.c_str(), ind, pg_id); - sai_status_t sai_status = sai_buffer_api->set_ingress_priority_group_attribute(pg_id, &attr); - if (sai_status != SAI_STATUS_SUCCESS) + if (need_update_sai) { - SWSS_LOG_ERROR("Failed to set port:%s pg:%zd buffer profile attribute, status:%d", port_name.c_str(), ind, sai_status); - task_process_status handle_status = handleSaiSetStatus(SAI_API_BUFFER, sai_status); - if (handle_status != task_process_status::task_success) + sai_object_id_t pg_id; + pg_id = port.m_priority_group_ids[ind]; + SWSS_LOG_DEBUG("Applying buffer profile:0x%" PRIx64 " to port:%s pg index:%zd, pg sai_id:0x%" PRIx64, sai_buffer_profile, port_name.c_str(), ind, pg_id); + sai_status_t sai_status = sai_buffer_api->set_ingress_priority_group_attribute(pg_id, &attr); + if (sai_status != SAI_STATUS_SUCCESS) { - return handle_status; + SWSS_LOG_ERROR("Failed to set port:%s pg:%zd buffer profile attribute, status:%d", port_name.c_str(), ind, sai_status); + task_process_status handle_status = handleSaiSetStatus(SAI_API_BUFFER, sai_status); + if (handle_status != task_process_status::task_success) + { + return handle_status; + } } } } From 8df51bbcc0a3f3debc6a5aa3936000c389cffdd3 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Fri, 5 Nov 2021 15:39:50 +0000 Subject: [PATCH 4/8] Fix a typo Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index d0f32059b1..d8d27b2112 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -744,7 +744,7 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) } else if (op == DEL_COMMAND) { - auto &typemap = (*m_buffer_type_maps[APP_BUFFER_PG_TABLE_NAME]); + auto &typemap = (*m_buffer_type_maps[APP_BUFFER_QUEUE_TABLE_NAME]); if (typemap.find(key) == typemap.end()) { SWSS_LOG_INFO("%s doesn't not exist, don't need to notfiy SAI", key.c_str()); From 84ed8cc278beb126194b29a9a0696c45c64b50d8 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 17 Nov 2021 03:17:29 +0000 Subject: [PATCH 5/8] Fix review comments Signed-off-by: Stephen Sun --- orchagent/orch.cpp | 2 +- orchagent/portsorch.cpp | 16 ++++-------- orchagent/portsorch.h | 1 - tests/test_buffer_dynamic.py | 47 ++++++++++++++++++------------------ 4 files changed, 30 insertions(+), 36 deletions(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index c606801d04..62193e8828 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -647,7 +647,7 @@ bool Orch::parseIndexRange(const string &input, sai_uint32_t &range_low, sai_uin * * Example: * Input idsMap: 3-4 - * Return: 00001100b + * Return: 00011000b */ unsigned long Orch::generateBitMapFromIdsStr(const string &idsStr) { diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp index dc278345de..539ccab1ed 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4071,9 +4071,10 @@ void PortsOrch::initializePriorityGroups(Port &port) SWSS_LOG_INFO("Get priority groups for port %s", port.m_alias.c_str()); } -void PortsOrch::initializePortMaximumHeadroom(Port &port) +void PortsOrch::initializePortBufferMaximumParameters(Port &port) { sai_attribute_t attr; + vector fvVector; attr.id = SAI_PORT_ATTR_QOS_MAXIMUM_HEADROOM_SIZE; @@ -4081,19 +4082,13 @@ void PortsOrch::initializePortMaximumHeadroom(Port &port) if (status != SAI_STATUS_SUCCESS) { SWSS_LOG_NOTICE("Unable to get the maximum headroom for port %s rv:%d, ignored", port.m_alias.c_str(), status); - return; } - - port.m_maximum_headroom = attr.value.u32; -} - -void PortsOrch::initializePortBufferMaximumParameters(Port &port) -{ - vector fvVector; - if (port.m_maximum_headroom > 0) + else { + port.m_maximum_headroom = attr.value.u32; fvVector.emplace_back("max_headroom_size", to_string(port.m_maximum_headroom)); } + fvVector.emplace_back("max_priority_groups", to_string(port.m_priority_group_ids.size())); fvVector.emplace_back("max_queues", to_string(port.m_queue_ids.size())); @@ -4108,7 +4103,6 @@ bool PortsOrch::initializePort(Port &port) initializePriorityGroups(port); initializeQueues(port); - initializePortMaximumHeadroom(port); initializePortBufferMaximumParameters(port); /* Create host interface */ diff --git a/orchagent/portsorch.h b/orchagent/portsorch.h index bd865fb56c..3468128cc7 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -253,7 +253,6 @@ class PortsOrch : public Orch, public Subject bool initializePort(Port &port); void initializePriorityGroups(Port &port); - void initializePortMaximumHeadroom(Port &port); void initializePortBufferMaximumParameters(Port &port); void initializeQueues(Port &port); diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index a2d7c06228..329279ab44 100644 --- a/tests/test_buffer_dynamic.py +++ b/tests/test_buffer_dynamic.py @@ -643,29 +643,30 @@ def test_removeBufferPool(self, dvs, testlog): self.counter_db = dvs.get_counters_db() self.flex_db = dvs.get_flex_db() - # Create a new pool - self.config_db.update_entry('BUFFER_POOL', 'ingress_test_pool', {'size': '0', 'mode': 'static', 'type': 'ingress'}) - - # Whether counterpoll is enabled? Enable it if not. - flex_counter = self.config_db.get_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") - counter_poll_disabled = (not flex_counter or flex_counter["FLEX_COUNTER_STATUS"] != 'enable') - if counter_poll_disabled: - self.config_db.update_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK", {"FLEX_COUNTER_STATUS": "enable"}) - - # Check whether counter poll has been enabled - time.sleep(1) - poolmap = self.counter_db.wait_for_entry("COUNTERS_BUFFER_POOL_NAME_MAP", "") - assert poolmap["ingress_test_pool"] - self.flex_db.wait_for_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(poolmap["ingress_test_pool"])) - - self.config_db.delete_entry('BUFFER_POOL', 'ingress_test_pool') - oid_to_remove = poolmap.pop('ingress_test_pool') - self.counter_db.wait_for_field_match("COUNTERS_BUFFER_POOL_NAME_MAP", "", poolmap) - self.flex_db.wait_for_deleted_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(oid_to_remove)) - - # Clean up: disable counterpoll if it was disabled - if counter_poll_disabled: - self.config_db.delete_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") + try: + # Create a new pool + self.config_db.update_entry('BUFFER_POOL', 'ingress_test_pool', {'size': '0', 'mode': 'static', 'type': 'ingress'}) + + # Whether counterpoll is enabled? Enable it if not. + flex_counter = self.config_db.get_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") + counter_poll_disabled = (not flex_counter or flex_counter["FLEX_COUNTER_STATUS"] != 'enable') + if counter_poll_disabled: + self.config_db.update_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK", {"FLEX_COUNTER_STATUS": "enable"}) + + # Check whether counter poll has been enabled + time.sleep(1) + poolmap = self.counter_db.wait_for_entry("COUNTERS_BUFFER_POOL_NAME_MAP", "") + assert poolmap["ingress_test_pool"] + self.flex_db.wait_for_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(poolmap["ingress_test_pool"])) + + self.config_db.delete_entry('BUFFER_POOL', 'ingress_test_pool') + oid_to_remove = poolmap.pop('ingress_test_pool') + self.counter_db.wait_for_field_match("COUNTERS_BUFFER_POOL_NAME_MAP", "", poolmap) + self.flex_db.wait_for_deleted_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK_STAT_COUNTER:{}".format(oid_to_remove)) + finally: + # Clean up: disable counterpoll if it was disabled + if counter_poll_disabled: + self.config_db.delete_entry("FLEX_COUNTER_TABLE", "BUFFER_POOL_WATERMARK") def test_bufferPortMaxParameter(self, dvs, testlog): self.setup_db(dvs) From c3a2527e4c83c9799b4735f46b775aac031f015e Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 17 Nov 2021 03:17:29 +0000 Subject: [PATCH 6/8] Fix typo clearBufferPoolWatermaskCounterCounterIdList => clearBufferPoolWatermarkCounterIdList Remove "reclaim" from a generic function in orch.cpp Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 4 ++-- orchagent/bufferorch.h | 2 +- orchagent/orch.cpp | 8 ++++---- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index d8d27b2112..0aa593846a 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -210,7 +210,7 @@ bool BufferOrch::isPortReady(const std::string& port_name) const return result; } -void BufferOrch::clearBufferPoolWatermarkCounterCounterIdList(const sai_object_id_t object_id) +void BufferOrch::clearBufferPoolWatermarkCounterIdList(const sai_object_id_t object_id) { string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(object_id); m_flexCounterTable->del(key); @@ -466,7 +466,7 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple) if (SAI_NULL_OBJECT_ID != sai_object) { - clearBufferPoolWatermarkCounterCounterIdList(sai_object); + clearBufferPoolWatermarkCounterIdList(sai_object); sai_status = sai_buffer_api->remove_buffer_pool(sai_object); if (SAI_STATUS_SUCCESS != sai_status) { diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index b70fc9638f..05fdd7917f 100644 --- a/orchagent/bufferorch.h +++ b/orchagent/bufferorch.h @@ -45,7 +45,7 @@ class BufferOrch : public Orch void doTask() override; virtual void doTask(Consumer& consumer); - void clearBufferPoolWatermarkCounterCounterIdList(const sai_object_id_t object_id); + void clearBufferPoolWatermarkCounterIdList(const sai_object_id_t object_id); void initTableHandlers(); void initBufferReadyLists(DBConnector *confDb, DBConnector *applDb); void initBufferReadyList(Table& table, bool isConfigDb); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 62193e8828..8c2b1d8588 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -684,7 +684,7 @@ set Orch::generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId unsigned long currentIdMask = 1; bool started = false, needGenerateMap = false; sai_uint32_t lower, upper; - set extraIdsToReclaim; + set idStringList; for (sai_uint32_t id = 0; id <= maxId; id ++) { // currentIdMask represents the bit mask corresponding to id: (1< Orch::generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId { if (lower != upper) { - extraIdsToReclaim.insert(to_string(lower) + "-" + to_string(upper)); + idStringList.insert(to_string(lower) + "-" + to_string(upper)); } else { - extraIdsToReclaim.insert(to_string(lower)); + idStringList.insert(to_string(lower)); } needGenerateMap = false; } @@ -722,7 +722,7 @@ set Orch::generateIdListFromMap(unsigned long idsMap, sai_uint32_t maxId currentIdMask <<= 1; } - return extraIdsToReclaim; + return idStringList; } From 7b2546cb7ed057d5ac9acbe2b6e8f344e12db706 Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 17 Nov 2021 06:58:05 +0000 Subject: [PATCH 7/8] Make the comment more clear Signed-off-by: Stephen Sun --- orchagent/orch.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 8c2b1d8588..5a27e9161a 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -671,7 +671,8 @@ unsigned long Orch::generateBitMapFromIdsStr(const string &idsStr) * Parse the idsMap and generate a vector which contains slices representing bits in idsMap * Args: * idsMap: The bitmap of IDs. The LSB stands for ID 0. - * maxId: The maximum value of ID. + * maxId: The (exclusive) upperbound of ID range. + * Eg: if ID range is 0~7, maxId should be 8. * Return: * A vector which contains slices representing bits in idsMap * From 814f6594f98f6b2e70da6acb50eccec3ad4077fb Mon Sep 17 00:00:00 2001 From: Stephen Sun Date: Wed, 17 Nov 2021 23:57:22 +0000 Subject: [PATCH 8/8] Check whether flex counter has been configured before clear counter on removing a pool Signed-off-by: Stephen Sun --- orchagent/bufferorch.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 0aa593846a..6f60d15c3e 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -212,8 +212,11 @@ bool BufferOrch::isPortReady(const std::string& port_name) const void BufferOrch::clearBufferPoolWatermarkCounterIdList(const sai_object_id_t object_id) { - string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(object_id); - m_flexCounterTable->del(key); + if (m_isBufferPoolWatermarkCounterIdListGenerated) + { + string key = BUFFER_POOL_WATERMARK_STAT_COUNTER_FLEX_COUNTER_GROUP ":" + sai_serialize_object_id(object_id); + m_flexCounterTable->del(key); + } } void BufferOrch::generateBufferPoolWatermarkCounterIdList(void)