From 4c8e2b53e982da45342037cb679ba3fe0b7ff226 Mon Sep 17 00:00:00 2001 From: Stephen Sun <5379172+stephenxs@users.noreply.github.com> Date: Tue, 29 Jun 2021 01:35:01 +0800 Subject: [PATCH] [Dynamic Buffer Calc] Avoid creating lossy PG for admin down ports during initialization (#1776) - What I did Avoid creating lossy PGs for admin down ports during system initialization Make ADMIN_DOWN the default state of a port in dynamic buffer manager. Explicitly remove configured PGs from an admin down port during initialization This is to make sure the items will be added to m_ready_list. Use APPL_DB as the source of initial PGs and queues for port ready deducting for warm reboot Signed-off-by: Stephen Sun stephens@nvidia.com - Why I did it Lossy PGs for admin down ports should not be applied to APPL_DB. Originally, we had the logic to remove lossy/lossless PGs when shutting a port. The same logic should be applied when the system is starting. Allocating lossy PG for an admin down port doesn't affect the system negatively except for wasting buffer. So we would like to resolve it. - How I verified it Manually test --- cfgmgr/buffermgrdyn.cpp | 22 ++++++++++++-- cfgmgr/buffermgrdyn.h | 6 ++-- orchagent/bufferorch.cpp | 56 ++++++++++++++++++++++++++++-------- orchagent/bufferorch.h | 4 +-- tests/test_buffer_dynamic.py | 14 ++++++++- 5 files changed, 81 insertions(+), 21 deletions(-) diff --git a/cfgmgr/buffermgrdyn.cpp b/cfgmgr/buffermgrdyn.cpp index 33ef6c3e5e95..4dd69840992d 100644 --- a/cfgmgr/buffermgrdyn.cpp +++ b/cfgmgr/buffermgrdyn.cpp @@ -1052,6 +1052,12 @@ task_process_status BufferMgrDynamic::doUpdatePgTask(const string &pg_key, const case PORT_ADMIN_DOWN: SWSS_LOG_NOTICE("Skip setting BUFFER_PG for %s because the port is administratively down", port.c_str()); + if (!m_portInitDone) + { + // In case the port is admin down during initialization, the PG will be removed from the port, + // which effectively notifies bufferOrch to add the item to the m_ready_list + m_applBufferPgTable.del(pg_key); + } break; default: @@ -1862,9 +1868,19 @@ task_process_status BufferMgrDynamic::handleOneBufferPgEntry(const string &key, } else { - SWSS_LOG_NOTICE("Inserting BUFFER_PG table entry %s into APPL_DB directly", key.c_str()); - m_applBufferPgTable.set(key, fvVector); - bufferPg.running_profile_name = bufferPg.configured_profile_name; + port_info_t &portInfo = m_portInfoLookup[port]; + if (PORT_ADMIN_DOWN != portInfo.state) + { + SWSS_LOG_NOTICE("Inserting BUFFER_PG table entry %s into APPL_DB directly", key.c_str()); + m_applBufferPgTable.set(key, fvVector); + bufferPg.running_profile_name = bufferPg.configured_profile_name; + } + else if (!m_portInitDone) + { + // In case the port is admin down during initialization, the PG will be removed from the port, + // which effectively notifies bufferOrch to add the item to the m_ready_list + m_applBufferPgTable.del(key); + } } if (!bufferPg.configured_profile_name.empty()) diff --git a/cfgmgr/buffermgrdyn.h b/cfgmgr/buffermgrdyn.h index 1a6830590992..e8e031315bbb 100644 --- a/cfgmgr/buffermgrdyn.h +++ b/cfgmgr/buffermgrdyn.h @@ -79,12 +79,12 @@ typedef struct { } buffer_pg_t; typedef enum { + // Port is admin down. All PGs programmed to APPL_DB should be removed from the port + PORT_ADMIN_DOWN, // Port is under initializing, which means its info hasn't been comprehensive for calculating headroom PORT_INITIALIZING, // All necessary information for calculating headroom is ready - PORT_READY, - // Port is admin down. All PGs programmed to APPL_DB should be removed from the port - PORT_ADMIN_DOWN + PORT_READY } port_state_t; typedef struct { diff --git a/orchagent/bufferorch.cpp b/orchagent/bufferorch.cpp index 5979445731d8..59fcce590ee2 100644 --- a/orchagent/bufferorch.cpp +++ b/orchagent/bufferorch.cpp @@ -2,6 +2,7 @@ #include "bufferorch.h" #include "logger.h" #include "sai_serialize.h" +#include "warm_restart.h" #include #include @@ -45,7 +46,7 @@ BufferOrch::BufferOrch(DBConnector *applDb, DBConnector *confDb, DBConnector *st { SWSS_LOG_ENTER(); initTableHandlers(); - initBufferReadyLists(confDb); + initBufferReadyLists(applDb, confDb); initFlexCounterGroupTable(); initBufferConstants(); }; @@ -61,31 +62,62 @@ void BufferOrch::initTableHandlers() m_bufferHandlerMap.insert(buffer_handler_pair(APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME, &BufferOrch::processEgressBufferProfileList)); } -void BufferOrch::initBufferReadyLists(DBConnector *db) +void BufferOrch::initBufferReadyLists(DBConnector *applDb, DBConnector *confDb) { - // The motivation of m_ready_list is to get the preconfigured buffer pg and buffer queue items - // from the database when system starts. - // When a buffer pg or queue item is updated, if the item isn't in the m_ready_list + /* + Map m_ready_list and m_port_ready_list_ref are designed to track whether the ports are "ready" from buffer's POV + by testing whether all configured buffer PGs and queues have been applied to SAI. The idea is: + - bufferorch read initial configuration and put them into m_port_ready_list_ref. + - A buffer pg or queue item will be put into m_ready_list once it is applied to SAI. + The rest of port initialization won't be started before the port being ready. + + However, the items won't be applied to admin down ports in dynamic buffer model, which means the admin down ports won't be "ready" + The solution is: + - buffermgr to notify bufferorch explicitly to remove the PG and queue items configured on admin down ports + - bufferorch to add the items to m_ready_list on receiving notifications, which is an existing logic + + Theoretically, the initial configuration should come from CONFIG_DB but APPL_DB is used for warm reboot, because: + - For cold/fast start, buffermgr is responsible for injecting items to APPL_DB + There is no guarantee that items in APPL_DB are ready when orchagent starts + - For warm reboot, APPL_DB is restored from the previous boot, which means they are ready when orchagent starts + In addition, bufferorch won't be notified removal of items on admin down ports during warm reboot + because buffermgrd hasn't been started yet. + Using APPL_DB means items of admin down ports won't be inserted into m_port_ready_list_ref + and guarantees the admin down ports always be ready in dynamic buffer model + */ SWSS_LOG_ENTER(); - Table pg_table(db, CFG_BUFFER_PG_TABLE_NAME); - initBufferReadyList(pg_table); + if (WarmStart::isWarmStart()) + { + Table pg_table(applDb, APP_BUFFER_PG_TABLE_NAME); + initBufferReadyList(pg_table, false); - Table queue_table(db, CFG_BUFFER_QUEUE_TABLE_NAME); - initBufferReadyList(queue_table); + Table queue_table(applDb, APP_BUFFER_QUEUE_TABLE_NAME); + initBufferReadyList(queue_table, false); + } + else + { + Table pg_table(confDb, CFG_BUFFER_PG_TABLE_NAME); + initBufferReadyList(pg_table, true); + + Table queue_table(confDb, CFG_BUFFER_QUEUE_TABLE_NAME); + initBufferReadyList(queue_table, true); + } } -void BufferOrch::initBufferReadyList(Table& table) +void BufferOrch::initBufferReadyList(Table& table, bool isConfigDb) { SWSS_LOG_ENTER(); std::vector keys; table.getKeys(keys); + const char dbKeyDelimiter = (isConfigDb ? config_db_key_delimiter : delimiter); + // populate the lists with buffer configuration information for (const auto& key: keys) { - auto tokens = tokenize(key, config_db_key_delimiter); + auto &&tokens = tokenize(key, dbKeyDelimiter); if (tokens.size() != 2) { SWSS_LOG_ERROR("Wrong format of a table '%s' key '%s'. Skip it", table.getTableName().c_str(), key.c_str()); @@ -96,7 +128,7 @@ void BufferOrch::initBufferReadyList(Table& table) auto appldb_key = tokens[0] + delimiter + tokens[1]; m_ready_list[appldb_key] = false; - auto port_names = tokenize(tokens[0], list_item_delimiter); + auto &&port_names = tokenize(tokens[0], list_item_delimiter); for(const auto& port_name: port_names) { diff --git a/orchagent/bufferorch.h b/orchagent/bufferorch.h index dc3752181368..ef4153e6a467 100644 --- a/orchagent/bufferorch.h +++ b/orchagent/bufferorch.h @@ -46,8 +46,8 @@ class BufferOrch : public Orch void doTask() override; virtual void doTask(Consumer& consumer); void initTableHandlers(); - void initBufferReadyLists(DBConnector *db); - void initBufferReadyList(Table& table); + void initBufferReadyLists(DBConnector *confDb, DBConnector *applDb); + void initBufferReadyList(Table& table, bool isConfigDb); void initFlexCounterGroupTable(void); void initBufferConstants(); task_process_status processBufferPool(KeyOpFieldsValuesTuple &tuple); diff --git a/tests/test_buffer_dynamic.py b/tests/test_buffer_dynamic.py index 0d831f703548..74575c6ee31b 100644 --- a/tests/test_buffer_dynamic.py +++ b/tests/test_buffer_dynamic.py @@ -548,6 +548,9 @@ def test_sharedHeadroomPool(self, dvs, testlog): def test_shutdownPort(self, dvs, testlog): self.setup_db(dvs) + lossy_pg_reference_config_db = '[BUFFER_PROFILE|ingress_lossy_profile]' + lossy_pg_reference_appl_db = '[BUFFER_PROFILE_TABLE:ingress_lossy_profile]' + # Startup interface dvs.runcmd('config interface startup Ethernet0') @@ -558,14 +561,23 @@ def test_shutdownPort(self, dvs, testlog): # Shutdown port and check whether all the PGs have been removed dvs.runcmd("config interface shutdown Ethernet0") + self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:0") self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:3-4") self.app_db.wait_for_deleted_entry("BUFFER_PROFILE", expectedProfile) - # Add another PG when port is administratively down + # Add extra lossy and lossless PGs when a port is administratively down + self.config_db.update_entry('BUFFER_PG', 'Ethernet0|1', {'profile': lossy_pg_reference_config_db}) self.config_db.update_entry('BUFFER_PG', 'Ethernet0|6', {'profile': 'NULL'}) + # Make sure they have not been added to APPL_DB + time.sleep(30) + self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:1") + self.app_db.wait_for_deleted_entry("BUFFER_PG_TABLE", "Ethernet0:6") + # Startup port and check whether all the PGs haved been added dvs.runcmd("config interface startup Ethernet0") + self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:0", {"profile": lossy_pg_reference_appl_db}) + self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:1", {"profile": lossy_pg_reference_appl_db}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:3-4", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"}) self.app_db.wait_for_field_match("BUFFER_PG_TABLE", "Ethernet0:6", {"profile": "[BUFFER_PROFILE_TABLE:" + expectedProfile + "]"})