Skip to content

Commit

Permalink
Td2: Reclaim buffer from unused ports (sonic-net#1830)
Browse files Browse the repository at this point in the history
Signed-off-by: Neetha John <nejo@microsoft.com>

By default, lossless profiles are attached to PGs 3, 4 of all ports which results in a lot of buffer wastage when most of those ports are unused (not associated with a DEVICE_NEIGHBOR). In TD2, ingress pool size comprises of the reserved space as well. Hence making use of a special cable length of '0m' to identify unused ports and skip reserving space for those ports

What I did
* In buffermgr, if port with cable len '0m' is identified, return immediately without creating pg lossless profile or attaching profile to the lossless pgs of that port
* Listen to 'admin_status' update as well from the PORT table and update port-speed mapping. This is to handle add-rack scenario where a port is added later
   - The port starts of with cable length 0m
   - Configlet to add a port is applied. The order of operations specifc to the PORT/CABLE_LENGTH table are - port is initially set to admin down, cable length is updated for that port, port table attributes are defined and port is set to admin up
   - speed update might not be seen when the port is set to admin up. Hence port-speed mapping will capture the speed update whenever its seen and once the cable length is updated while the port is brought back up, profiles can be attached to the lossless pgs of the port

How I verified it
- Manual tests done. Verified that no space is reserved for unused ports
- Verified that when a port is added using 'add-rack' scenario, profile is attached to pgs of that port
- New VS test added
  • Loading branch information
neethajohn authored Jul 26, 2021
1 parent 0fe2dfe commit 7f80f06
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 6 deletions.
25 changes: 20 additions & 5 deletions cfgmgr/buffermgr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ void BufferMgr::readPgProfileLookupFile(string file)
task_process_status BufferMgr::doCableTask(string port, string cable_length)
{
m_cableLenLookup[port] = cable_length;
SWSS_LOG_INFO("Cable length set to %s for port %s", m_cableLenLookup[port].c_str(), port.c_str());
return task_process_status::task_success;
}

Expand Down Expand Up @@ -120,10 +121,11 @@ Create/update two tables: profile (in m_cfgBufferProfileTable) and port buffer (
}
}
*/
task_process_status BufferMgr::doSpeedUpdateTask(string port, string speed)
task_process_status BufferMgr::doSpeedUpdateTask(string port)
{
vector<FieldValueTuple> fvVector;
string cable;
string speed;

if (m_cableLenLookup.count(port) == 0)
{
Expand All @@ -132,7 +134,13 @@ task_process_status BufferMgr::doSpeedUpdateTask(string port, string speed)
}

cable = m_cableLenLookup[port];
if (cable == "0m")
{
SWSS_LOG_NOTICE("Not creating/updating PG profile for port %s. Cable length is set to %s", port.c_str(), cable.c_str());
return task_process_status::task_success;
}

speed = m_speedLookup[port];
if (m_pgProfileLookup.count(speed) == 0 || m_pgProfileLookup[speed].count(cable) == 0)
{
SWSS_LOG_ERROR("Unable to create/update PG profile for port %s. No PG profile configured for speed %s and cable length %s",
Expand Down Expand Up @@ -368,11 +376,18 @@ void BufferMgr::doTask(Consumer &consumer)
// receive and cache cable length table
task_status = doCableTask(fvField(i), fvValue(i));
}
// In case of PORT table update, Buffer Manager is interested in speed update only
if (m_pgfile_processed && table_name == CFG_PORT_TABLE_NAME && fvField(i) == "speed")
if (m_pgfile_processed && table_name == CFG_PORT_TABLE_NAME && (fvField(i) == "speed" || fvField(i) == "admin_status"))
{
// create/update profile for port
task_status = doSpeedUpdateTask(port, fvValue(i));
if (fvField(i) == "speed")
{
m_speedLookup[port] = fvValue(i);
}

if (m_speedLookup.count(port) != 0)
{
// create/update profile for port
task_status = doSpeedUpdateTask(port);
}
}
if (task_status != task_process_status::task_success)
{
Expand Down
4 changes: 3 additions & 1 deletion cfgmgr/buffermgr.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ typedef std::map<std::string, pg_profile_t> speed_map_t;
typedef std::map<std::string, speed_map_t> pg_profile_lookup_t;

typedef std::map<std::string, std::string> port_cable_length_t;
typedef std::map<std::string, std::string> port_speed_t;

class BufferMgr : public Orch
{
Expand All @@ -52,10 +53,11 @@ class BufferMgr : public Orch

pg_profile_lookup_t m_pgProfileLookup;
port_cable_length_t m_cableLenLookup;
port_speed_t m_speedLookup;
std::string getPgPoolMode();
void readPgProfileLookupFile(std::string);
task_process_status doCableTask(std::string port, std::string cable_length);
task_process_status doSpeedUpdateTask(std::string port, std::string speed);
task_process_status doSpeedUpdateTask(std::string port);
void doBufferTableTask(Consumer &consumer, ProducerStateTable &applTable);

void transformSeperator(std::string &name);
Expand Down
145 changes: 145 additions & 0 deletions tests/test_buffer_traditional.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,145 @@
import pytest
import time


class TestBuffer(object):
LOSSLESS_PGS = [3, 4]
INTF = "Ethernet0"

def setup_db(self, dvs):
self.app_db = dvs.get_app_db()
self.asic_db = dvs.get_asic_db()
self.config_db = dvs.get_config_db()
self.counter_db = dvs.get_counters_db()

# enable PG watermark
self.set_pg_wm_status('enable')

def get_pg_oid(self, pg):
fvs = dict()
fvs = self.counter_db.get_entry("COUNTERS_PG_NAME_MAP", "")
return fvs[pg]

def set_pg_wm_status(self, state):
fvs = {'FLEX_COUNTER_STATUS': state}
self.config_db.update_entry("FLEX_COUNTER_TABLE", "PG_WATERMARK", fvs)
time.sleep(1)

def teardown(self):
# disable PG watermark
self.set_pg_wm_status('disable')

def get_asic_buf_profile(self):
return set(self.asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_PROFILE"))

def check_new_profile_in_asic_db(self, profile):
retry_count = 0
self.new_profile = None
while retry_count < 5:
retry_count += 1
diff = set(self.asic_db.get_keys("ASIC_STATE:SAI_OBJECT_TYPE_BUFFER_PROFILE")) - self.orig_profiles
if len(diff) == 1:
self.new_profile = diff.pop()
break
else:
time.sleep(1)
assert self.new_profile, "Can't get SAI OID for newly created profile {} after retry {} times".format(profile, retry_count)

def get_asic_buf_pg_profiles(self):
self.buf_pg_profile = dict()
for pg in self.pg_name_map:
buf_pg_entries = self.asic_db.get_entry("ASIC_STATE:SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", self.pg_name_map[pg])
self.buf_pg_profile[pg] = buf_pg_entries["SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE"]

def change_cable_len(self, cable_len):
fvs = dict()
fvs[self.INTF] = cable_len
self.config_db.update_entry("CABLE_LENGTH", "AZURE", fvs)

@pytest.fixture
def setup_teardown_test(self, dvs):
try:
self.setup_db(dvs)
pg_name_map = dict()
for pg in self.LOSSLESS_PGS:
pg_name = "{}:{}".format(self.INTF, pg)
pg_name_map[pg_name] = self.get_pg_oid(pg_name)
yield pg_name_map
finally:
self.teardown()

def test_zero_cable_len_profile_update(self, dvs, setup_teardown_test):
self.pg_name_map = setup_teardown_test
orig_cable_len = None
orig_speed = None
try:
dvs.runcmd("config interface startup {}".format(self.INTF))
self.orig_profiles = self.get_asic_buf_profile()
# get orig cable length and speed
fvs = self.config_db.get_entry("CABLE_LENGTH", "AZURE")
orig_cable_len = fvs[self.INTF]
fvs = self.config_db.get_entry("PORT", self.INTF)
orig_speed = fvs["speed"]

if orig_speed == "100000":
test_speed = "40000"
elif orig_speed == "40000":
test_speed = "100000"
test_cable_len = "0m"

# check if the lossless profile for the test speed is already present
fvs = dict()
new_lossless_profile = "pg_lossless_{}_{}_profile".format(test_speed, orig_cable_len)
fvs = self.app_db.get_entry("BUFFER_PROFILE_TABLE", new_lossless_profile)
if len(fvs):
profile_exp_cnt_diff = 0
else:
profile_exp_cnt_diff = 1

# get the orig buf profiles attached to the pgs
self.get_asic_buf_pg_profiles()

# change cable length to 'test_cable_len'
self.change_cable_len(test_cable_len)

# change intf speed to 'test_speed'
dvs.runcmd("config interface speed {} {}".format(self.INTF, test_speed))
test_lossless_profile = "pg_lossless_{}_{}_profile".format(test_speed, test_cable_len)
# buffer profile should not get created
self.app_db.wait_for_deleted_entry("BUFFER_PROFILE_TABLE", test_lossless_profile)

# buffer pgs should still point to the original buffer profile
orig_lossless_profile = "pg_lossless_{}_{}_profile".format(orig_speed, orig_cable_len)
self.app_db.wait_for_field_match("BUFFER_PG_TABLE", self.INTF + ":3-4", {"profile": "[BUFFER_PROFILE_TABLE:{}]".format(orig_lossless_profile)})
fvs = dict()
for pg in self.pg_name_map:
fvs["SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE"] = self.buf_pg_profile[pg]
self.asic_db.wait_for_field_match("ASIC_STATE:SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", self.pg_name_map[pg], fvs)

# change cable length to 'orig_cable_len'
self.change_cable_len(orig_cable_len)

# change intf speed to 'test_speed'
dvs.runcmd("config interface speed {} {}".format(self.INTF, test_speed))
if profile_exp_cnt_diff != 0:
# new profile will get created
self.app_db.wait_for_entry("BUFFER_PROFILE_TABLE", new_lossless_profile)
self.check_new_profile_in_asic_db(new_lossless_profile)
# verify that buffer pgs point to the new profile
fvs = dict()
for pg in self.pg_name_map:
fvs["SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE"] = self.new_profile
self.asic_db.wait_for_field_match("ASIC_STATE:SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", self.pg_name_map[pg], fvs)

else:
# verify that buffer pgs do not point to the old profile since we cannot deduce the new profile oid
fvs = dict()
for pg in self.pg_name_map:
fvs["SAI_INGRESS_PRIORITY_GROUP_ATTR_BUFFER_PROFILE"] = self.buf_pg_profile[pg]
self.asic_db.wait_for_field_negative_match("ASIC_STATE:SAI_OBJECT_TYPE_INGRESS_PRIORITY_GROUP", self.pg_name_map[pg], fvs)
finally:
if orig_cable_len:
self.change_cable_len(orig_cable_len)
if orig_speed:
dvs.runcmd("config interface speed {} {}".format(self.INTF, orig_speed))
dvs.runcmd("config interface shutdown {}".format(self.INTF))

0 comments on commit 7f80f06

Please sign in to comment.