Skip to content

Commit

Permalink
[Dynamic Buffer Calc] Avoid creating lossy PG for admin down ports du…
Browse files Browse the repository at this point in the history
…ring initialization (sonic-net#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 [email protected]

- 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
  • Loading branch information
stephenxs authored Jun 28, 2021
1 parent 3602124 commit 4c8e2b5
Show file tree
Hide file tree
Showing 5 changed files with 81 additions and 21 deletions.
22 changes: 19 additions & 3 deletions cfgmgr/buffermgrdyn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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())
Expand Down
6 changes: 3 additions & 3 deletions cfgmgr/buffermgrdyn.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
56 changes: 44 additions & 12 deletions orchagent/bufferorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#include "bufferorch.h"
#include "logger.h"
#include "sai_serialize.h"
#include "warm_restart.h"

#include <inttypes.h>
#include <sstream>
Expand Down Expand Up @@ -45,7 +46,7 @@ BufferOrch::BufferOrch(DBConnector *applDb, DBConnector *confDb, DBConnector *st
{
SWSS_LOG_ENTER();
initTableHandlers();
initBufferReadyLists(confDb);
initBufferReadyLists(applDb, confDb);
initFlexCounterGroupTable();
initBufferConstants();
};
Expand All @@ -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<std::string> 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());
Expand All @@ -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)
{
Expand Down
4 changes: 2 additions & 2 deletions orchagent/bufferorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
14 changes: 13 additions & 1 deletion tests/test_buffer_dynamic.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')

Expand All @@ -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 + "]"})

Expand Down

0 comments on commit 4c8e2b5

Please sign in to comment.