From 32d7a69e3f54b440b4636b0eeb5de3baeaa53acd Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Mon, 22 Nov 2021 17:45:24 +0800 Subject: [PATCH] [Reclaiming buffer] Common code update (#1996) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - What I did Common code update for reclaiming buffer. 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 5. Change the log severity from ERROR to NOTICE when parsing buffer profile from buffer profile list failed. Typically this can be resolved by retrying. The severity of similar log when parsing buffer PG and queue is already NOTICE. - Why I did it To split large PR into smaller ones and help pass CI. - How I verified it vs test and sonic-mgmt test. Signed-off-by: Stephen Sun --- cfgmgr/buffermgrd.cpp | 37 ++++++---- cfgmgr/buffermgrdyn.cpp | 2 +- cfgmgr/buffermgrdyn.h | 2 +- orchagent/bufferorch.cpp | 66 ++++++++++++----- orchagent/bufferorch.h | 1 + orchagent/orch.cpp | 135 ++++++++++++++++++++++++++++++++++- orchagent/orch.h | 3 + orchagent/portsorch.cpp | 17 +++-- orchagent/portsorch.h | 2 +- tests/test_buffer_dynamic.py | 38 ++++++++++ 10 files changed, 262 insertions(+), 41 deletions(-) diff --git a/cfgmgr/buffermgrd.cpp b/cfgmgr/buffermgrd.cpp index 9f4d97923752..74e88f733750 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 f1bbcd395abf..a1c64b7e5eb5 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 68a614bbf842..fa1c69dbc2c7 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 fce79fca3b08..e7204344d5a3 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -210,6 +210,15 @@ bool BufferOrch::isPortReady(const std::string& port_name) const return result; } +void BufferOrch::clearBufferPoolWatermarkCounterIdList(const sai_object_id_t object_id) +{ + 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) { // This function will be called in FlexCounterOrch when field:value tuple "FLEX_COUNTER_STATUS":"enable" @@ -460,6 +469,7 @@ task_process_status BufferOrch::processBufferPool(KeyOpFieldsValuesTuple &tuple) if (SAI_NULL_OBJECT_ID != sai_object) { + clearBufferPoolWatermarkCounterIdList(sai_object); sai_status = sai_buffer_api->remove_buffer_pool(sai_object); if (SAI_STATUS_SUCCESS != sai_status) { @@ -699,6 +709,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); @@ -736,6 +747,12 @@ task_process_status BufferOrch::processQueue(KeyOpFieldsValuesTuple &tuple) } else if (op == DEL_COMMAND) { + 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()); + 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); @@ -760,7 +777,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) { @@ -772,16 +788,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; + } } } } @@ -823,6 +843,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); @@ -861,6 +882,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); @@ -886,7 +913,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) { @@ -901,16 +927,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; + } } } } diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index ef4153e6a467..05fdd7917fdc 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 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 226512f388e4..5a27e9161a7b 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,139 @@ 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: 00011000b + */ +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 (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 + * + * 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 idStringList; + 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 9801f09f7d0b..ca75fac17993 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 f222c62be491..db592024a649 100755 --- a/orchagent/portsorch.cpp +++ b/orchagent/portsorch.cpp @@ -4064,9 +4064,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; @@ -4074,12 +4075,16 @@ 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; + } + else + { + port.m_maximum_headroom = attr.value.u32; + fvVector.emplace_back("max_headroom_size", to_string(port.m_maximum_headroom)); } - vector fvVector; - 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())); + m_stateBufferMaximumValueTable->set(port.m_alias, fvVector); } @@ -4091,7 +4096,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 748f2ea844e4..c4e192aa1429 100755 --- a/orchagent/portsorch.h +++ b/orchagent/portsorch.h @@ -260,7 +260,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); diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index b3ce795ff3c7..329279ab4472 100644 --- a/tests/test_buffer_dynamic.py +++ b/tests/test_buffer_dynamic.py @@ -636,3 +636,41 @@ 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() + + 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) + + # 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"])